Closed Bug 1297189 Opened 3 years ago Closed 3 years ago

Implicit grid areas need boundary line numbers exposed to dev tools

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(1 file, 6 obsolete files)

Implicit areas currently report 0 for all boundary line numbers.
Attached patch gridAreaLineNames1.patch (obsolete) — Splinter Review
This exposes the names of all the lines that bound grid areas, both explicit and implicit. To do that properly, the code also fixes a bug in the GetLineNamesAtIndex function where it didn't properly account for the expansion of the implicit grid. Tests have been expanded to track these cases. One TODO remains in the dom/grid/test/chrome/test_grid_implicit.html test: explicit line names applied to implicitly created lines are not yet reported.
Attachment #8785445 - Flags: review?(mats)
Attached patch gridAreaLineNames2.patch (obsolete) — Splinter Review
Dramatically simplified the code by stripping out the no-longer-necesary grid line name hash.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6096830bad02
Attachment #8785445 - Attachment is obsolete: true
Attachment #8785445 - Flags: review?(mats)
Attachment #8785466 - Flags: review?(mats)
Can you explain why you make the extent of an item that uses an implicit
named area define the extent for that area?  It makes no sense to me.

I think that the first rev of the patch was basically correct, with those
parts removed.  I think the lines for an implicit named area should be
derived from the container style only, as you did in AddImplicitNamedAreas
in rev 1.
Flags: needinfo?(bwerth)
If I understand your question correctly, I did it this way because the practical boundaries of partially-specified grid areas is dictated by the "cursor" calculations in nsGridContainerFrame::Grid::PlaceGridItems. I didn't want to replicate that algorithm to figure out the missing boundaries. In other words, I don't believe we CAN derive the missing boundary lines purely from the container style, since items placed into the grid move that cursor around.

Isn't it correct that an item placed in area X has the same boundaries as area X, by definition? That seems the most sensible behavior to me from a developer expectation perspective -- the item fills the area entirely -- and I don't think it violates the spec.

I'm unclear on what is the layout behavior if multiple items are placed in the same area, and I'm having trouble determining that from the spec.
Flags: needinfo?(bwerth) → needinfo?(mats)
> Isn't it correct that an item placed in area X has the same boundaries
> as area X, by definition?

Well, line name resolution is based on named lines and each line in
a grid item's grid area is resolved independently, so some names may
resolve to an explicitly named line and some to a line name associated
with an implicit area.

An "implicit area" is really just a switch to enable the -start/-end
part in this rule:
https://drafts.csswg.org/css-grid/#grid-placement-slot
So, given an explicit line X-start (or X-end) in grid-template-rows/
columns, it says that an item using the name X (by itself) also matches
explicit lines named X-start/X-end, and with higher priority than any
lines named X.  The spec invented the "implicit area" to explain this,
but personally I think it's a rather misleading concept which I tried
to explain to the editors here:
https://lists.w3.org/Archives/Public/www-style/2015Oct/0069.html

Anyway, let's look at all the cases and see what the implicit area
boundaries are for each.

For each axis, the boundary lines for an implicit area X is defined as:
1. we have one or more X-start (in the relevant grid-template-*)
   => the start line is the first of those
2. we have no X-start
   => the start is the first implicit line on the start-side
3. we have one or more X-end
   => the end line is the first of those
4. we have no X-end
   => the end is the first implicit line on the end-side

If start > end then swap them.
(if start==end I tend to think we should present it as an area that
starts and ends in the same line, even though any item using start==end
for placement defaults to covering one track (end=start+1))

Note that in 2 and 4 there must be a X-start or X-end line in the other
axis, otherwise the implicit area would not exist.

Implicit areas also have bounds when there are no items in the grid,
and items don't affect implicit area boundaries at all.


An example item that resolves some of its lines to an area is:
  grid-column: X / X 1;
where the container has:
  grid-template-columns: [X-start] 0 [X];
