1742042 - Intrinsically-sized flex container has the wrong intrinsic ISize, if it has a definite BSize that stretches a child with an aspect ratio

Status: VERIFIED FIXED

(Core :: Layout: Flexbox, defect)

Description

[Daniel Holbert [:dholbert]]

STR:

1. Load attached testcase.

EXPECTED RESULTS:
Orange should be contained in blue border.

ACTUAL RESULTS:
Orange overflows blue border.

Firefox Nightly (as of today) and Chrome both give versions of EXPECTED RESULTS, though they disagree -- Firefox renders the blue box as being 46px wide (40px-wide content area), and Chrome renders it as 106px wide (100px-wide content area).

Firefox Nightly with bug 1735589's fixes (which I suspect are landing soon) produces ACTUAL RESULTS -- the orange area is 100px wide, but the blue box is 46px wide (40px-wide content area).

This differing size reflects a difference between how the flex container computes its own intrinsic size based on its children, vs. how its flex items actually compute their own sizes (and automatic minimum sizes) during layout. The latter phase is changing in bug 1735589. We might need to change the former as well to avoid triggering unexpected overflow for cases like this, because it's a bit odd for a max-content-sized element (like the flex container in this example) to nonetheless have its content spill out of it.

Comment 1

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

Thanks for filing this bug. Could you attach the testcase?

I can take a look at this bug and coordinate the landing of bug 1735589. In other words, if this is hard to fix in a short time frame, we probably shouldn't land bug 1735589 for now.

Comment 2

[Daniel Holbert [:dholbert]]

oops, sorry for forgetting the testcase! yup, I'll attach shortly (I had it in /tmp and have since rebooted, so I'll need to regenerate it)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase in various browsers

Here's a screenshot of the testcase in:

• Nightly 96.0a1 (2021-11-19)
• Local build with bug 1735589's patches
• Chrome

Comment 5

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

Ah, yet another bug that the stretched cross-size of the flex items affects the flex container's intrinsic size. I think this is similar to bug 1740658.

Comment 6

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

FWIW, Safari Technology Preview 17.0 (Release 172) behaves like Google Chrome for testcase 1.

Comment 7

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

Attachment: Bug 1742042 Part 1 - Remove unused FlexItem::SetIsStretched(). r?dholbert,#layout

We only ever set mIsStretched to true in
FlexItem::ResolveStretchedCrossSize() [1]. However, we cannot use
SetIsStretched() there because FlexItem::ResolveStretchedCrossSize() is
called within GenerateFlexItemForChild(), where the main size has not yet been
resolved. This triggers the assertion in SetIsStretched().

[1] https://searchfox.org/mozilla-central/rev/14a66905574728bc9eb60e927d17d98721167dcf/layout/generic/nsFlexContainerFrame.cpp#3891

Comment 8

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

Attachment: Bug 1742042 Part 2 - Sort headers in nsLayoutUtils.h to match the coding style. r?dholbert,#layout

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Comment 9

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

Attachment: Bug 1742042 Part 3 - Move StyleSizeOverrides from ReflowInput.h to LayoutStructs.h. r?dholbert,#layout

We are going to use StyleSizeOverrides in nsLayoutUtils.h in Part 4 that
requires a full definition of StyleSizeOverrides. but we don't want to include
the entire ReflowInput.h in nsLayoutUtils.h that can add unnecessary header
dependency.

We could also move it into LayoutConstants.h that has been included by
nsLayoutUtils.h, but the header is a home for layout constants. Therefore, I
create a new header for layout helper classes and structs.

Comment 10

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

Attachment: Bug 1742042 Part 4 - Apply flex item's stretched cross-size when computing flex container's intrinsic inline size. r?dholbert,#layout

For flex items with an aspect-ratio, the stretched cross-sizes can be
transferred to the main axis, affecting their intrinsic inline-size contribution
to the flex container's intrinsic inline-size.

Add aspect-ratio-intrinsic-size-008.html (adapted from 003.html) to test a
flex item with border and padding since the existing tests do not cover this.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea37bd55d9cf
Part 1 - Remove unused FlexItem::SetIsStretched(). r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/35769cab3573
Part 2 - Sort headers in nsLayoutUtils.h to match the coding style. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/8475273664b3
Part 3 - Move StyleSizeOverrides from ReflowInput.h to LayoutStructs.h. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/426eef56bc22
Part 4 - Apply flex item's stretched cross-size when computing flex container's intrinsic inline size. r=dholbert

Comment 12

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

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

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/ea37bd55d9cf
https://hg.mozilla.org/mozilla-central/rev/35769cab3573
https://hg.mozilla.org/mozilla-central/rev/8475273664b3
https://hg.mozilla.org/mozilla-central/rev/426eef56bc22

Comment 14

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

Upstream PR merged by moz-wptsync-bot

Comment 17

[Bianca Hidecuti, Desktop Test Engineering [:bhidecuti]]

I was able to reproduce the issue with an affected Firefox 130.0b1 build, using WIndows 11 and the test case from Comment 3.
Verified as fixed using Firefox 132.0b2, on macOS 14.6, Windows 11 and Ubuntu 22.04.