1963013 - [css-grid] Investigate flexbox tests that are failing because they use grid baseline alignment in the references.

Status: RESOLVED WORKSFORME

(Core :: Layout: Grid, task)

Description

[Tiaan Louw]

These tests are failing, because we're updating baseline alignment in grid and the tests use grid baseline to test flexbox baseline alignment:

css/css-flexbox/align-items-baseline-column-horz.html
css/css-flexbox/flexbox_columns.html
css/css-flexbox/gap-007-ltr.html
css/css-flexbox/gap-007-rtl.html
css/css-flexbox/align-items-baseline-column-vert.html
css/css-flexbox/align-items-baseline-row-vert.html
css/css-flexbox/flex-flow-004.html
css/css-flexbox/flex-flow-005.html
css/css-flexbox/flex-flow-006.html
css/css-flexbox/flex-flow-009.html
css/css-flexbox/flex-flow-012.html
css/css-flexbox/gap-007-lr.html
css/css-flexbox/gap-007-rl.html

Do we fix the references to not rely on grid baseline alignment or fix grid baseline alignment?

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Tiaan Louw from comment #0)

These tests are failing,

(I assume you mean they're failing locally for you with your in-progress patches. Right now these tests are passing in current mozilla-central, with one exception that I noticed, noted below.)

css/css-flexbox/align-items-baseline-column-horz.html

This one has been failing for a while and is annotated as-such:
https://wpt.fyi/results/css/css-flexbox/align-items-baseline-column-horz.html
https://searchfox.org/mozilla-central/rev/931de234363586a45ea9ab66d281960ccf1c3597/testing/web-platform/meta/css/css-flexbox/align-items-baseline-column-horz.html.ini

So that one's perhaps not something to worry about in this bug here.

But these other ones are currently passing though, so let's look at them:

css/css-flexbox/gap-007-ltr.html
css/css-flexbox/gap-007-rtl.html
[...]
css/css-flexbox/gap-007-lr.html
css/css-flexbox/gap-007-rl.html

https://wpt.fyi/results/css/css-flexbox?label=master&label=experimental&aligned&q=%20%20css%2Fcss-flexbox%2Fgap-007

css/css-flexbox/flex-flow-004.html
css/css-flexbox/flex-flow-005.html
css/css-flexbox/flex-flow-006.html
css/css-flexbox/flex-flow-009.html
css/css-flexbox/flex-flow-012.html

https://wpt.fyi/results/css/css-flexbox?label=master&label=experimental&aligned&q=%20%20css%2Fcss-flexbox%2Fflex-flow

From spot-checking a few of these, it looks to me like we're passing them legitimately/correctly (i.e. it's not a case where we were mistakenly "passing" due to having a shared bug between grid/flex that made us render both files incorrectly but in the same way).

Do we fix the references to not rely on grid baseline alignment or fix grid baseline alignment?

We shouldn't mess with the references, since it's likely that the mismatch represents a real bug (perhaps in rendering the reference case); and some of these are interop-relevant which adds some overhead/scrutiny to test/reference changes.

Do you have a Try link showing the test failures? if we're changing our rendering of the reference cases, that's probably a real bug and potentially a regression that we'd like to avoid shipping (i.e. fix as part of whatever patch stack is lined up that would otherwise break these tests).

Comment 2

[Tiaan Louw]

After checking, these are the only ones being affected by my patch stack (the bug this one is blocking):

css/css-flexbox/align-items-baseline-column-horz.html
css/css-flexbox/align-items-baseline-column-vert.html
css/css-flexbox/align-items-baseline-row-vert.html

Top one gets fixed, but last 2 gets broken. The others break locally for me (with the stack applied). I'm running a full try just to make sure about the others and possibly any other failures.

Comment 3

[Tiaan Louw]

This is a full try run:

https://treeherder.mozilla.org/jobs?revision=04d8ca77f3fe66f94ce327b4754270c2444679df&repo=try

css/css-flexbox/align-items-baseline-column-horz.html PASS
css/css-flexbox/align-items-baseline-column-vert.html FAIL
css/css-flexbox/align-items-baseline-row-vert.html FAIL

These failures are now reflected in: https://phabricator.services.mozilla.com/D240317

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Tiaan Louw from comment #3)

css/css-flexbox/align-items-baseline-column-horz.html PASS

Hooray!

css/css-flexbox/align-items-baseline-column-vert.html FAIL

This one already fails:
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/css-flexbox/align-items-baseline-column-vert.html.ini

It looks like the previous patch in your queue ( https://phabricator.services.mozilla.com/D216793 ) was removing the failure annotation, and then https://phabricator.services.mozilla.com/D240317 is adding it back. Not sure how/why the test was fixed by D216793, but possibly that temporary "pass" status is just a version of the known breakage that that patch introduces (in e.g. baseline-alignment-affects-intrinsic-size-001.html.ini ) -- maybe that first patch is just breaking the reference case in the same way that the testcase is already broken, making it temporarily "pass"?

Anyway, bottom line, this isn't a new failure, so probably nothing to worry about here.

css/css-flexbox/align-items-baseline-row-vert.html FAIL

These failures are now reflected in: https://phabricator.services.mozilla.com/D240317

This one's interesting and worth looking into... In this one, it looks like we agree with Chromium on the testcase rendering, and (with the patch stack) we disagree on the reference case's rendering: https://wpt.live/css/css-flexbox/align-items-baseline-row-vert-ref.html

Specifically for the cyan box there:

• Chromium, Safari, and current Firefox use the cyan box's central baseline for baseline-alignment with the central baseline of the "line1" and "line2" text above it.
• Firefox-with-the-patch-stack seems to instead be using the cyan box's left edge (i.e. its line-under edge) for baseline alignment with the central baseline of the "line1" and "line2" text above it.
• (Note that there's no orthogonal-writing-mode stuff here - this is just a case where the cyan box is empty and so we're synthesizing a baseline for it, and I think Chrome/Safari/current-Firefox are correct to be synthesizing the central baseline.)

Comment 5

[Daniel Holbert [:dholbert]]

So: tl;dr, I think the only thing that needs investigation/fixing here is that last one from comment 3 ( https://wpt.live/css/css-flexbox/align-items-baseline-row-vert-ref.html ) where it looks like we're regressing a particular case of grid-item baseline synthesis, which probably needs a fix in our grid implementation (whether directly in D240317 or in a followup).

Comment 6

[Daniel Holbert [:dholbert]]

It looks like reason for that regression is that D240317 is moving us from using SynthesizeBOffsetFromBorderBox (which internally handles some cases where we should use a central baseline) to now using frameSize or 0 (which is an edge, never a central baseline).

Probably we need to reconsider/refine that adjustment to restore SynthesizeBOffsetFromBorderBox there, at least in some form.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a testcase that's an extended version of https://wpt.live/css/css-flexbox/align-items-baseline-row-vert-ref.html

Comparing this testcase between Firefox and Chrome:

• Current Firefox release matches Chrome on this except that we put the orange box on the left side whereas Chrome puts it on the right.
• Firefox with your patch stack ( D240317 ) still has that same difference (orange box is on the left side), plus we're now not synthesizing the central baselines anymore, but apparently we should.

Comment 8

[Tiaan Louw]

I created a bug for addressing the central baseline issue here: Bug 1964417.