Closed Bug 1215182 Opened 9 years ago Closed 9 years ago

[css-grid] Make our "Implicit Named Areas" handling match the spec

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

Attachments

(2 files)

fix
(deleted), patch
dholbert
: review+
Details | Diff | Splinter Review
reftest
(deleted), patch
Details | Diff | Splinter Review
https://lists.w3.org/Archives/Public/www-style/2015Oct/0040.html It appears there are no conditions whatsoever other than the existence of at least one line of the form X-start or X-end for the "area" X to exist.
Attached patch fix (deleted) — — Splinter Review
Attachment #8674344 - Flags: review?(dholbert)
I'm forgetting why we need to maintain these areas in the first place. Can we do away with it & just deal with lines? In particular, Tab said the following in the thread that you linked to: # The spec doesn't actually "create" any such areas; they exist as a # spec concept for convenience only. The placement properties never # care about areas, they just check for lines with particular names. https://lists.w3.org/Archives/Public/www-style/2015Oct/0052.html Given this, I'm wondering why we're creating structures for these areas internally. Was it just an optimization?
I think the answer is basically "yes, it's an optimization". IIUC: given a grid item with "grid-row-start:A", the lazy thing to do is to search first for "A-start" in our explicit-line-list [and also look for explicit areas named "A"], and if we fail, then search our explicit-line-list *again* for just "A". (This is basically what tab described in the post I linked to.) For authors who just use "A" naming for their lines (who don't name anything "A-start" etc.), the first search is doomed & wasteful. This sucks, because we have to perform it for each grid item that uses a named line. So, the ImplicitNamedAreas object (a hash set populated during a precomputation step) lets us optimize these doomed searches away. Does this make sense? (I slightly wonder whether we should make the further optimization of hashing the actual line-names, so we can optimize away the second search as well, and perhaps end up with better code-sharing/symmetry as a result. For this to help, authors would have to be using completely-undefined line names, though, and maybe that's not a case that's worth optimizing for.)
Comment on attachment 8674344 [details] [diff] [review] fix Review of attachment 8674344 [details] [diff] [review]: ----------------------------------------------------------------- r=me -- I've just got one comment-nit, below: ::: layout/generic/nsGridContainerFrame.cpp @@ +1162,5 @@ > nsGridContainerFrame::AddImplicitNamedAreas( > const nsTArray<nsTArray<nsString>>& aLineNameLists) > { > // http://dev.w3.org/csswg/css-grid/#implicit-named-areas > + // A line named 'x-start' or 'x-end' implies an implicit named area 'x'. To me, this implies that we'll be creating an actual area (with resolveable start/end lines in all dimensions). But that's not what actually happens. Might be worth adding a parenthetical clarifying this -- e.g.... // (But only explicitly-defined edges of this implicit area can be // matched against during grid-item placement.) ...or something along those lines. (I think this is accurate? Please correct me if not.) So for example a row-line named "x-start" will create an implicit named area "x", but it doesn't actually influence how "grid-row-end: x" is resolved, or how "grid-column-start/end" are resolved. (It means we'll take a slightly different codepath -- we'll traverse the HasImplicitNamedArea()-guarded code -- but we'll fail to match there, I think.)
Attachment #8674344 - Flags: review?(dholbert) → review+
Comment on attachment 8674345 [details] [diff] [review] reftest Review of attachment 8674345 [details] [diff] [review]: ----------------------------------------------------------------- One drive-by nit on the reftest patch: ::: layout/reftests/css-grid/grid-placement-implicit-named-areas-001.html @@ +6,5 @@ > +<html><head> > + <meta charset="utf-8"> > + <title>CSS Grid Test: implicit named areas</title> > + <link rel="author" title="Mats Palmgren" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1215182"> > + <link rel="help" href="http://dev.w3.org/csswg/css-grid/#abspos-items"> I think this #abspos-items anchor is incorrect. (probably from copypaste error) You probably really want to link to http://dev.w3.org/csswg/css-grid/#grid-placement-slot (This applies to the testcase as well as the reference case)
(In reply to Daniel Holbert [:dholbert] from comment #4) > I think the answer is basically "yes, it's an optimization". Yes, it's just an optimization now.
(In reply to Daniel Holbert [:dholbert] from comment #5) > > // http://dev.w3.org/csswg/css-grid/#implicit-named-areas > > + // A line named 'x-start' or 'x-end' implies an implicit named area 'x'. > > To me, this implies that we'll be creating an actual area (with resolveable > start/end lines in all dimensions). But that's not what actually happens. I removed that line. The spec link is sufficient. (I think you're comment here illustrates perfectly why "implicit named area" is not only a useless, but also harmful, concept.) (In reply to Daniel Holbert [:dholbert] from comment #6) > You probably really want to link to > http://dev.w3.org/csswg/css-grid/#grid-placement-slot Fixed, thanks.
https://hg.mozilla.org/mozilla-central/rev/7b8f601030db https://hg.mozilla.org/mozilla-central/rev/dc90c209ea61
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: