1833482 - Firefox fails wpt tests css-grid/subgrid/subgrid-baseline-003.html and subgrid-baseline-004.html

Status: RESOLVED FIXED

(Core :: Layout: Grid, defect)

Description

[Daniel Holbert [:dholbert]]

wpt.fyi:
https://wpt.fyi/results/css/css-grid/subgrid/subgrid-baseline-003.html
https://wpt.fyi/results/css/css-grid/subgrid/subgrid-baseline-004.html

The failure screenshots look similar, and probably share a single underlying root cause. Hence, tracking both here.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1

Here's a reduced testcase. It looks like we're just not doing align-self:baseline alignment at all, for items in a subgrid, I guess? (We snap "C" and "D" to the top of the container here, not even baseline aligning those two items with each other. Whereas Chrome/Safari other browsers baseline-align those items with the other items outside of the subgrid.)

Comment 2

[Daniel Holbert [:dholbert]]

Pernosco trace, with a variant of the testcase where I just removed D for simplicity:
https://pernos.co/debug/CEWQ1qk4AoYz4c4s9lrNbw/index.html#f{m[6us,ATLohg_,t[+g,FhVe_,f{e[6us,ATBT4g_,s{af5ECQAAA,bCvU,uErARfQ,oEsjfXA___/

It looks like:
(A) the subgrid owns the canonical copy of the GridItemInfo struct for its own items (stored on the Subgrid() frame-property, in the mGridItems member)
(B) we copy those GridItemInfo objects into a struct used by the outer grid (mItems on the outer grid's gridReflowInput stack-variable), in CollectSubgridItemsForAxis
(C) The outer-grid's mItems array (populated in (B)) is what gets passed to nsGridContainerFrame::Tracks::InitializeItemBaselines, and that results in us setting mBaselineOffset with the actual baseline offset for every baseline-aligned entry in that array.

...BUT:
(D) when it comes time to actually reflow the item that lives in the subgrid, we do so using the subgrid's copy of the GridItemInfo struct (the one from (A)), because it seems the subgrid is responsible for reflowing its items. And so our mBaselineOffset values never make it into that struct.

Comment 3

[Daniel Holbert [:dholbert]]

Aha, and this is precisely the issue that we call attention to with the XXXmats code-comment here:

https://searchfox.org/mozilla-central/rev/b580e3f77470b2337bc8ae032b58a85c11e66aba/layout/generic/nsGridContainerFrame.cpp#3731-3747

// We run the Track Sizing Algorithm in non-subgridded axes, and in some
// cases in a subgridded axis when our parent track sizes aren't resolved yet.
if (MOZ_LIKELY(!isSubgriddedAxis) || fallbackTrackSizing.isSome()) {
  const size_t origGridItemCount = mGridItems.Length();
  if (mFrame->HasSubgridItems(aAxis)) {
    CollectSubgridItemsForAxis(aAxis, mGridItems);
  }
  tracks.CalculateSizes(
      *this, mGridItems,
      fallbackTrackSizing ? *fallbackTrackSizing : sizingFunctions,
      aContentBoxSize,
      aAxis == eLogicalAxisInline ? &GridArea::mCols : &GridArea::mRows,
      aConstraint);
  // XXXmats we're losing the baseline state of subgrid descendants that
  // CollectSubgridItemsForAxis added here.  We need to propagate that
  // state into the subgrid's Reflow somehow...
  mGridItems.TruncateLength(origGridItemCount);

Comment 4

[Daniel Holbert [:dholbert]]

I suspect our test-failures in the other subgrid-baseline-00* WPTs are all this same root cause and may be fixed by this bug -- in particular, these ones:
bug 1841299: Firefox fails WPT css/css-grid/subgrid/subgrid-baseline-005.html
bug 1841300: Firefox fails WPT css/css-grid/subgrid/subgrid-baseline-006.html
bug 1841301: Firefox fails WPT css/css-grid/subgrid/subgrid-baseline-007.html
bug 1841303: Firefox fails WPT css/css-grid/subgrid/subgrid-baseline-008.html
bug 1841304: Firefox fails WPT css/css-grid/subgrid/subgrid-baseline-009.html

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1833482 part 0: Rename CollectSubgridForAxis to CollectSubgridItemsForAxisRecursive. r?emilio,TYLin,#layout

This patch doesn't change behavior; it's just a rename.

We have an "entry-point" function CollectSubgridItemsForAxis that the outer
grid calls, and most of its work is done by a recursive helper function.

Before this patch, the recursive helper function has nearly the same name but
with the word "Items" removed, which doesn't make any sense, since it's still
collecting items from subgrids.

Let's just rename the recursive helper to match its entry-point, with a
"Helper" suffix, and document that it's a recursive helper. This makes the
function name a little longer, but not too much longer.

(I'm bothering to do this because the next patch in this series will add a
similar pair of functions, and I want the new functions to use the same naming
scheme as these existing functions.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1833482 part 1: Propagate baseline offset metrics from temporary GridItemInfo structs in parent grid to the canonical GridItemInfo structs in the subgrids. r?emilio,TYLin,#layout

Before this patch, when we baseline-aligned items in a grid, we were careful to
include items from subgrids in that alignment geometry. However, we ended up
dropping the baseline alignment metrics for those subgrid items on the floor,
per the code-comment and TruncateLength call here:
https://searchfox.org/mozilla-central/rev/b580e3f77470b2337bc8ae032b58a85c11e66aba/layout/generic/nsGridContainerFrame.cpp#3744-3747

This patch makes us propagate subgrid items' baseline-alignment metrics to
their actual GridItemInfo structs (which canonically live in the Subgrid()
frame-property on their direct subgrid parent).

Now that those baseline-alignment metrics are having an effect, we can also see
that the (previously-ignored) metrics are wrong in some cases -- they don't
properly account for various forms of padding on subgrid containers of
baseline-aligned grid items. That's tracked in bug 1871719, and that's the
reason this patch has several WPT subtests, which were previously passing.
(They were passing just by chance, since our previous trivial-fallback behavior
happened to place some items at the correct start/end-aligned location, and our
new baseline-aligned location happens to be incorrect due to bug 1871719.)

Since this patch is regressing those cases: out of an abundance of caution, I'm
putting the new behavior behind an about:config preference
"layout.css.grid-subgrid-baselines.enabled" (defaulting to true in Nightly,
false in other builds), so that this patch will have no behavioral effect for
users of release builds. We can remove the about:config pref once bug 1871719
is fixed.

Depends on D197232

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1833482 part 2: Add modified copies of some subgrid-baseline-* WPTs with various forms of offsets removed from subgrids (which is sufficient for us to pass them). r?emilio,TYLin,#layout

As noted in the previous patch in this series, our baseline alignment code for
grid items in subgrids is partially working, but doesn't yet account for
various forms of padding on the subgrid itself.

So, to get a bit of code coverage and regression-proofing for the functionality
that we've got working so far, I'm temporarily adding some modified copies of
existing WPT tests, with the not-quite-working-properly-yet offsets removed.

Our rendering of these modified tests (as of the Gecko fix in this patch stack)
agrees with Chromium, which is an indication that we're laying these out
modified files properly. I've manually set the various 'data-offset-*' values
in these modified tests to simply match the observed values that we and
Chromium agree on when running these tests.

Depends on D197233

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75f5d477db6f
part 0: Rename CollectSubgridForAxis to CollectSubgridItemsForAxisRecursive. r=emilio,layout-reviewers
https://hg.mozilla.org/integration/autoland/rev/f3c7b9040d68
part 1: Propagate baseline offset metrics from temporary GridItemInfo structs in parent grid to the canonical GridItemInfo structs in the subgrids. r=emilio,layout-reviewers
https://hg.mozilla.org/integration/autoland/rev/34c9705d9009
part 2: Add modified copies of some subgrid-baseline-* WPTs with various forms of offsets removed from subgrids (which is sufficient for us to pass them). r=emilio,layout-reviewers

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/75f5d477db6f
https://hg.mozilla.org/mozilla-central/rev/f3c7b9040d68
https://hg.mozilla.org/mozilla-central/rev/34c9705d9009