In this case the item's start X is resolved as an area (matches X-start)
but the end X is resolved as the explicit line X (resulting in 1 / 2).

Another example:
  grid-area: X;
where the container has:
  grid-template-columns: [X-start] 0 [X];

"grid-area: X" is simply short for "grid-area: X/X/X/X" and as above,
the column start X is resolved X-start but the column end is resolved
as the explicit line X.  So even "grid-area: X" doesn't necessarily
correspond to an area X (which in this case also goes from X-start
to the first implicit line on the end-side).

Another example:
  grid-area: X;
where the container has:
  grid-template-columns: [X-start] 0 [X] 0 [X-end];

Here we resolve the end X to X-end since it has higher priority than
the X in line 2.  However, if the item had "grid-column: X / X 1",
then the end X is only resolved as X, not X-end, so we get line 2.

Let me know if anything is still unclear.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #6)

This all helps me a lot, thank you. I see now that implicit areas are a side-effect of the spec, and if we expose them in the dev tools, we'll be treating them as more of a concrete concept than they actually are. I do think there is value in doing so, and here are some principles I am proposing to guide us in our implementation:

We treat an implicit area as well-formed if:
a) It has four boundary lines defined explicitly, in a non-zero-area rectangle, or
b) It appears in the grid-template-areas declaration (giving it the four boundary lines in case a, above), or
c) An element has been placed in the area, giving it a definite place in the grid (possibly based on partial line specification).

We can definitely report/draw/inspect the implicit grid areas in these cases, and I think we should, even if case c feels different from the others. To me, I feel this lets the tools best reflect the developer's intentions without assuming too much.

There are cases where an implicit grid area is badly-formed:
d) Less than four boundary lines (and no element placed in the area), or
e) Four boundary lines where end < start on at least one axis, or
f) Four boundary lines where end = start on at least one axis.

In all these cases, if we could determine where an element WOULD APPEAR if it was placed in the area, then I feel we should consider doing so. I feel that case d is too hard and too dangerous -- it requires implementing the reflow algorithm a second time to determine where the element would appear. Case e is doable, but weird. Section 9.3.1 in the spec tells us that reflow will swap the start/end in this case, so we could report it, but it seems to me that we should also report the swap. Case f is also weird; 9.3.1 tell us to "remove the end line", which implies a span 1 on that axis. If we report this case with "fixed" boundaries to represent that span 1, then we could be referring to a track/cell that is outside the grid!

So, given all that, I propose that we report cases a through c as objects in the "areas" property, but for cases d through f we do not. In all the cases, all the explicit lines would get reported through the "lines" properties. For case c, we would add the missing lines to match the actual location of the grid area.

Alternatively, we could report all implicit areas, including cases d through f, and put the burden on the dev tools to note that they are poorly formed (and choose to either render them or not). I prefer not reporting these cases.
Flags: needinfo?(mats)
> b) It appears in the grid-template-areas declaration

You mean we have both implicit/explicit areas with the same name?

It's certainly possible to have that:
  grid-template-areas: ". a .";
  grid-template-columns: [a-start] 0 0 0;
and in this case matching is even more complicated since
"grid-columns: a" will use the implicit area for the start
line and the explicit area for the end line, i.e. 1/3.

explicit area "a": column 2/3
implicit area "a": column 1/5

Do we want to present both these (overlapping) at the same time?
something like this:
  1         2         3       4     5(implicit)
a-start a-start    a-end          a-end
            < expl. a >
  < implicit a                      >

I'm not sure this is a good idea.  I think the devtools display
should encourage the user to think primarily in line names because
that's how names are actually matched.

Or, we could merge areas with the same name like so:
  1         2         3       4     5(implicit)
a-start a-start    a-end          a-end
  <  "area"        a  >

to display more directly "this is what you get if you use
'grid-columns: a'"

Or, perhaps we should just skip implicit areas altogether?
I doubt implicit area names will be used much in practice.

Note that track 4 (line 5) above might not even exist so it might be
hard to present line names for all edges of an implicit area.


