Open Bug 1611303 Opened 1 year ago Updated 2 months ago

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

Categories

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

defect

Tracking

()

Tracking Status
firefox74 --- affected

People

(Reporter: dholbert, Unassigned)

References

Details

Attachments

(1 file)

Attached file 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.

(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.

Depends on: 1092007

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.

You need to log in before you can comment on or make changes to this bug.