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.
Summary: nsFlexContainerFrame shouldn't create a reflow state for measuring flex items' cross size, if it doesn't reflow them to do that measurement → 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
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Created attachment 8552711 [details] [diff] [review] 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.)
Attachment #8552711 - Flags: review?(mats)
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...
Attachment #8552711 - Flags: review?(mats) → review+
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.)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.