1800566 - [css-grid] WPT test subgrid/line-names-002.html and line-names-005.html fail in Firefox

Status: RESOLVED FIXED

(Core :: Layout: Grid, defect)

Description

[Daniel Holbert [:dholbert]]

This WPT test fails just in Firefox:
https://wpt.fyi/results/css/css-grid/subgrid/line-names-002.html

Direct link to test:
https://wpt.live/css/css-grid/subgrid/line-names-002.html
https://wpt.live/css/css-grid/subgrid/line-names-002-ref.html

It happens to be included in the interop2022 test suite, in the subgrid category. Hence: filing under that metabug. (Here's the link for all such tests that we fail, FWIW:
https://wpt.fyi/results/css/css-grid/subgrid?label=master&label=experimental&aligned&view=subtest&q=label%3Ainterop-2022-subgrid%20firefox%3A%21pass )

Looks like this test used to pass, but it was adjusted in bug 1752957 in response to csswg issue https://github.com/w3c/csswg-drafts/issues/6905 .

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

Looks like this test used to pass, but it was adjusted in bug 1752957 in response to csswg issue https://github.com/w3c/csswg-drafts/issues/6905 .

That test also changed the reference case for line-names-005-ref.html, so I'm going to assume our failure for that test has the same or a similar root cause.

wpt.fyi:
https://wpt.fyi/results/css/css-grid/subgrid/line-names-005.html
Direct link to test:
http://wpt.live/css/css-grid/subgrid/line-names-005.html
http://wpt.live/css/css-grid/subgrid/line-names-005-ref.html

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (redued from first test failure in line-names-005.html)

Here's a reduced testcase from the first section of line-names-005.html that Firefox fails (where we stretch an item leftwards to column 7, when the expected-result is for it to only stretch leftwards to column 8).

In this reduced testcase, we size the grid item to fill the right 3/4 of the grid. Chrome sizes it to fill the right half of the grid.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (reduced from second test failure in line-names-005.html)

Here's a reduced testcase from the second section of line-names-005.html that Firefox fails (where we stretch an item leftwards to fill all the way to the left edge of the grid, when the expected-result is for it to only occupy the rightmost column).

In this reduced testcase, we size the grid item to fill the whole (cyan-outlined) subgrid, whereas Chrome has it only occupy the rightmost column of the subgrid.

Comment 4

[Daniel Holbert [:dholbert]]

I think one key note from the github discussion is in https://github.com/w3c/csswg-drafts/issues/6905#issuecomment-1026346391 :

The important part to me was the clarification that resolving the number of explicit tracks in a subgrid can't happen until placement of the subgrid item in the parent has been done.

(I don't yet see where that insight comes from in the spec text, but it's entirely likely I just haven't found it yet.)
(EDIT: Found it, see comment 6.)

Right now, we do things in the opposite order; we resolve the number of explicit tracks in the subgrid first, and then place the subgrid items (constrained to those tracks). That happens by virtue of passing subgridRange here, before we place the subgrid items:
https://searchfox.org/mozilla-central/rev/3bd65516eb9b3a9568806d846ba8c81a9402a885/layout/generic/nsGridContainerFrame.cpp#4613,4624-4625

  subgridRange = &subgrid->SubgridCols();
...
LineNameMap colLineNameMap(gridStyle, mAreas, aState.mColFunctions,
                           parentLineNameMap, subgridRange,

In my attached testcase 2, when current Firefox tries to place the item, we already know that the subgrid has 4 lines (3 tracks) and can't create implicit lines, so we clamp foo 6 to be line #4 in the outer grid, and we look backwards (non-inclusive) from that line to resolve span a, and we go all the way to the beginning of the subgrid.

But it sounds like the test is requiring that we place the item in a less-constrained way, such that we're able to resolve line a as the 4th line, somehow... and then I guess we size the subgrid after that, and slide the item inwards to constrain it after-the-fact, or something?

(That seems to be what Chromium/WebKit are doing with testcase 2, at least -- they're recognizing that grid-column-start:a is a known line-name, but they end up using the previous line as the start line, and they use a i.e. line#4 as the end line...)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

Here's a third testcase, which is just a simplified version of the previous testcase (testcase 2), expanded to test a bunch of different grid-column-end values.

We agree with WebKit/Chrome for the first few boxes, up through grid-column-end: 3, but we disagree on the cases after that.

We treat the grid-column-end: 4 and 5 and foo cases as being equivalent to 3; in all those cases, we use the final line of the subgrid (line #3) as the clamped column-end-line for the item, and we search leftwards to find a line named a and we fail to find one, so we stop at the start of the subgrid.

WebKit/Chrome apparently honor the larger-than-3 grid-column-end values at first, and that lets them start their "item column-start line discovery-walk" from further rightwards, which lets them discover the line that is actually named a (line #3 in this case). And then I guess they shift the item leftwards after that, to place it in bounds for the subgrid (taking its start edge off of a but keeping it with a span of 1), or something to that effect.

Comment 6

[Daniel Holbert [:dholbert]]

Maybe the key bit of spec text is this (note, I'm adding my own numbering i/ii/iii to phrases, to reference them below)
https://www.w3.org/TR/css-grid-2/#subgrid-implicit

(i) The subgrid does not have any implicit grid tracks in the subgridded dimension(s).
(ii) Hypothetical implicit grid lines are used to resolve placement as usual when the explicit grid does not have enough lines
(iii) however each grid item’s grid area is clamped to the subgrid’s explicit grid (using the same procedure as for clamping placement in an overly-large grid).

I guess (ii) explains why Chrome/WebKit are starting their "item column-start line discovery-walk" from beyond the bounds of the subgrid.
And (iii) references why it gets shifted inwards (breaking start-alignment with the line a that it was trying to start on) and gets a span of 1 -- the "clamping placement in an overly large grid" procedure is detailed here:
https://www.w3.org/TR/css-grid-2/#overlarge-grids

To clamp a grid area:

• If the grid area would span outside the limited grid, its span is clamped to the last line of the limited grid.
• If the grid area would be placed completely outside the limited grid, its span must be truncated to 1 and the area repositioned into the last grid track on that side of the grid.

In this case, that second bullet point explains the WebKit/Chrome behavior.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1800566: Wait to clamp lines for items in a subgrid until after we've resolved both the start and the end lines. r?emilio,TYLin,#layout

This is a slight change to our "clamping over-large grid" procedure, to more
closely match the spec requirements.

We use this same procedure for two different things:
(a) to clamp the placement of grid items that are using extremely large grid line indices -- beyond 10,000.
(b) to clamp grid lines that are simply outside the bounds of their subgrid.

The second case is much more mundane, but the spec says to "[use] the same
procedure" at https://www.w3.org/TR/css-grid-2/#subgrid-implicit

Before this patch, we were eagerly clamping the start/end lines of grid items,
up-front, to keep each line within the limits. This approach doesn't quite
match the spec text linked above, which says to resolve the grid placement
first (i.e. resolve both the start and end lines, possibly to something
out-of-bounds) and only then clamp.

The distinction is subtle, but it makes a difference for cases like the bottom
half of https://bug1800566.bmoattachments.org/attachment.cgi?id=9369740

See comments in the patch for more details.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de03aedeb549
Wait to clamp lines for items in a subgrid until after we've resolved both the start and the end lines. r=emilio,layout-reviewers

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/de03aedeb549