Closed Bug 821775 Opened 13 years ago Closed 12 years ago

{inc} After horizontal window-resize in quirks mode, vertically-stretched nested flex container is no longer stretched

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla21
Tracking Flags:
Tracking Status
firefox20 --- verified
firefox21 --- verified

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files, 3 obsolete files)

testcase 1
397 bytes, text/html
Details
fix v3a
21.01 KB, patch
dbaron
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review
STR: 1. Load testcase 2. Resize window horizontally EXPECTED RESULTS: No red visible. ACTUAL RESULTS: Red visible. (nested flex container becomes un-stretched) This appears somewhat similar to bug 821449, but that bug's patch doesn't fix this testcase, so I'm filing a new bug. Testcase reduced from bug 820111 (though it's not related to the original reported brokenness in that bug)
Attached file testcase 1 —
NOTE: If I add <!DOCTYPE html> (to trigger standards mode), then the bug doesn't reproduce.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Shouldn't this bug block 'css3-flexbox' rather than depend on it?
Possibly. I marked it as "depends" in the sense of "layers on top of / is only sensible in builds w/ css3-flexbox". But it's also a bug that we should fix before shipping flexbox, so blocking makes sense too.
Blocks: css3-flexbox
No longer depends on: css3-flexbox
FWIW: I think this happens to work correctly in standards-mode, but *not* quirks-mode, due to a difference in whether the body-nsBlockFrame's reflowstate gets its mVResize flag set to true in the reflow after a horizontal window-resize. In standards-mode, that flag gets set to true, but in quirks mode, it doesn't, because of this chunk (run on the body nsBlockFrame): https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=cf8196bc19b4&mark=540-546#539 That affects our behavior on this testcase because the body-reflowState's mVResize value gets copied into the outer flex container's reflowState's mVResize flag, and *that* flag gets copied into the *inner* flex container's reflowState's mVResize flag -- both of those copies happen via this line: https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=cf8196bc19b4&mark=538-538#533 So -- if the inner flex container's mVResize is false, that means we end up returning false from ShouldReflowAllKids() when we're reflowing it. So we leave its (anonymous-block flex item) child at its last height, which was from when we reflowed the inner flex container at an un-stretched height. IN ANY CASE: we shouldn't rely on that flag being passed down from the root block frame -- the relevant vertical-resize here is the resize on the inner flex container itself, between the outer flex container's "measuring" and "real" reflows. I think we might need to add a reflow-state flag that the outer flex container can use to tell its stretched flex items that they're being vertically resized during their stretch reflow. (CONTAINS_RELATIVE_HEIGHT isn't enough -- we need to give them a little more, in order for them to set mVResize in this situation.)
Attached patch fix v1 (obsolete) — — Splinter Review
I went through a few options here[1], but I think this is the most straightforward. Right now, the testcase's inner flex container ends up visiting the "!nsLayoutUtils::IsNonWrapperBlock(frame)" clause on its "real" reflow after a window-resize. That clause ends up copying mVResize = false from the outer flex container. I don't think we want that to happen -- I don't think we want to use that clause for flex items. Rather, I think we want to take the later clauses and actually compare mComputedHeight (if it's finite) to the frame's existing height, and toggle mVResize based on that. I think that's what we want for flex items in general -- they shouldn't pay attention to mVResize on their container, because the container may resize them vertically without its own height changing. So, instead, it's more appropriate for flex items to set mVResize based on whether their own computed height has changed. [1] other options that I tried which also would fix this, but aren't as clean IMO: - Adding a new reflow state flag for "are we currently being stretched by a flex container", which we check in InitConstraints() and set mVResize accordingly. - Adding a special case at the beginning of nsFlexContainerFrame::Reflow to detect height changes & force a reflow (which is sort of a delayed version of what the attached patch does)
Attachment #697352 - Flags: review?(dbaron)
Comment on attachment 697352 [details] [diff] [review] fix v1 So this doesn't really seem right to me. The basic logic of this part of InitResizeFlags (and the reasons blocks take different codepaths) is that we take different codepaths for elements that percentage heights can be relative to (blocks, non-wrapper, since they're what can establish the containing block for something) and those that they can't be relative to (e.g., inlines). So the first question is whether (a) flexboxes and (b) flex items are containing blocks (for percentage heights). The spec says that (a) is clearly true (where it introduces display:flex); I suppose whether flex-items are containing blocks depends on their display type, as normal (though after the forcing-to-block that happens). so I think nsIFrame::GetContainingBlock probably needs adjusting -- though you should probably go through its callers. And I suspect that it would be good to turn the "is this frame a containing block" test that GetContainingBlock contains inside it into a method that InitResizeFlags can call. Then, on top of that, there's the issue of InitResizeFlags needing to do *more* than that to handle stretch. Is NS_FRAME_CONTAINS_RELATIVE_HEIGHT being set on the flexbox when one of its kids is stretched vertically? (I hope so.) If so, the next question is whether the reflow state construction is looking at an auto height here (which I"d think it would be.) If both of those are as I expect, I'd think what you'd want to fix the principal bug here would be to check if: - this frame is a flex item - and it's stretched in the vertical dimension - and its parent has mVResize set then it should also set mVResize. (But I'd think that flex items that aren't containing blocks for percentage heights would need to propagate the mVResize on their parent unconditionally. Or something like that.)
Attachment #697352 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] from comment #7) > So the first question is whether (a) flexboxes and (b) flex items > are containing blocks (for percentage heights). The spec says that > (a) is clearly true (where it introduces display:flex); I suppose > whether flex-items are containing blocks depends on their display > type, as normal (though after the forcing-to-block that happens). Agreed. I think, generally, both will be containing blocks for percentage heights. > so I think nsIFrame::GetContainingBlock probably needs adjusting -- > though you should probably go through its callers. OK -- so basically: if (this->IsFlexItem()) { return GetParent(); } That makes sense. (I'm not sure it impacts our behavior on this bug's testcase, though). > And I suspect that it would be good to turn the "is this frame a > containing block" test that GetContainingBlock contains inside it > into a method that InitResizeFlags can call. I'm not sure I understand what you mean here. Right now, GetContainingBlock() just has a check for "is this frame absolutely positioned?", and then the rest of it calls out to a helper-method. Which was the logic that you're suggesting to split out? https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#5368 > Then, on top of that, there's the issue of InitResizeFlags needing > to do *more* than that to handle stretch. Is > NS_FRAME_CONTAINS_RELATIVE_HEIGHT being set on the flexbox when one of its > kids is stretched vertically? (I hope so.) We set that flag on the *stretched flex item* (via the patch for Bug 821449), but currently we don't set that flag on the flex container just because it has a stretched child. Based on the documentation for that flag, it doesn't seem to me like it should be set on the container... The documentation says it's set if "this frame or one of its descendants has a percentage height that depends on an ancestor of this frame." And if we've got a stretched flex item, then the item's height depends on "this frame" (the flex container), *not* on "an ancestor of this frame". Having said that, it looks like we do set it in other cases when our kids depend on us (e.g. in nsColumnSetFrame.cpp), so maybe I'm misunderstanding that documentation (or maybe it's outdated). > If so, the next question is > whether the reflow state construction is looking at an auto height here > (which I"d think it would be.) If both of those are as I expect, I'd think > what you'd want to fix the principal bug here would be to check if: > - this frame is a flex item > - and it's stretched in the vertical dimension > - and its parent has mVResize set > then it should also set mVResize. I think I agree, *except* for the last bullet-point about the parent having mVResize set. If we depend on that, then we're not sensitive enough. In this bug's testcase, *none* of our ancestors are undergoing a vertical resize -- we're resizing the window _horizontally_ -- so the mVResize flags are 0, all the way up the chain. But from the perspective of innerContainer (in the testcase), *we're* being vertically resized, when we get our 'stretch' reflow. When any flex item gets a "measuring" reflow and then a "real" stretched-height reflow, it's basically undergoing its own private vertical-resize, regardless of whether its parent happens to have mVResize set. So, that's why my patch tries to get flex items past the "copy my parent" clauses and down to the "OK, forget about my parent -- did *my* height actually change since my last reflow?" clause. You're right that we can be more sensitive about which children we do that for, though -- it only needs to happen for vertically-stretched flex items, not all flex items.
(In reply to Daniel Holbert [:dholbert] from comment #8) > But from the perspective of innerContainer (in the testcase), *we're* being > vertically resized, when we get our 'stretch' reflow. When any flex item > gets a "measuring" reflow and then a "real" stretched-height reflow, it's > basically undergoing its own private vertical-resize, regardless of whether > its parent happens to have mVResize set. > > So, that's why my patch tries to get flex items past the "copy my parent" > clauses and down to the "OK, forget about my parent -- did *my* height > actually change since my last reflow?" clause. Ah, I think I misunderstood the bit about the measuring reflow and the real reflow. In that case, maybe the caller that's constructing the reflow state should force-set mVResize in the cases where the previous reflow's "is a measuring reflow" is different from the current? (I suspect the caller can figure that out without tracking it, if you do a measuring reflow before every real reflow.)
(In reply to David Baron [:dbaron] from comment #9) > Ah, I think I misunderstood the bit about the measuring reflow and the real > reflow. In that case, maybe the caller that's constructing the reflow state > should force-set mVResize in the cases where the previous reflow's "is a > measuring reflow" is different from the current? Yeah, that's simpler, and it's probably the best way to fix this. (I'd thought of that before, but I initially avoided that strategy because various reflowState.SetComputed[Height|Width]() (which I call in a few places) will stomp on mHResize and mVResize -- so if I naively set the flags at the point in the code where we know they need to be set, they may get clobbered a few lines later). So, this new patch adds a flag to track whether we've done a measuring reflow -- and if we have, and we end up overriding height and/or width, then this patch makes us set mHResize and/or mVResize as-appropriate.
Attachment #697352 - Attachment is obsolete: true
Attachment #703778 - Flags: review?(dbaron)
BTW, there are two places where the flex container does measuring reflows on its flex items: (1) if our flex container is vertical, it'll need to measure the height of any children that have a flex-basis or min-height of "auto". (in order to know where to start flexing those items from, and so it can apply the correct min-height clamping when resolving flexible lengths) (2) if our flex container is horizontal, it'll need to measure the height of all of its children, so that it can properly align them per their "align-self" values, and also so we can compute the height of our flex container if it's auto-height itself. (so that we can then stretch any align-self:stretch children to that height before giving them their 'real' reflow) So -- there are two places where we do measuring reflows -- but one is only for vertical flex containers, and one is only for horizontal flex containers. So, we're guaranteed that each flex items will receive no more than 1 measuring reflow before it gets its "real" reflow.
Comment on attachment 703778 [details] [diff] [review] fix v2: set mVResize and/or mHResize if we did a measuring reflow and we're overriding height and/or width in the final reflow I think you're missing the actual call to HadMeasuringReflow() that you intended to add. Also, do you need to set the resize flags in the measuring reflow too, in addition to after the measuring reflow? (I also wonder why nsFlexContainerFrame::AppendFlexItemForChild return nsresult when it always returns NS_OK; it seems like it could return void.) I think it's also worth getting bugs filed on the other things in comment 7.
Attachment #703778 - Flags: review?(dbaron) → review-
Attached patch fix v3 (obsolete) — — Splinter Review
(In reply to David Baron [:dbaron] from comment #12) > Comment on attachment 703778 [details] [diff] [review] > I think you're missing the actual call to HadMeasuringReflow() that you > intended to add. Oops! Thanks for catching that. Added. > Also, do you need to set the resize flags in the measuring reflow too, in > addition to after the measuring reflow? Not for this bug's testcase to work* but in general, I think so. I can imagine needing to forcibly set that flag on the measuring reflow, e.g. if a flex container's parent decides to reflow it even though it's not dirty and none of its dimensions have changed. In that case, it'd still need to tell its stretched kids that *they're* being effectively resized for their measuring reflows. So: I addressed this in the patch w/ conditional "mVResize" tweaks before the measuring reflows. (I don't know how to make a testcase that tests this, though, since I think it could only run into trouble when we do a reflow when nothing's changed, basically, and our code is generally smart enough to not bother reflowing in that situation.) * In this bug's testcase, this wasn't needed because the initial "measuring reflow" is right after the window-resize -- so that's where the flex items see that their computed width is changing, and they set mHResize = 1, which makes them reflow greedily. (in particular, it makes ShouldReflowAllKids() return true). > (I also wonder why nsFlexContainerFrame::AppendFlexItemForChild return > nsresult when it always returns NS_OK; it seems like it could return void.) It returns nsresult because it has "NS_ENSURE_SUCCESS(rv, rv);" statements after its ReflowChild/FinishReflowChild calls, to propagate any measuring-reflow-failures upwards. > I think it's also worth getting bugs filed on the other things in comment 7. I filed bug 832168 on the "nsIFrame::GetContainingBlock probably needs adjusting" part (to treat flex containers as containing blocks). I think that's the only thing that needed a new bug filed -- I don't think we ended up needing to split off a helper-method for use in InitResizeFlags as you'd suggested, since I'm no longer touching InitResizeFlags; and I think (?) I'm setting NS_FRAME_CONTAINS_RELATIVE_HEIGHT correctly as I noted in comment 8, unless I'm misunderstanding what that flag means.
Attachment #703778 - Attachment is obsolete: true
Attachment #705226 - Flags: review?(dbaron)
Attached patch fix v3a — — Splinter Review
This patch's resize-flag-tweak makes the crashtest 798020-1.html fail a few more assertions about doing math on unconstrained sizes (because it has text with "letter-spacing: 693626589697em"). I don't think there's really anything we can do about that, aside fixing that bug about suppressing assertions when we detect a ridiculously-sized element. Anyway, here's a new patch w/ that crashtest's assertion-count increased to account for this.
Attachment #705226 - Attachment is obsolete: true
Attachment #705226 - Flags: review?(dbaron)
Attachment #705655 - Flags: review?(dbaron)
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/9f5df0139930
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment on attachment 705655 [details] [diff] [review] fix v3a I'd like to take this on Beta. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 666041 (it's a bug in our initial css3 flexbox implementation) User impact if declined: Users & web developers who are working with CSS3 flexbox (and have the flexbox pref enabled) will see content unexpectedly change position/size (possibly disappearing) in some cases, when resizing the browser-window. Testing completed (on m-c, etc.): Landed with a reftest included, and has been baking on mozilla-central for 10 days (and has since made it to Aurora 21). Risk to taking this patch (and alternatives if risky): Basically zero risk, because this feature is preffed off on beta/release (bug 841873). For users who have toggled the pref, this is still low-risk -- the code-change makes a flex container's children realize that they need a reflow, in situations where they previously wouldn't recognize that. String or UUID changes made by this patch: None
Attachment #705655 - Flags: approval-mozilla-beta?
(note: if/when this lands on beta, we should also land the comment-only tweak https://hg.mozilla.org/mozilla-central/rev/e7b94c8df70c in the same push -- this bug's patch layers on top of that.)
Comment on attachment 705655 [details] [diff] [review] fix v3a Low risk and has been on central for a week, approving for uplift to beta.
Attachment #705655 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This is not fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0b2
But it's verified fixed FF 21.0a2 (2013-02-27) Win 7, Ubuntu 12.04, Mac OS X 10.8.2
status-firefox21: fixed → verified
Paul: Thanks for the info -- do you know if that beta build that you tested actually included the change? (What's the "built from" URL, at about:buildconfig in that build?)
(I don't know how often beta builds are published, but I'm under the impression that it's infrequent; so, given that this just landed on beta 2 days ago, I wouldn't be surprised if there hasn't been a new beta build posted in that time.)
(In reply to Daniel Holbert [:dholbert] from comment #24) > Paul: Thanks for the info -- do you know if that beta build that you tested > actually included the change? (What's the "built from" URL, at > about:buildconfig in that build?) http://hg.mozilla.org/releases/mozilla-beta/rev/f45f4b3cba11 If I look in the pushlog between 20b1 and 20b2 I see this bug on the list: http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=f9348d70933c&tochange=f45f4b3cba11 (In reply to Daniel Holbert [:dholbert] from comment #25) > (I don't know how often beta builds are published once a week > so, given that this just landed on beta 2 > days ago, I wouldn't be surprised if there hasn't been a new beta build > posted in that time.) this landed in beta on 2013-02-26 - comment 26 FF 20b2 was built on 2013-02-27
(In reply to Paul Silaghi [QA] from comment #26) > this landed in beta on 2013-02-26 - comment 26 comment 21, sorry
On beta, you'll need to toggle the flexbox pref for the test to actually be testing what it's supposed to be testing. (Visit about:config, search for flexbox, set the pref to true) If you do that, then does this work?
(I'm pretty sure that was it -- sorry for not mentioning that earlier.) I just updated my own Firefox beta-version install, and (with the pref set) the STR give EXPECTED RESULTS for me. So I'm pretty sure this is fixed on beta.
Indeed, everything's ok now. Thank you very much for explaining. Verified fixed FF 20b2 Win 7, Ubuntu 12.04, Mac OS X 10.8.2
Status: RESOLVED → VERIFIED
status-firefox20: fixed → verified
Great, thanks!
Blocks: 841876
No longer depends on: 841876
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: