Closed
Bug 841847
Opened 11 years ago
Closed 11 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)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
(Keywords: regression)
Attachments
(4 files, 1 obsolete file)
448 bytes,
text/html
|
Details | |
500 bytes,
text/html
|
Details | |
685 bytes,
text/html
|
Details | |
9.08 KB,
patch
|
dbaron
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
(oops, previous testcase had a "<div>" instead of "</div>". Doesn't make a difference to the rendering, but fixing anyway for correctness.)
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
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
Assignee | ||
Comment 3•11 years ago
|
||
(For the record, here's the slightly-larger original testcase that I received via email, which tipped me off to this bug.)
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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
Assignee | ||
Comment 6•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite?
Comment on attachment 716883 [details] [diff] [review] fix v1 r=dbaron
Attachment #716883 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d339d3b173d4
Flags: in-testsuite? → in-testsuite+
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d339d3b173d4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 10•11 years ago
|
||
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?
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Assignee | ||
Comment 12•11 years ago
|
||
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 13•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
(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.)
Assignee | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1a067a80819
Assignee | ||
Comment 16•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/08938603bfaf
Assignee | ||
Updated•11 years ago
|
Comment 17•11 years ago
|
||
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.
Description
•