1834663 - WPT failures in css-flexbox/align-items-baseline-column-horz.html and align-items-baseline-row-vert.html

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, defect)

Description

[Daniel Holbert [:dholbert]]

We fail these interop-2023 relevant WPT tests:
https://wpt.fyi/results/css/css-flexbox/align-items-baseline-column-horz.html
https://wpt.fyi/results/css/css-flexbox/align-items-baseline-row-vert.html

It looks like we're aligning the flex items to the wrong side of the flex container, or something to that effect.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Hi Daniel,

I'm trying to understand http://wpt.live/css/css-flexbox/align-items-baseline-column-horz.html why the first two flex item with writing-mode: vertical-rl; are aligned to the cross-end side in a column-oriented flex container.

The flexbox spec https://drafts.csswg.org/css-flexbox-1/#valdef-align-items-baseline seems to indicate the item should be aligned to the cross-start edge of the container, regardless of the item's own writing-mode.

However, the alignment spec 9.3.3 https://drafts.csswg.org/css-align-3/#align-by-baseline says

Position the aligned baseline-sharing group within the alignment container according to its fallback alignment.

and 4.2 Baseline Alignment https://drafts.csswg.org/css-align-3/#valdef-justify-self-first-baseline says

The fallback alignment for first baseline is safe self-start (for self-alignment)

So the alignment spec seems to indicate that a flex item with align-self: baseline needs to be aligned to its self-start side, which is the cross-end side in the testcase.

In short, it looks like we have three things that can change which side to align: align-self: first/last baseline, flex-wrap: wrap-reverse (reverse the cross-axis), and the flex-item's own writing-mode.

Could you check if my understand is correct?

Comment 2

[Daniel Holbert [:dholbert]]

Hand-waving a bit... I think roughly what's going on here is:

• The flex container has two different sets of baseline-aligned flex items:
  (1) the items that are doing alignment using an exported baseline taken from their line that's closest to the flex container's start side
  (2) the items that are doing alignment using an exported baseline taken from their line that's closest to the flex container's end side
• Group (1) gets (collectively) aligned to the flex container's start side, and group (2) gets collectively aligned to the flex container's end side.
• In most cases, the flex items with align-self: baseline (and first baseline) are in group (1) (the start group), and group.
• However, in this case, even though all of the flex items have align-self: baseline (i.e. first baseline): nonetheless, for some of those flex items, that first baseline corresponds to their line that's closest to the flex item's end side. So they belong in group (2), which gets end aligned.

Does that make sense?

