1784135 - abspos flex children in multi-line flex container are aligned incorrectly with `align-content:stretch`

Status: RESOLVED INVALID

(Core :: Layout: Flexbox, defect)

Description

[Daniel Holbert [:dholbert]]

Here:

      // Single-line, or multi-line but the (one) line stretches to fill
      // container. Respect align-self.

https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/layout/generic/nsFlexContainerFrame.cpp#1295-1297
...we assume that abspos flex items in a multi-line flex container (with one "virtual line", because we're just aligning an abspos flex item on its own) should behave as if that line were stretched to fill the container

The spec does indeed say to do that for single-line flex containers, in the first sentence of "Calculate the cross size of each flex line", here:
https://drafts.csswg.org/css-flexbox-1/#algo-cross-line
... but single-line is linkified to a definition "A single-line flex container (i.e. one with flex-wrap: nowrap)"

So, our multi-line but the (one) line stretches to fill container categorization (in my initial code-comment link here) is incorrect.

In practice, this means we end up honoring align-self for alignment of a flex item in a flex-wrap:wrap flex container, when we should in fact defer to align-content (since that aligns the line, which is sized to the flex item, which then leaves no positive or negative space for align-self to do anything with).

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a testcase.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: reference case 1

Comment 3

[Daniel Holbert [:dholbert]]

er, sorry, I over-reduced the testcases. Our rendering of testcase 1 is actually correct, because we honor stretch and fill the flex container.

The case where we're wrong is when the flex container is smaller than the flex item (in which case, for a single-line container, the flex line would nonetheless fill the container exactly per https://drafts.csswg.org/css-flexbox-1/#algo-cross-line -- but for a multi-line container, the flex line should size to fit the flex item which leaves negative free space, and align-content:stretch ends up behaving as flex-start.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: reference case 2a (using `align-content: flex-start`)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: reference case 2b (using in-flow flex items)

Comment 7

[Daniel Holbert [:dholbert]]

First reference case (2a) demonstrates the align-content:flex-start layout that the spec instructs us to fall back to.

The second reference case (2b) demonstrates the "just one flex item, in-flow" analogue that the spec is essentially going for when aligning abspos flex children.

Comment 8

[Daniel Holbert [:dholbert]]

Looks like we fixed this for in-flow flex items (i.e. we got to our current/correct rendering of reference case 2b) in bug 1090031.

(And this happened shortly before we landed this alignment code for out-of-flow flex items in bug 1269046.)

Having said that, it looks like we also have bug 1682641 tracking the fact that we should be ignoring align/justify-content entirely here, maybe. Though I'm not sure if that change is web-compatible at this point.

Comment 9

[Daniel Holbert [:dholbert]]

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

Having said that, it looks like we also have bug 1682641 tracking the fact that we should be ignoring align/justify-content entirely here, maybe. Though I'm not sure if that change is web-compatible at this point.

(That's associated with https://github.com/w3c/csswg-drafts/issues/5843 which is unresolved at the moment. So: it's still worth making a targeted fix to this bug here, and then circle back to bug 1682641 when the csswg-drafts #5843 is resolved.)

Note, this all came across my radar because we have a WPT test https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/testing/web-platform/tests/css/css-flexbox/abspos/flex-abspos-staticpos-align-content-005.html (and neighbors I think?) that improperly expect our current behavior. My attached testcase 2 here is a slightly embiggened version of the relevant part of that testcase.

Comment 10

[Daniel Holbert [:dholbert]]

(and this WPT test issue was raised in https://github.com/web-platform-tests/wpt/issues/35420 )

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: WIP: Bug 1784135: Make "align-content:stretch" fall back to flex-start, for overflowing abspos flex children.

Background: We align abspos flex children as if they were the sole flex item in
their flex container, per https://drafts.csswg.org/css-flexbox-1/#abspos-items

Before this patch, we were incorrectly disregarding "align-content:stretch" for
these flex children and just applying "align-self", under the assumption that
"align-content:stretch" would simply cause the sole "virtual flex line" to
exactly fill the flex container (which means we can just use align-self to align within the flex container).

This assumption is accurate if the flex container is larger than the child; but
if the child is larger than its flex container, then the assumption is not
true. In that case, the "virtual flex line" will simply size to the child and
will be larger than the flex container. This leaves negative free space, which
should trigger the fallback to flex-start as described in
https://drafts.csswg.org/css-flexbox-1/#valdef-align-items-stretch

I'm implementing this by passing a flag into our general
CSSAlignmentForAbsPosChild method to indicate whether we have negative free
space; this lets us handle stretch properly in both cases (falling back to
flex-start if there's no space).

Comment 12

[Daniel Holbert [:dholbert]]

After writing a patch here and seeing what tests broke, I realized that this change may not be web-compatible, per
https://github.com/web-platform-tests/wpt/issues/35420#issuecomment-1211401820

i.e. there's a reasonable chance that the web depends on align-self working for abspos flex children (including if they happen to be in a too-small multi-line flex container with default align-cotnent)... It does work in all browsers right now, even if strictly-speaking it shouldn't, so sites may have grown to depend on that.

Comment 13

[Daniel Holbert [:dholbert]]

So: this is probably not actionable for the time being, and we need to wait on https://github.com/w3c/csswg-drafts/issues/5843 .

Comment 14

[Daniel Holbert [:dholbert]]

The CSSWG resolved to ignore align-content on abspos flex children in https://github.com/w3c/csswg-drafts/issues/7596

...which makes this bug invalid. I'll file a new bug on making that change.

Comment 15

[Daniel Holbert [:dholbert]]

--> Filed bug 1786910 on the new spec behavior (ignoring align-content for positioned children of flex container).