1054046 - nsFlexContainerFrame shouldn't create a reflow state for measuring flex items' cross size, if it doesn't actually need to reflow them to do that measurement

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, we have this code for sizing flex items in the cross axis:
>3507         nsHTMLReflowState childReflowState(aPresContext, aReflowState,
>3508                                            item->Frame(), availSize);
[...]
>3516         SizeItemInCrossAxis(aPresContext, aAxisTracker,
>3517                             childReflowState, *item);

...where SizeItemInCrossAxis() is basically a no-op, if the cross axis is horizontal. (It just immediately returns childReflowState.ComputedWidth().)

So, we're creating a reflow state, and then possibly throwing it away without actually reflowing. This isn't kosher, now that (in bug 1054010) we may be skipping the item's final reflow.

This reflow state is bad if we don't end up reflowing because nsHTMLReflowState's constructor *makes changes to the frame*. In particular, it copies dirty bits from the flex container to the flex item.  And if we end up deciding we don't need to reflow the flex item after this point, then this code quoted above is setting the dirty bit without anyone clearing it.

Fortunately, we can get the information we need (childReflowState.ComputedWidth()) earlier on, in the FlexItem constructor -- at that point, we know we *are* going to be reflowing the flex item at least once, so the nsHTMLReflowState that we create there doesn't have the same problem.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

As comments in the patch describe, this will become a bit more complex when we support flex items with vertical writing modes (whose computed heights will influence their desired widths).

But for now, we already assume that the computed width can just be taken off of the reflow state, and for scenarios where that's valid (all scenarios at the moment), we're better off avoiding recreating the reflow state just to pick off that value. (since as described in comment 0, that causes problems if we decide we don't need to reflow, after creating that second reflow state.)

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8552711 [details] [diff] [review]
fix v1

This looks fine to me, r=mats.

When you revisit this code for vertical writing mode later...
Since there's just one SizeItemInCrossAxis call site, it might be simpler
to move all the stuff above it in the if-block to SizeItemInCrossAxis and
let it decide when to create 'childReflowState' for itself?
just a thought...

Comment 3

[Daniel Holbert [:dholbert]]

Thanks for the review! I'll land this later when the tree re-opens.

(RE moving the reflow state inside of SizeItemInCrossAxis -- that's a good suggestion, and I may end up doing that. I don't recall exactly why the reflow state lives outside of that method right now.)

Comment 4

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/1ee270f30171

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ee270f30171