> c) An element has been placed in the area

I tend to think we shouldn't involve items in this at all.
If an item is "placed in the area" isn't a well defined term
and can be quite complicated to keep track of.


So I'm kind of leaning towards only displaying explicit areas
(with their implicit line names) and explicit line names, like so:
  1         2         3       4
a-start a-start    a-end       
            < expl. a >

We can always add implicit areas at a later time if authors asks
for them...
Flags: needinfo?(mats)
Perhaps Jen and the devtools team have some thoughts on the usefulness
of presenting implicit areas? and if so how they intend to present them?
(In reply to Mats Palmgren (:mats) from comment #8)
 
> It's certainly possible to have that:
>   grid-template-areas: ". a .";
>   grid-template-columns: [a-start] 0 0 0;
> and in this case matching is even more complicated since
> "grid-columns: a" will use the implicit area for the start
> line and the explicit area for the end line, i.e. 1/3.
> 
> explicit area "a": column 2/3
> implicit area "a": column 1/5
> 
> Do we want to present both these (overlapping) at the same time?

I think the greatest value the dev tools will bring is if they show where an item WOULD appear if it were to be put into the named area, so there's no value in us representing two areas with the same name. Which one is the "real" area a? Instead, we would represent this example as having one area a, I think we should report it as implicit, and elements placed in it would span 1/3 (start with the explicit line and end with the implicit line at the far edge of the explicit area). Indeed, I believe that's where layout would put an element placed in area a, and if I'm wrong, I would want to see us report wherever layout does put it as the bounds of area a.

> something like this:
>   1         2         3       4     5(implicit)
> a-start a-start    a-end          a-end
>             < expl. a >
>   < implicit a                      >

I don't think layout would create line 5. This layout should have 3 tracks and 4 lines, yes? Again, whatever layout does according to spec, that's what we should report as area a. We would characterize it as implicit because if it has any contributing factor of an explicit line extending (or defining) the explicit area, then that's what we mean when we call something implicit. Sort of a 1% doctrine -- if there's any implicit-ness at all in the style, then it's an implicit area.

> I'm not sure this is a good idea.  I think the devtools display
> should encourage the user to think primarily in line names because
> that's how names are actually matched.

I agree two areas is a bad idea. Regarding line names, I'm sure the dev tools will provide lots of mode-switching such that the user can think in line names or think in areas or both, whatever is most sensible for the layout and the user. As long as we report information that is actionable, then the user will decide how to think about it. Thus my proposed litmus test about reporting areas that show where an element either is or would be if assigned to that area.

> I tend to think we shouldn't involve items in this at all.
> If an item is "placed in the area" isn't a well defined term
> and can be quite complicated to keep track of.

I realize there are likely edge cases, but the common case seems easy enough and I think provides a lot of value. The code in the patch accomplishes it, I think. If an element is put in a grid aligned to 4 lines with the the same name as would happen with a grid-area: <name> style (patch 1 / attachment 8785445 [details] [diff] [review] does this in nsGridContainerFrame.cpp line 2993), those lines will resolve somewhere according to spec, and that is the practical boundary of the area. If the developer does something more complicated with placing with <name>-start lines and the nth <name>-end line or something, then the named area might not be reported at all if there aren't 4 boundary lines defining it. But I think we can live with that. The worst case scenario is that the code would calculate the boundaries of an unused named area, display them, and then when the developer put an element in that named area, the layout actually places the element somewhere else. I think we can build some tests around this and fix these cases and provide good value.

I'm going to get to work putting back in the grid line name hash stuff I took out in patch 2 / attachment 8785466 [details] [diff] [review] since it seems like we'll need it for the implicit area boundaries when no element is placed in it -- something we're both agreeing is correct behavior, I believe.
Attached patch gridAreaLineNames3.patch (obsolete) — Splinter Review
This patch implements the plan laid out in comment 10, with these exceptions:
a) Poorly-formed implicit areas are still reported (no test to verify)
b) No test to verify that completely specified, properly-formed implicit areas are reported (though the code should report them)
Attachment #8785466 - Attachment is obsolete: true
Attachment #8785466 - Flags: review?(mats)
Attachment #8786148 - Flags: feedback?(mats)
(In reply to Brad Werth [:bradwerth] from comment #10)
> > explicit area "a": column 2/3
> > implicit area "a": column 1/5
> > 
> > Do we want to present both these (overlapping) at the same time?
> 
> I think the greatest value the dev tools will bring is if they show where an
> item WOULD appear if it were to be put into the named area, so there's no
> value in us representing two areas with the same name.

Fair enough, let's present a "merged" area that takes both into account.
Reporting it as implicit if any edge came from an explicit line name
and reporting it as explicit area otherwise seems fine to me.

> As long as we report information that is actionable,
> then the user will decide how to think about it.

Well, in all fairness, we both misunderstood implicit areas on the first
reading of the spec.  I'm certain that most authors will misunderstand it
too.  That's why I think the UI should emphasise line names and encourage
authors to think in terms of them because that's how the actual placement
is done.  Having a different mental model than the spec will only lead to
confusion IMHO.

But sure, let's try the "merged" areas concept and see how that works...

> > I tend to think we shouldn't involve items in this at all.
> > If an item is "placed in the area" isn't a well defined term
> > and can be quite complicated to keep track of.
> 
> I realize there are likely edge cases, but the common case seems easy enough
> and I think provides a lot of value. 

I think we should calculate areas as described in comment 6, with areas
from 'grid-template-areas' merged in.  That should result in the same
area extents as if an item is placed in it using "grid-area:X".
I'm strongly against tracking actual item placement for this.
Comment on attachment 8786148 [details] [diff] [review]
gridAreaLineNames3.patch

>layout/generic/nsGridContainerFrame.cpp
> struct nsGridContainerFrame::GridArea
> [...]
>+  nsString mName;

I think we should create a separate class for this instead, e.g.
class NamedGridArea {
  GridArea mArea;
  nsString mName;
}

I'll get to the reason at the end...

>+  // Hoist the offset values to assist in explicit line name mapping later.
>+  mExplicitGridOffsetCol = aGrid.mExplicitGridOffsetCol;
>+  mExplicitGridOffsetRow = aGrid.mExplicitGridOffsetRow;

Is this necessary?  Aren't these values the same as
gridReflowInput.m[Col|Row]Functions.mExplicitGridOffset ?

> nsGridContainerFrame::AddImplicitNamedAreas(
>+      bool found = false;
>+      bool isStart = false;
>+      uint32_t indexOfSuffix;
>+      if (Grid::IsNameWithStartSuffix(name, &indexOfSuffix)) {
>+        found = true;
>+        isStart = true;
>+      } else if (Grid::IsNameWithEndSuffix(name, &indexOfSuffix)) {
>+        found = true;
>+      }

I think this can be simplified to something like this:

  uint32_t indexOfSuffix;
  bool found = Grid::IsNameWithStartSuffix(name, &indexOfSuffix);
  bool isStart = found;
  if (!found) {
    found = Grid::IsNameWithEndSuffix(name, &indexOfSuffix);
  }

Do we acrtually need aLineInfo? can't the line numbers be derived
from the loop index 'i'?  It seems we can just fill in the first
-start/-end line number for each name directly here.  Then merge in
the grid-template-areas extents on top of that, if present.

> nsGridContainerFrame::InitImplicitNamedAreas(const nsStylePosition* aStyle)
>+
>+  AddImplicitNamedAreas(aStyle->mGridTemplateRows.mLineNameLists,
>+                        nullptr,
>+                        true);
>+
>+  AddImplicitNamedAreas(aStyle->mGridTemplateColumns.mLineNameLists,
>+                        nullptr,
>+                        false);
>+

Nit: I think the spacing here is excessive.  Drop the empty lines and put
"nullptr, true" on the same line please.

>+nsGridContainerFrame::UpdateImplicitNamedAreas(const nsStylePosition* aStyle,
>+{
>+  AddImplicitNamedAreas(aStyle->mGridTemplateRows.mLineNameLists,
>+                        GetComputedTemplateRowLines(),
>+                        true);
>+
>+  AddImplicitNamedAreas(aStyle->mGridTemplateColumns.mLineNameLists,
>+                        GetComputedTemplateColumnLines(),
>+                        false);
>+

Same here.

>+   * Check for any under-specified implicit areas and find the items

Please remove all this.  Which means we don't need to have an mName
in each item's GridArea.
Attachment #8786148 - Flags: feedback?(mats) → feedback+
(In reply to Mats Palmgren (:mats) from comment #12)

> I think we should calculate areas as described in comment 6, with areas
> from 'grid-template-areas' merged in.  That should result in the same
> area extents as if an item is placed in it using "grid-area:X".
> I'm strongly against tracking actual item placement for this.

Tell me more about your opposition to this. Earlier in your comments, it seems like you agree with the principle that we should report named grid areas that reflect where an item would be if assigned to that area. If that's the case, then determining the boundaries of a grid area by examining the layout placement of an element inside it achieves that goal by definition. Is your opposition because of the inherit inefficiency of the O(n^2) search algorithm? Because of code ugliness/complexity? Contamination of concerns with names being added to the Grid structure? Tell me more.

My concern is that the approach in comment 6 is either:
a) accurately re-implementing layout itself, which is duplication of some complex code, and to me feels dangerous and unmanageable in the long run
b) sometimes wrong and therefore not useful
Flags: needinfo?(mats)
An area's existence, and extents, does not depend on an item being 
"placed in it" - all areas are always there even in a grid without
any items.  This is an important principle in the Grid spec - all
name lookup is independent of the grid items, they are matched
solely against grid container properties.  Changing the areas based
on item placement would also go against the principle that an area
should reflect where an item would be placed if it used that name.
Violating these principles would be confusing to users since it
might give them the idea that an area somehow depend on an item
using it, which is false.

> a) accurately re-implementing layout itself

That's an exaggeration/misunderstanding.  We only need to make
AddImplicitNamedAreas record the associated line numbers, do a name
lookup in 'grid-template-areas' and merge them, this doesn't seem hard.

It might be possible to reuse ResolveLineRange instead if that's
simpler, by creating a suitable nsStyleGridLine and calling it.

> b) sometimes wrong and therefore not useful

All areas and line numbers we report should be 100% accurate.
I don't see any reason why they would be sometimes wrong -
do you have an example to illustrate?
Flags: needinfo?(mats)
Indeed you are correct. Looking through the code, there's no way that PlaceDefinite can affect anything other than mExplicitGridOffsetCol and mExplicitGridOffsetRow, which is accounted for. So there's no example to be found where placement changes the shape of a grid area. If I do what you propose and re-use ResolveLineRange, then the replication of the layout algorithm is minimal. Thank you for talking me through it. I'll implement it as you propose.
Attached patch gridAreaLineNames4.patch (obsolete) — Splinter Review
Resolves line boundaries of implicit grid areas by re-running ResolveLineRange.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cad7f8e00bd
Attachment #8786148 - Attachment is obsolete: true
Attachment #8786967 - Flags: review?(mats)
Comment on attachment 8786967 [details] [diff] [review]
gridAreaLineNames4.patch

>layout/generic/nsGridContainerFrame.cpp
>+  // Replicate part of PlaceGridItems to re-establish the name maps
>+  LineNameMap colLineNameMap(aState.mGridStyle->mGridTemplateColumns,
>+                             aGrid.mNumRepeatCols);
>+  LineNameMap rowLineNameMap(aState.mGridStyle->mGridTemplateRows,
>+                             aGrid.mNumRepeatRows);

I think it would be better to update the areas at the end of
PlaceGridItems instead, if possible.  I think(?) we have all the data
we need there, so creating new LineNameMap shouldn't be necessary.

>+      // Now resolve the -start and -end lines for the area. We'll use
>+      // these names for both the row lines and the column lines.

Hmm, shouldn't we resolve "grid-column: X / X" rather than
"grid-column: X-start / X-end"?  I think "X-start" would also try
to match lines named "X-start-start", i.e. matching "X-start" as
an implicit area name.

>+
>+      // FIXME: We need to hoist this name to be associated with the implicit
>+      // line when reported through the GetGridFragments API.

Is this comment relevant?

>-    // Generate the line info properties. We need to provide the number of
>-    // repeat tracks produced in the reflow.
>+    /**
>+     * Generate the line info properties. We need to provide the number of
>+     * repeat tracks produced in the reflow. Only explicit names are assigned
>+     * to lines here; the mozilla::dom::GridLines class will later extract
>+     * implicit names from grid areas and assign them to the appropriate lines.
>+     */

Please use // for this comment.

(/** should only be used for declarations, to guide tools generating
documentation)

>     Properties().Set(GridRowLineInfo(), rowLineInfo);
> 
>+
>     // Generate area info for explicit areas. Implicit areas are handled

Spurious empty line.
Flags: needinfo?(bwerth)
Attached patch gridAreaLineNames5.patch (obsolete) — Splinter Review
Addresses all the issues raised in comment 18. Moves calculation of implicit line boundaries into the PlaceGridItems function.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55a431880904
Attachment #8786967 - Attachment is obsolete: true
Attachment #8786967 - Flags: review?(mats)
Flags: needinfo?(bwerth)
Attachment #8787327 - Flags: review?(mats)
Comment on attachment 8787327 [details] [diff] [review]
gridAreaLineNames5.patch

>layout/generic/nsGridContainerFrame.cpp
>+  // While we have the LineNameMaps created, update the line boundaries of the
>+  // implicit grid areas.

Nit: The "While we have the LineNameMaps created, " doesn't seem
important enough to comment on.  I think just "Update ..." is fine.

>+  if (mAreas) {

Can we add "&& HasAnyStateBits(NS_STATE_GRID_GENERATE_COMPUTED_VALUES"
to avoid doing this in the common case?

r=mats on the layout/ part

Please ask a DOM peer to review the dom/ part.
Attachment #8787327 - Flags: review?(mats) → review+
Attached patch gridAreaLineNames6.patch (obsolete) — Splinter Review
Exposes boundaries of implicit grid areas to dev tools API. Addresses Mats' r= issues in comment 20.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40eed8c2f064
Attachment #8787327 - Attachment is obsolete: true
Attachment #8787358 - Flags: review?(bzbarsky)
Comment on attachment 8787358 [details] [diff] [review]
gridAreaLineNames6.patch

>+      auto areaInfo = iter.Data();

Shouldn't that be "auto&"?  Otherwise aren't you making a copy?

>+      if (!namesSeen.Contains(areaInfo.mName)) {

How many names can appear here?  Would it make sense to have a hashset instead of an array?  Seems to me like it would....

>+        nsString areaName;
>+        area->GetName(areaName);
>+        nsAutoString nameToAdd(areaName);

  nsAutoString nameToAdd;
  area->GetName(nameToAdd);

I didn't really review the tests; I don't know enough about grid to tell how they should behave.  Please check with mats about whether he reviewed them...

r=me on the code bits in dom/, and sorry for the lag.
Attachment #8787358 - Flags: review?(bzbarsky) → review+
Addresses bz's issues in comment 22. Now r=mats, r=bz. Mats, please address Boris's question whether or not you have reviewed the tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5594150d299
Flags: needinfo?(mats)
Attachment #8787358 - Attachment is obsolete: true
The tests looks good to me.
Flags: needinfo?(mats)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd249dba87c2
Expose implicit grid areas via dev tools API. r=mats, r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd249dba87c2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.