(I haven't yet grounded this in the actual spec text -- need to do that before going too far here -- but it makes some sense based on what Chrome seems to be doing and what the test seems to expect.)

Comment 3

[Daniel Holbert [:dholbert]]

Side note, this test was added by IanK in https://github.com/web-platform-tests/wpt/commit/e19fd3d174d2adbb228daf7718f5e57d8ae32ce6 , and the tests reference https://drafts.csswg.org/css-align-3/#baseline-export as their spec link, though I'm still not immediately seeing the spec text that would suggest that some flex items with the same align-self value would go into different baseline alignment groups.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (with more lines, making the baseline alignment of opposite-WM flex items more problematic)

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

• Group (1) gets (collectively) aligned to the flex container's start side, and group (2) gets collectively aligned to the flex container's end side.
• In most cases, the flex items with align-self: baseline (and first baseline) are in group (1) (the start group), and group.
• However, in this case, even though all of the flex items have align-self: baseline (i.e. first baseline): nonetheless, for some of those flex items, that first baseline corresponds to their line that's closest to the flex item's end side. So they belong in group (2), which gets end aligned.

Here's a testcase to make it a little bit clearer why this is a useful thing to do. When there are flex items that have many lines of text, and have their first baseline on opposite sides, our current behavior can end up aligning the left edge of one with the right edge of the other (offset a little bit) making them overflow the flex container and/or just be awkwardly tall/offset.

(I recall there being a CSSWG discussion about related stuff sometime over the last year; I think https://github.com/w3c/csswg-drafts/issues/7641 and https://github.com/w3c/csswg-drafts/issues/7776 are what I'm remembering, though I'm not sure if that issue precisely covered this. They seem to be more focused on how to determine the flex container's own baseline.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1 in Chrome (left) vs Firefox (right)

Here's a screenshot of testcase 1 in Chrome vs. Firefox.

It looks like we and Chrome are agreeing on using the central baseline (due to the vertical writing mode), and we agree that we should be using the central baseline of line 1 (due to first baseline).

But Chrome is splitting up the items into two different baseline-aligned groups based on which physical side that first baseline corresponds to, which means they don't overflow the flex container, whereas we do.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (with some italicized `last baseline` items)

This is an extension of testcase 1 where I added some align-self: last baseline items.

Chrome puts these into baseline sharing groups based on the physical side that the last baseline is on (lining up the leftmost lines of the upper teal & pink items, vs. the rightmost lines of the lower teal and pink items).

Firefox puts them into baseline sharing groups based simply on whether they're using first / last (which means we align the upper teal area with the lower pink area (the two non-italic first baseline sections), and we align the upper pink area to the lower teal area (the two italic last baseline sections).

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (no text / content inside flex items, so they have to synthesize a baseline)

This testcase is like testcase 2 but now without text, so the flex items have to synthesize a baseline. Here I've used a border to indicate which items have align-self: last baseline alignment.

(Chrome and Firefox still behave the same way as described in comment 6; Chrome aligns the upper two items together, and aligns the lower two items together. Firefox aligns based on the literal align-self values, i.e. aligning the borderless items together and aligning the bordered items together.)

Comment 8

[Daniel Holbert [:dholbert]]

I think we need to add a new getter to FlexItem, something like UsesSwappedBaselineSharingGroups()[1].

And then in FlexLine::ComputeCrossSizeAndBaseline, https://searchfox.org/mozilla-central/rev/986024d59bff59819a3ed2f7c1d0f5254cdc3f3d/layout/generic/nsFlexContainerFrame.cpp#3647 , we would want to do
s/if (useFirst) {/if (useFirst) && !item.UsesSwappedBaselineSharingGroups() {/

And we'd need some sort of similar change here:
https://searchfox.org/mozilla-central/rev/986024d59bff59819a3ed2f7c1d0f5254cdc3f3d/layout/generic/nsFlexContainerFrame.cpp#3818-3832

(Basically: wherever we reason about "is this item using first-baseline or last-baseline alignment", we need to now separate (1) which baseline are we asking the item for, and (2) which baseline-sharing group do we contribute to in the flex line.)

[1] I think this would tests whether the flex item's block axis is anti-parallel to the flex container's cross axis (I think that's the relevant thing to test? Or maybe we should be comparing against the flex container's [block,inline] axis, before flex-flow:row-reverse has taken effect? It'd be good to test with flex-flow:row-reverse to be sure we're interoperable.

Comment 9

[Daniel Holbert [:dholbert]]

TYLin: if you're wanting to take this bug, let me know if comment 8 and the analysis of the attached testcases makes sense. (Alternately, I can take this, if that's easier / if you've got other bugs on your plate at the moment.)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: testcase 4: same as 2 but with `flex-flow:wrap-reverse`

(I'll drop a few more testcases here with various forms of reversal on the cross axis, for convenience when testing tentative patches. I'm not sure if WPTs exist for these; we'll perhaps find out when running patches through Try & seeing if any more than just these two tests start passing. if not, we should consider adding such tests.)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: testcase 5: same as 2 but with `writing-mode:vertical-rl` instead of `lr` on the flex container

(Chrome renders this testcase exactly the same as testcase 2, FWIW, and I think that's correct.)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 6: same as 2 but now with `flex-direction:column` and default WM on flex container

(Chrome renders this testcase exactly the same as testcase 2, too, and I think that's correct.)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: testcase 7: same as 6 but with `direction: rtl`

This is the same as the above one but with direction:rtl. Chrome renders this like testcase 2.

The upshot here is that it seems like Chrome is placing flex items into their flex line's baseline-sharing-groups based on matching up the underlying physical sides. If the flex item's first line is on the left edge, then Chrome has it contribute to whichever baseline sharing group is on the flex line's left edge. (And this makes sense, from a perspective of using the available space most effectively & avoiding awkward overflow.) This is why testcases 2, 5, 6, and 7 all render the same in Chrome.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: testcase 8: same as 6 but with `wrap-reverse`

Chrome renders this testcase the same way they render testcase 4 (with the upper flex items being center-baseline-aligned using the leftmost line, but snapped to the flex container's right side; and the lower flex items being center-baseline-aligned using the rightmost line, but snapped to the flex container's left side).

i.e. wrap-reverse (reversing the cross axis) flips which physical side of the flex line that the baseline sharing groups (as a whole) end up being aligned to.

I think this probably makes sense? extrapolating from https://drafts.csswg.org/css-flexbox-1/#valdef-align-items-baseline which says that (first-)baseline aligned flex items are placed "flush against the cross-start edge of the line."

Comment 15

[Daniel Holbert [:dholbert]]

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

TYLin: if you're wanting to take this bug, let me know if comment 8 and the analysis of the attached testcases makes sense. (Alternately, I can take this, if that's easier / if you've got other bugs on your plate at the moment.)

Actually: hopefully it won't be too troublesome if I just take this. I suspect it'll be most efficient to just write the patch while I've got a mental model for how this should work work.

Side note: one interesting wrinkle here is that we disagree with Chrome on the rendering for this WPT reference case (which uses grid rather than flexbox):
https://wpt.live/css/css-flexbox/align-items-baseline-row-vert-ref.html

Chrome and WebKit both snap the cyan square to the right side; Firefox snaps it to the left side. I'll need to fix that (in our grid impl) before we pass WPT align-items-baseline-row-vert.html (if we're interoperable on the testcase but non-interoperable on the reference case, then we still fail of course.)

Comment 16

[Daniel Holbert [:dholbert]]

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

i.e. wrap-reverse (reversing the cross axis) flips which physical side of the flex line that the baseline sharing groups (as a whole) end up being aligned to.

I dug into this a bit; I don't think css-align-3 makes this^ clear right now, but it does seem to be interoperable (Chrome/Firefox/Epiphany-webkit all do this for horizontal-tb content). So it's likely that there are webcompat depenencies on it, and we need to preserve this invariant in our updated behavior here.

(My first attempt at a patch, in https://treeherder.mozilla.org/jobs?repo=try&revision=d9abb9b0e510213bb99eb0e2846e5d6937aad0da , happened to change this, as a side effect of the approach. i.e. with my current patch, I think we render the wrap-reverse testcases (4 and 8) the same as the other testcases. But I don't think that's workable, per the previous paragraph; hence I'll fix that before posting a patch for review.)

Comment 17

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 5:

But Chrome is splitting up the items into two different baseline-aligned groups based on which physical side that first baseline corresponds to, which means they don't overflow the flex container, whereas we do.

From this perspective to prevent the flex item from overflowing the container, I feel it makes sense.

Re comment 15:

Actually: hopefully it won't be too troublesome if I just take this. I suspect it'll be most efficient to just write the patch while I've got a mental model for how this should work work.

Of course if you've had idea to fix this bug. Thank you for taking this!

Comment 18

[Ian Kilpatrick [:iank]]

Hey folks - feel free to shoot me any questions. The baseline group part of the spec is here: https://drafts.csswg.org/css-align-3/#compatible-baseline-alignment-preferences

Internally we refer to these as the "major" / "minor" group, and logic to decide on the group is: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/ng/ng_baseline_utils.h;l=51?q=DetermineBaselineGroup&ss=chromium

The flex "reverse" cases are quite complex for baseline exporting - but roughly follow if its at the physical "top" use that as the "first" baseline, etc.

Ian

Comment 19

[Daniel Holbert [:dholbert]]

Thanks, Ian! That spec reference is handy (RE "opposite block flow and opposite baseline alignment preference") - that helps ground the intuitively-correct-looking expectations of the tests here.

I've got a patch locally that passes tests, which I'm just cleaning up. It lets me remove the following test-failure-expectation .ini files:

R testing/web-platform/meta/css/css-flexbox/align-items-baseline-column-horz.html.ini
R testing/web-platform/meta/css/css-flexbox/align-items-baseline-column-vert-rl-flexbox-item.html.ini
R testing/web-platform/meta/css/css-flexbox/align-items-baseline-column-vert-rl-grid-item.html.ini
R testing/web-platform/meta/css/css-flexbox/align-items-baseline-column-vert-rl-items.html.ini
R testing/web-platform/meta/css/css-flexbox/align-items-baseline-column-vert-rl-table-item.html.ini
R testing/web-platform/meta/css/css-flexbox/alignment/flex-align-baseline-001.html.ini
R testing/web-platform/meta/css/css-flexbox/alignment/flex-align-baseline-002.html.ini
R testing/web-platform/meta/css/css-flexbox/alignment/flex-align-baseline-003.html.ini
R testing/web-platform/meta/css/css-flexbox/alignment/flex-align-baseline-004.html.ini

...and also this one modulo comment 20-22:

R testing/web-platform/meta/css/css-flexbox/align-items-baseline-row-vert.html.ini

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Ian Kilpatrick [:iank] from comment #18)

Hey folks - feel free to shoot me any questions.

Actually I do have one question for you. Per comment 15, we still fail one interop2023-relevant test here due to misrendering its reference case (which uses grid alignment). Our misrendering seems to be triggered by a writing-mode declaration on an empty box in that reference case -- specifically the last line in this CSS rule, for the empty cyan square:
https://github.com/web-platform-tests/wpt/blob/433226a28e/css/css-flexbox/align-items-baseline-row-vert.html#L30

#target > :nth-child(3) {
  background: cyan;
  width: 40px;
  height: 40px;
  writing-mode: horizontal-tb;
}

I intend to investigate that behavior difference in a separate bug, but in the meantime, would it be all right with you [as the test author] for me to remove that one writing-mode: horizontal-tb; from the reference case here, since it has zero impact on the rendering in WebKit and Chrome, and it makes the test a bit shorter, and it makes the reference case serve better as an actual reference case for this test in Firefox?

Comment 21

[Ian Kilpatrick [:iank]]

Yup that sounds fine. Unsure why I used grid there. There are is analogous set of grid tests (in css-grid/alignment i think) testing all the complex behaviours.

Ian

Comment 22

[Daniel Holbert [:dholbert]]

Thanks! I spun off bug 1839927 with a reduced testcase, for the grid issue that the reference case is tripping over.

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: Bug 1834663 part 1: Remove an unnecessary declaration from a flexbox WPT reference case, so that it renders properly in Firefox. r?TYLin

The line that's being removed here was making the reference case render in an
unintended way in Firefox (i.e. it wasn't producing the intended reference
rendering), due to this bug in our grid implementation:
https://bugzilla.mozilla.org/show_bug.cgi?id=1839927

This line wasn't particularly relevant and isn't supposed to impact the
rendering of this reference case. It has no effect in Chromium or WebKit,
so let's remove it to simplify the file slightly. Test-author IanK says this
seems fine, too, per: https://bugzilla.mozilla.org/show_bug.cgi?id=1834663#c21

With this reference-case change, the corresponding WPT test
(align-items-baseline-row-vert.html) will start passing in Firefox, as of a
later patch in this series.

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: Bug 1834663 part 2: Give the FlexItem class a method to return the BaselineSharingGroup that it participates in. r?TYLin

This patch doesn't affect behavior; it's just a refactoring to prepare for the
next patch in this series.

(As of this patch, the BaselineSharingGroup is always the same as the
'align-self' baseline selection, but the next patch will change that.)

Depends on D181822

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: Bug 1834663 part 3: Make FlexItems join the opposite BaselineSharingGroup on their FlexLine, if their block-flow direction is opposite to the flex container's corresponding LogicalAxis. r?TYLin

The idea behind this change is for flex items with swapped block-flow direction
to end up in swapped baseline-sharing groups on the flex container's FlexLine.

The outcome of this is that all of the baseline-aligned flex items that are
using a baseline on their left edge will be aligned together (e.g. in the
"first" group), and all of the flex items with a baseline on their right edge
will be aligned together (e.g. in the "last" group), even if they may have
varying block-flow directions (vertical-lr vs. rl) and varying align-self
values.

This is rooted in the idea of baseline alignment preferences being "compatible"
as defined here:
https://drafts.csswg.org/css-align-3/#compatible-baseline-alignment-preferences

This makes us start passing a bunch of WPT tests; this patch removes the
corresponding .ini files for those tests that are now passing.

Depends on D181823

Comment 26

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/jobs?repo=try&revision=3886534156344774e8b51e3c816ec236d53f03f3

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69d4d1deb1a3
part 1: Remove an unnecessary declaration from a flexbox WPT reference case, so that it renders properly in Firefox. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/4a201b923227
part 2: Give the FlexItem class a method to return the BaselineSharingGroup that it participates in. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/006c43fdfa96
part 3: Make FlexItems join the opposite BaselineSharingGroup on their FlexLine, if their block-flow direction is opposite to the flex container's corresponding LogicalAxis. r=TYLin

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/40728 for changes under testing/web-platform/tests

Comment 29

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/69d4d1deb1a3
https://hg.mozilla.org/mozilla-central/rev/4a201b923227
https://hg.mozilla.org/mozilla-central/rev/006c43fdfa96

Comment 30

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot