Closed Bug 841847 Opened 7 years ago Closed 7 years ago

{inc} After horizontal window-resize, vertical flex container's flex items shrink to auto-height and jump to upper-left corner of container's border box

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 --- fixed
firefox21 --- fixed
firefox22 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Attached file testcase 1 (obsolete) —
STR:
 1. load attached testcase.
 2. Resize window horizontally

EXPECTED RESULTS: No change
ACTUAL RESULTS:
 - The black-outlined flex item is shrunken to its auto height.
 - The black-outlined flex item shifts up & to the left, overlapping the container's border.
Attached file testcase 1
(oops, previous testcase had a "<div>" instead of "</div>".  Doesn't make a difference to the rendering, but fixing anyway for correctness.)
Assignee: nobody → dholbert
Attachment #714523 - Attachment is obsolete: true
Status: NEW → ASSIGNED
As this testcase shows, when there are multiple flex items, they end up piling on top of each other (at the containers upper-left border-box corner) after a horizontal window-resize.
OS: Linux → All
Hardware: x86_64 → All
Summary: {inc} After horizontal window-resize, flex item in vertical flexbox snaps to auto-height and shifts into container's border → {inc} After horizontal window-resize, vertical flex container's flex items shrink to auto-height and jump to upper-left corner of container's border box
Attached file (original testcase)
(For the record, here's the slightly-larger original testcase that I received via email, which tipped me off to this bug.)
OK, so what's happening here on the incremental (window-resize) reflow is:
 - We enter nsFlexContainerFrame::Reflow(). Inside that:
   - We notice that none of our children need a reflow (shouldReflowAllChildren = false).
   - Still, we generate our FlexItem array, so we can compute our main size. Given that we're a vertical flex container, this involves doing a "measuring" reflow for any auto-height children.
   - Now: since shouldReflowAllChildren is false, we skip the "actual" reflow of our kids.

This leaves them with their most recent reflow being the "measuring" reflow, which is wrong. (it leaves them at the wrong position)

I'm somewhat inclined to remove the "shouldReflowAllChildren" bool entirely... but for the purposes of this bug, we can do a more targeted fix to update shouldReflowAllChildren based on whether any of our kids got a measuring reflow.
The measuring reflow's effects have only been persistent since bug 821426 landed.  So technically, this is a regression from bug 821426.  (Confirmed by testing nightly builds on either side of that landing.)
Blocks: 821426
Keywords: regression
Attached patch fix v1Splinter Review
This patch makes us reflow all our children if we ended up giving any of them a "measuring" reflow.

2 reftests included -- one with a fixed-height flex item (which gets a measuring reflow to resolve its implicit 'min-height:auto'), and one with an auto-height flex item (which gets a measuring reflow to resolve its "height:auto").

(I hg cp'd the reftests from an earlier similar reftest -- apologies if that confuses bugzilla's diff viewer)
Attachment #716883 - Flags: review?(dbaron)
Flags: in-testsuite?
Comment on attachment 716883 [details] [diff] [review]
fix v1

r=dbaron
Attachment #716883 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/d339d3b173d4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
I'm aware of a use of flexbox in the wild that hit this bug. Will this bug cause widespread issues in the use of flexbox? If the former is true, is the patch safe enough to be uplifted now that we're early in the cycle?
(In reply to John Volikas from comment #10)

Note that on Beta & Release builds, we have flexbox support preffed off by default while some last issues (including this bug) are worked out -- that happened in bug 841873.  (bug 841876 tracks re-enabling it.)

So this will only affect people who are running Aurora/Nightly builds, and those who've opted to turn on the pref in their release build.

> is the
> patch safe enough to be uplifted now that we're early in the cycle?

Good question.
 - Definitely safe enough for an aurora uplift; we'd like people testing this to have working vertical flex containers.
 - Possibly safe enough for a beta uplift, by the same justification *plus* there's basically 0 risk of causing issues on beta since this is preffed off there.

I'll request uplift approval.
Comment on attachment 716883 [details] [diff] [review]
fix v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 821426

User impact if declined: Sites developed with vertical CSS3 flexbox will break when the browser-window is resized, on Aurora/Nightly builds, or in release builds w/ the pref enabled.

Testing completed (on m-c, etc.): Local testing, it's baked for a few days on m-c. Patch landed with 2 reftests (which I confirmed to fail before the fix & pass after the fix).

Risk to taking this patch (and alternatives if risky):
 * On Aurora: low risk. This just disables an optimization that skips some child reflows, in a situation where the child really does need the reflow.
 * On Beta: basically zero risk, since this feature is preffed off there (bug 841873).  For those users who've opted to pref it on, this is still low risk, per the Aurora risk overview above.

String or UUID changes made by this patch: None
Attachment #716883 - Flags: approval-mozilla-beta?
Attachment #716883 - Flags: approval-mozilla-aurora?
Comment on attachment 716883 [details] [diff] [review]
fix v1

Based on risk, we can take this since it's early in 20beta cycle.
Attachment #716883 - Flags: approval-mozilla-beta?
Attachment #716883 - Flags: approval-mozilla-beta+
Attachment #716883 - Flags: approval-mozilla-aurora?
Attachment #716883 - Flags: approval-mozilla-aurora+
(I just realized that the reftest part of this bug's patch depends on the patch from similar bug 821775.  That bug was fixed in Aurora 21 (by landing before the last merge day) but isn't yet fixed on Beta.  Rather than hack up a custom beta patch, I've requested beta approval over on bug 821775 - once that's landed, this bug's m-c patch should apply cleanly on beta.)
Blocks: 841876
No longer depends on: 841876
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0

Verified on Firefox 21.0b5, on Windows 7, 64 bits using the two provided testcases.
You need to log in before you can comment on or make changes to this bug.