1611303 - percent height in flex item doesn't resolve, in scenario with nested auto-height flex containers (which are ultimately definite-height)

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Load the attached testcase.

EXPECTED RESULTS:
Lime rect, with no orange visible (all covered up).

ACTUAL RESULTS:
Orange rect (the lime element is 0px tall).

This was reported to us by Chrome folks in https://bugs.chromium.org/p/chromium/issues/detail?id=1043071#c5

Some Chrome versions give expected results, some give actual results, as noted there (but they've changed to ultimately produce expected results in canary 81, I think).

Analysis of why the expected results are correct:
Here's my analysis:
(1) Looking at the outermost element, .tabs: it has an indefinite height (i.e. an indefinite cross-size).

(2) Looking at its child, .tab: it's a flex item in a horizontal flex container, and it eventually is considered to have a definite height (cross size) due to rule #3 of https://drafts.csswg.org/css-flexbox/#definite-sizes (which applies to that element as a flex item). So yes, .tab has a definite height for its final layout.

(3) Looking at its child, .content: it's a flex item in a vertical flex container, and its container is eventually considered definite as noted above, so rule #2 of https://drafts.csswg.org/css-flexbox/#definite-sizes does apply and the flex item is eventually considered definite.

(4) So the percent size in .chart should ultimately be resolvable.

Comment 1

[Daniel Holbert [:dholbert]]

(This is kind of followup work to bug 1092007, I think, so adding a relationship to that bug.)

Also noteworthy: when I inspect the testcase in devtools, I sometimes see EXPECTED RESULTS (i.e. the lime div sometimes jumps to have a nonzero height and fully cover up the orange area). So I think there might be a little bit of nondeterminism here (perhaps getting tickled by the data gathered by our flexbox inspector devtools, which can trigger a special synchronous reflow when devtools open). I'm guessing that this is due to the presence/absence of a cached height on some flex item at some point of (re)layout.

Comment 2

[Daniel Holbert [:dholbert]]

I think this is a case where we need to activate this sort of code, and treat our "final reflow" as a BSize change for the purposes of relative-height descendants:
https://searchfox.org/mozilla-central/rev/d48b18fa27315206bf68d4dac82bc4e27a3f138e/layout/generic/nsFlexContainerFrame.cpp#5108-5111,5119-5121

Currently, that^^ linked searchfox code has logic to handle the following scenario:
(1) [Enter reflow for Flex Container A]
(2) Flex container does "measuring reflow" of flex item, with unconstrained/indefinite BSize. (So: percent-height descendants get laid out with percentages treated as auto.)
(3) Flex item BSize is resolved and is now considered definite
(4) Flex container does "final reflow" of flex item, with known & "definite" BSize, with this reflow treated as a block-axis resize so that we'll be sure to reflow percent-height descendants inside the flex item, even if the height didn't change & the descendants don't think they're dirty. (So: percent-height descendants will be forced to re-lay-out out with percentages resolved appropriately.)
(5) [Exit reflow for Flex Container A]

However, the logic doesn't handle scenarios where step (2) happens before step (1) -- i.e. where the "measuring reflow" happened in an earlier call into nsFlexContainerFrame::Reflow. This is possible if we're several layers deep in nested flex containers. In that scenario, the relevant nsFlexContainerFrame::Reflow call ("Step 1" above) will find a cached already-computed flex-item measurement, and it'll jump straight to the "final reflow" for this flex item, without needing to perform any measuring reflow this time (skipping Step 2 above). So aItem.HadMeasuringReflow() will be false and we won't get to the childReflowInput.SetBResize(true) call, even though we need to do so, because this is potentially the first time we're laying out this child with a definite height, which makes it effectively a block-axis resize from the perspective of percent-height descendants.

Comment 3

[Daniel Holbert [:dholbert]]

wpt.fyi has one of the "compat2021" tests (percentage-heights-010.html) annotated as failing due to this bug:
https://wpt.fyi/results/css/css-flexbox/percentage-heights-010.html?label=master&label=experimental

--> Marking this as blocking the compat2021 metabug.

Comment 4

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

Attachment: Bug 1611303 - Always assume a flex item needs a block-axis resize in final reflow.

In an incremental reflow, if a flex item has a valid bsize cache , we
skip its measuring reflow. However, we may still need to set relevant
bsize flags if the flex container is changing its definiteness in the
block-axis. See bug 1611303 comment 2 for an analysis.

This patch is playing safe by always calling SetHasBSizeChange() if we
override bsize for the item. Of course this can be solved in a more
sophisticated way by checking whether the item really has a block-axis
resize, but that means we'll need to duplicate a lot of logic in
FlexItem::NeedsFinalReflow().

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99e249810495
Always assume a flex item needs a block-axis resize in final reflow. r=dholbert

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/99e249810495