1743890 - New wpt failures in /css/css-break/flexbox/single-line-column-flex-fragmentation-026.html (due to failure to push tall flex item with `contain:size` into second column)

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, defect, P3)

Description

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

Syncing wpt PR 31800 found new untriaged test failures in CI

Tests Affected

Firefox-only failures

/css/css-break/flexbox/single-line-column-flex-fragmentation-026.html: FAIL

CI Results

Missing results from treeherder
GitHub PR Head

Notes

These updates will be on mozilla-central once bug 1743700 lands.

Note: this bug is for tracking fixing the issues and is not
owned by the wpt sync bot.

This bug is linked to the relevant tests by an annotation in
https://github.com/web-platform-tests/wpt-metadata. These annotations
can be edited using the wpt interop dashboard
https://jgraham.github.io/wptdash/

If this bug is split into multiple bugs, please also update the
annotations, otherwise we are unable to track which wpt issues are
already triaged. Resolving as duplicate or closing this issue should
be cause the bot to automatically update or remove the annotation.

Comment 1

[Daniel Holbert [:dholbert]]

The testcase has contain:size on the flex items here, which seems to prevent us from fragmenting them (or even pushing the tall one to the second column after we discover it doesn't fit in the first one).

Reduced testcase/reference coming up.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: (version of testcase 1 without 'contain:size', for comparison)

Comment 4

[Daniel Holbert [:dholbert]]

Aha, so the containment spec does say:

Size containment boxes are monolithic (See CSS Fragmentation 3 § 4.1 Possible Break Points).
https://drafts.csswg.org/css-contain-1/#size-containment

So it's actually 100% appropriate that we don't fragment in this case. I had thought that was what the testcase was expecting, but I oversimplified it a bit and misunderstood the testcase a bit (in part because Chrome Dev does-actually-fragment even though it shouldn't).

The testcase actually isn't expecting us to fragment, but rather it's intending to expect that we:
(a) lay out the first item in the first column
(b) lay out the second item in the second column (after figuring out that it won't fit in the first column)
(c) lay out an abspos block at the bottom of the first column (top:50px) to fill in the empty space there and give us a solid green square.

We are currently failing at (b) here (specifically when the container is a flex item -- if I change the container to be a block, then we succeed).

Here's the testcase's wpt.fyi page:
https://wpt.fyi/results/css/css-break/flexbox/single-line-column-flex-fragmentation-026.html
...and the wpt.live page (live version of the WPT test):
https://wpt.live/css/css-break/flexbox/single-line-column-flex-fragmentation-026.html

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (using default display:block instead of flex for the container)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: (version of testcase 2 without 'contain:size', for comparison)

Comment 7

[Daniel Holbert [:dholbert]]

I think we render testcase 2 properly here, and we should render testcase 1 the same way. (And if we did, then I think we would pass this WPT test.)

I also think Chrome renders testcase 1 and testcase 2 improperly, in that they fragment the contain:size thing when they should not. So they lay out the WPT test incorrectly as well, but in a way that still ends up matching the reference case, given that all the components are the same color (green) and the area-that's-expected-to-be-covered is still covered.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (to demonstrate the Chrome bug): teal rect should not fragment

(Here's a testcase just for the purposes of illustrating Chrome's bug here. I'm going to link to it from an upcoming Chrome bug report. We render this testcase correctly, with the teal rect un-fragmented since it's monolithic due to having 'contain:size'.)

Comment 9

[Daniel Holbert [:dholbert]]

TYLin, maybe you'd be interested to take a look if you have cycles? If not, I can take this.

Summing up the bug: in the bottom part of testcase 1, we put the tall rect in the first column, but we should put it in the second column.

Comment 10

[Daniel Holbert [:dholbert]]

update: I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1285427 for the above-mentioned Chrome bug, and Alison clued me in that I need to be running Chrome with --enable-blink-features=LayoutNGFlexFragmentation (which enables the relevant-to-this-bug pieces of new fragmentation code).

With that, Chrome matches my expectations on testcase 1, 2, and 3 here (and serves as a reference for the correct rendering of testcase 1).

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: testcase 4 (flex container has 'justify-content:space-around')

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

TYLin, maybe you'd be interested to take a look if you have cycles? If not, I can take this.

Summing up the bug: in the bottom part of testcase 1, we put the tall rect in the first column, but we should put it in the second column.

Pushing the tall item to the second column makes sense when justify-content: flex-start. What if the flex container has justfy-content: space-around like Testcase 4? In the spirit of https://github.com/w3c/csswg-drafts/issues/6812, we currently align the items as if the flex container is not fragmented, and then place the items to their corresponding positions when fragmenting the container. If we push the tall item to the second column, we disrupt the global alignment, and have to push the subsequent items to later columns to avoid overlapping with the previous push item.

Daniel, any thoughts on the correct rendering of Testcase 4?

Comment 13

[Daniel Holbert [:dholbert]]

From a dataloss perspective, my instinct is that we should generally push monolithic flex items to the start of the next flex-container-fragment, when they run off the bottom of the current fragment (if they aren't already at the top of the current fragment).

I think this is what we do in block layout, and it's a pretty important principle to avoid dataloss in printed output (though I suppose it's somewhat-less-important these days now that we have our fragmentation-fallback codepath which makes monolithic things sort-of fragmentable).

So in testcase 4, this principle would mean that we should push the flex item to the next column, effectively stealing some extra packing space for it.

I think this does raise an interesting question for how to handle that packing-space-theft. If we've got a fragmented fixed-height flex container, and we think we've got 100px (or whatever) of packing space and we distribute it globally along the lines of https://github.com/w3c/csswg-drafts/issues/6812 , and then we find that we have to push a monolithic flex item and "steal" some extra packing space in an early fragment, then we might inadvertently distribute more packing space in later fragments (from our early "global" distribution) than we actually end up having left over, after accounting for the fact that we've had to push flex items.

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1743890 Part 1 - Add more log to flex container. r?dholbert

We always perform final reflow for flex items under constrained
bsize (NeedsFinalReflow() returns true in this case). Add an assertion to ensure
we don't go into MoveFlexItemToFinalPosition() path.

MoveFlexItemToFinalPosition() already has a log printing flex item's position.
This patch adds a log in ReflowFlexItem() to print flex item's position, too.

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1743890 Part 2 - Push monolithic flex item exceeding available block-size to next-in-flow. r?dholbert

This patch deals with two things:

1. Push tall monolithic flex items to next-in-flow, and adjust their positions.
2. Stretch flex container's block-size if it is unconstrained.

This patch doesn't fix:

1. Item shifts in different lines in a multi-line column-oriented container.
2. Flex container block-size stretch due to flex item's block-size grow in
   fragmentation.

If a flex item has break-before:avoid, we don't want to push it to the
next-in-flow (in the computaion of shouldPushItem). Otherwise, we'll fail
web-platform/tests/css/css-break/flexbox/multi-line-column-flex-fragmentation-034.html

Depends on D165191

Comment 16

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1743890 Part 3 - Add more web-platform tests. r?dholbert

• single-line-column-flex-fragmentation-061.html is adapted from
  single-line-column-flex-fragmentation-059.html, with flex-direction value
  changed from column to column-reverse.

• multi-line-row-flex-fragmentation-066.html is adapted from
  multi-line-row-flex-fragmentation-063.html, with flex-wrap value
  changed from wrap to wrap-reverse.

Depends on D165192

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: testcase 5

Here's a testcase where I think Chrome's implementation is a bit better than ours-with-the-current patch.

We end up with flex item 7 stomping on top of flex item 3 (which was tall & was shifted downwards).

Chrome seems to account for this by pushing the second flex line downwards by the amount that item 3 was shifted away from its idealized position.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: testcase 5

(Sorry, here's a slightly clearer version where I've changed the border on item #3 to make the overlap more obvious.)

Comment 19

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1743890 Part 4 - Rename mSumOfChildrenBSize to mCumulativeContentBoxBSize. r?dholbert

Depends on D167978

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bebf28f32a7
Part 1 - Add more log to flex container. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/bc44c46861dd
Part 2 - Push monolithic flex item exceeding available block-size to next-in-flow. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/8caafe7e4b9a
Part 3 - Add more web-platform tests. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6064a73fc99f
Part 4 - Rename mSumOfChildrenBSize to mCumulativeContentBoxBSize. r=dholbert

Comment 21

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

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

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: testcase 6

FWIW here's a testcase that I had initially attached on phabricator (I've made a few small edits/cleanups before attaching here).

This is illustrating a case where a flex container has plenty of space for its items (and plenty of space between them, via justify-content: space-between), but nonetheless one item overflows (in Chrome and in Firefox-with-this-bug's-patches), since we're being careful to preserve at-least-as-much packing space as our theoretical/unfragmented layout would require.

More notes here:
https://phabricator.services.mozilla.com/D165192#5733910

In this specific case it kinda feels like we should let the packing space between item 2 and item 3 be reduced (since we had to allocate extra packing space between item 1 and 2), so that item 3 can be placed at the bottom of the 2nd fragment and not have to overflow.

Having said that, I'm also not sure there's a good way to do that robustly; this isn't something we could easily do for most of the other alignment values (e.g. justify-content:end in particular) without some substantial increase in complexity. So maybe it's fine that we're just consistently preserving the packing-space-plan from the theoretical unfragmented layout for now.

Comment 23

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3bebf28f32a7
https://hg.mozilla.org/mozilla-central/rev/bc44c46861dd
https://hg.mozilla.org/mozilla-central/rev/8caafe7e4b9a
https://hg.mozilla.org/mozilla-central/rev/6064a73fc99f

Comment 24

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Daniel, thanks for attaching testcase 6.

Having said that, I'm also not sure there's a good way to do that robustly; this isn't something we could easily do for most of the other alignment values (e.g. justify-content:end in particular) without some substantial increase in complexity. So maybe it's fine that we're just consistently preserving the packing-space-plan from the theoretical unfragmented layout for now.

I agree with this observation. When we do fragment flex layout, we don't have any knowledge of the packing space anymore; we only have the (unfragmented) flex item positions. If we were to move item 3 towards the block-start to the bottom of the 2nd column, as if the packing space is shrinking between item 2 and item 3, we'll have to check whether item 3 is overlapping with other items before it. This is easy to track in single column flex containers, but can be complex in row column containers.

Comment 25

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

Upstream PR merged by moz-wptsync-bot