Closed
Bug 775350
Opened 11 years ago
Closed 11 years ago
Lexus.com navigational menu item expands and collapses, when it should open and remain open
Categories
(Core :: Layout, defect)
Tracking
()
People
(Reporter: stephend, Assigned: ehsan.akhgari)
References
()
Details
(Keywords: regression, Whiteboard: [description of the UpdateOverflow bug in comment 14])
Attachments
(5 files, 3 obsolete files)
6.97 KB,
text/html
|
Details | |
20.46 KB,
text/plain
|
Details | |
159.25 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
633 bytes,
text/html
|
Details | |
2.94 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
Build ID: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 STR: 1. Load http://www.lexus.com/ 2. Hover over "Future Vehicles" 3. Mouse away 4. Go back to it and stay over it Expected Results: Menu item opens on hover, and remains open Actual Results: The menu item continually collapses and expands: http://screencast.com/t/aylxIEhBki
![]() |
||
Comment 1•11 years ago
|
||
Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/c29b842c4159 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120606221720 Bad: http://hg.mozilla.org/mozilla-central/rev/3933384d8315 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120607023619 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c29b842c4159&tochange=3933384d8315 Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/f5a441d6929f Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120605210720 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/df6702c41ddd Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120605215419 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f5a441d6929f&tochange=df6702c41ddd Suspected: Bug 157681
Blocks: 157681
Status: UNCONFIRMED → NEW
tracking-firefox16:
--- → ?
tracking-firefox17:
--- → ?
Component: DOM: Events → Layout
Ever confirmed: true
Keywords: regression
Version: Trunk → 16 Branch
Assignee: nobody → ehsan
Comment 2•11 years ago
|
||
Unfortunately, I won't be able to work on it further, but this should give an indication where the bug is coming from. Steps to reproduce: - Move your mouse over the red box It seems that in trunk, the movement of the white absolutely positioned box under the red box is triggering mouseover events, which then causes the function that triggers the movement of the white absolutely positioned box to go on.
Assignee | ||
Comment 3•11 years ago
|
||
Thanks Martijn for the test case. I tracked it down today to us getting the wrong element here <http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#4185> if we avoid reflowing (which is the optimization that bug 157681 implements.) My guess is that this is happening because of something which happens during the reflow process and my fast path is missing, but I don't know what that is yet.
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #644044 -
Attachment is obsolete: true
Assignee | ||
Comment 5•11 years ago
|
||
Attached the wrong file!
Attachment #644072 -
Attachment is obsolete: true
Assignee | ||
Comment 6•11 years ago
|
||
So I finally convinced myself that this is happening because I'm not moving the view corresponding to the div, and that causes this call <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5151> to fail to return the correct view, which as a result causes the mosemove event to be dispatched to the wrong element (the div not the anchor.) If you remove the float property from line 5 in the test case, the bug stop happening. I know almost nothing about the view hierarchy. Can someone please tell me how I should get the correct view in nsCSSFrameConstructor::RecomputePosition, and if nsIView::SetPosition() is the right function to move it? Thanks!
Comment 7•11 years ago
|
||
The only things that have views now are root frames, object frames, subdocument frames, and popup panels. From what I can tell of the testcase it should only have one view, the root view on the root frame. (The term 'floating view' is for popup panels and has no relation to CSS floats.) So from what I can tell quickly that doesn't seem like it could be the problem.
![]() |
||
Comment 8•11 years ago
|
||
As far as the right position for the view, nsContainerFrame::SyncFrameViewAfterReflow would do it. But it's worth checking whether you have a view at all (via GetView()).
Assignee | ||
Comment 9•11 years ago
|
||
Oh well, I tried calling nsContainerFrame::PositionFrameView(aFrame); after repositioning the frame but that didn't help either. Not sure how to proceed here really. Timothy, I'm seeing quite a large view tree under the debugger, I don't know why, but the thing that I got out of the layout debugger seems to be quite misleading, and the thing that I have by calling rootView->List(__stdoutp, 0) contradicts what you're saying.
Assignee | ||
Comment 10•11 years ago
|
||
OK so now I have more information. the nsLayoutUtils::GetFrameForPoint call in PresShell::HandleEvent is returning the block frame for the div when hit-testing, as opposed to the block frame for the anchor element, and this is where the wrong element in comment 6 is coming from. But still it's not clear to me why this happens. The geometries seem to be the same whether or not reflow happens. I guess I need to delve into our hit-testing code now...
Assignee | ||
Comment 11•11 years ago
|
||
(Oh, also Timothy demonstrated to me in person that my theory about views is incorrect.)
Comment 12•11 years ago
|
||
If the frame tree is the same then the hit testing should be the same so I would think that the problem is elsewhere.
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to comment #12) > If the frame tree is the same then the hit testing should be the same so I > would think that the problem is elsewhere. Well the frame trees seem to be the same (at least the geometry of the block frame in question is the same) and stepping through the code I'm pretty sure that the result of the hit-testing _is_ different, which is the puzzling part.
Updated•11 years ago
|
Assignee | ||
Comment 14•11 years ago
|
||
So here's a frame tree diff comparing the case where we do reflow to the one that we don't. Both frametrees are snapshotted after the first round of animation is done. The only difference that I can spot in this frame tree is the overflow areas for this line: 0x116597ee8. With a reflow, the overflow area looks like this: vis-overflow={0,0,60000,17040} scr-overflow={0,0,60000,17040} Without reflow, the overflow area looks like this: vis-overflow={59880,0,0,0} scr-overflow={59880,0,0,0} I use the UpdateOverflow hint together with the RecomputePosition hint. Shouldn't that take care of updating the overflow areas properly?
Yes. I guess that's not working...
Assignee | ||
Comment 16•11 years ago
|
||
Matt, do you know why the UpdateOverflow hint is not working correctly here? Can you please take a look at this? Thanks!
Comment 17•11 years ago
|
||
Mats might be a better person to ask.
Assignee | ||
Comment 18•11 years ago
|
||
Jet, would you mind looking to see if someone who's familiar with how the UpdateOverflow hint works can take a look at this? I'm not sure how that hint works, and I'd hate for bug 157681 to get backed out because of this regression. :( Also, the thing which scares me a bit more is that this bug may manifest itself in other ways even for situations where the optimization in bug 157681 doesn't kick in, as the UpdateOverflow hint is also used in a number of other situations. Thanks!
Assignee: ehsan → bugs
Assignee | ||
Comment 19•11 years ago
|
||
Firefox 16 will go to Beta today. We need to backout bug 157681 from Beta since this wasn't fixed in time. :(
Assignee | ||
Comment 20•11 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): 157681 User impact if declined: layout regressions on some websites Testing completed (on m-c, etc.): the patch backs out the offending changeset Risk to taking this patch (and alternatives if risky): no other solution to this bug is currently known String or UUID changes made by this patch: none
Attachment #655694 -
Flags: approval-mozilla-beta?
Comment 21•11 years ago
|
||
Comment on attachment 655694 [details] [diff] [review] Backout bug 157681 [Triage Comment] Will prevent web regression in FF16 - approved for beta.
Attachment #655694 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/3d7907e9e5a9
status-firefox16:
--- → fixed
Reporter | ||
Comment 23•11 years ago
|
||
This bug is happening again, at least with Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/18.0 Firefox/18.0 (and probably earlier nightly builds...)
![]() |
||
Comment 24•11 years ago
|
||
Fixed in Firefox16beta only. And I can reproduce in Aurora17.0a2 and Nightly18.0a1
tracking-firefox18:
--- → ?
Updated•11 years ago
|
Assignee | ||
Comment 25•11 years ago
|
||
Comment on attachment 655694 [details] [diff] [review] Backout bug 157681 Sigh, need to back out from Aurora as well...
Attachment #655694 -
Flags: approval-mozilla-aurora?
Comment 26•11 years ago
|
||
Comment on attachment 655694 [details] [diff] [review] Backout bug 157681 Thanks Ehsan, let's get this in before merge.
Attachment #655694 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1ff2abbedea7
status-firefox17:
--- → fixed
Assignee | ||
Comment 28•11 years ago
|
||
The backout broke the build, so I backed it out: https://hg.mozilla.org/releases/mozilla-aurora/rev/37e18e9c9ca9
Assignee | ||
Comment 29•11 years ago
|
||
Fixed and relanded: https://hg.mozilla.org/releases/mozilla-aurora/rev/151ad4f02509
Assignee | ||
Comment 30•11 years ago
|
||
Backed it out again because of reftest failures this time: https://hg.mozilla.org/releases/mozilla-aurora/rev/ce2acecd4d79 Example log: https://tbpl.mozilla.org/php/getParsedLog.php?id=15569134&tree=Mozilla-Aurora dholber, jwatt: any idea what's going on here?
Assignee | ||
Comment 31•11 years ago
|
||
Specifically, could this be because of bug 614732?
![]() |
||
Comment 32•11 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #3) > I tracked it down today to us getting the > wrong element here > <http://mxr.mozilla.org/mozilla-central/source/content/events/src/ > nsEventStateManager.cpp#4185> [For future reference, please, please use versioned hg.mozilla.org links.]
Assignee | ||
Comment 33•11 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #32) > (In reply to Ehsan Akhgari [:ehsan] from comment #3) > > I tracked it down today to us getting the > > wrong element here > > <http://mxr.mozilla.org/mozilla-central/source/content/events/src/ > > nsEventStateManager.cpp#4185> > > [For future reference, please, please use versioned hg.mozilla.org links.] Sorry, I believe this is the correct line: http://hg.mozilla.org/mozilla-central/annotate/aacf4867f830/content/events/src/nsEventStateManager.cpp#l4175 FWIW, my final analysis is in comment 14 which basically narrows down the bug. I just didn't know where to go from there.
![]() |
||
Comment 34•11 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #31) > Specifically, could this be because of bug 614732? I don't know (any particular reason you think it might?). I've spent a bit of time digging into why that test is failing. Basically the FillProperty() frame property is not being updated for the nsSVGPathGeometryFrame. We invalidate and repaint the correct area, but with the old frame property that points to the old (red) gradient. The old frame property should be discarded when nsCSSFrameConstructor::ProcessRestyledFrames calls nsSVGEffects::UpdateEffects when it sees the nsChangeHint_UpdateEffects hint: http://hg.mozilla.org/mozilla-central/annotate/e327e66a027d/layout/base/nsCSSFrameConstructor.cpp#l8102 In a working build we hit this call twice (once for the <g>, once for the <rect>), but in the broken build we hit it only once (for the <g>). As far as figuring out why we don't hit that call for the <rect> in a broken build, I've only got as far as noticing that the hint is added to the restyle hints in nsStyleSVG::CalcDifference in the |mFill != aOther.mFill| check four times when the "fill" attribute changes in the testcase - both in broken and working builds. That's as far as I'll get today.
Comment 35•11 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #34) > (In reply to Ehsan Akhgari [:ehsan] from comment #31) > > Specifically, could this be because of bug 614732? > > I don't know (any particular reason you think it might?). I suggested that as a possible-cause to ehsan in IRC -- this bug's patch (a backout) isn't generally supposed to affect behavior, and it only started causing problems in Aurora (not earlier branches), and bug 614732 is the widest-reaching SVG change to land in the current-Aurora timeframe (and could feasibly be involved in invalidation issues). It's a guess, though. Seems like some local targeted builds (with this bug's backout applied) could be helpful in confirming that guess or establishing a more definitive cause.
Assignee | ||
Comment 36•11 years ago
|
||
So I tried to reproduce this locally and I get tons of failures in SVG reftests with and without this backout. It would be nice if someone can try and see if backing out bug 614732 on top of my backout fixes the reftest. In that case, we should probably backout both on Aurora and then later on figure out the SVG bug...
![]() |
||
Comment 37•11 years ago
|
||
Okay, I've figure this out. Basically the patch from comment 29 also backs out a portion of bug 775304 (the addition of nsChangeHint_UpdateEffects to nsChangeHint_NonInherited_Hints in nsChangeHint.h). The result is that in CaptureChange we end up with an aMinChange that already includes the nsChangeHint_UpdateEffects hint, which makes the NS_UpdateHint(aMinChange, ourChange) call return false, which prevents us reaching the AppendChange() call for the nsSVGPathGeometryFrame, which prevents us calling nsSVGEffects::UpdateEffects as noted in comment 34. As it happens, bug 775304 needs to be fully reverted from Aurora (see bug 782888 comment 10). Once I do that, you should be able to reland without the gradient test failure, Ehsan.
Assignee | ||
Comment 38•11 years ago
|
||
OK, great! Could you please comment here when that happens so that I don't forget? Thanks!
![]() |
||
Comment 39•11 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #37) > As it happens, bug 775304 needs to be fully reverted from Aurora (see bug > 782888 comment 10). Once I do that, you should be able to reland without the > gradient test failure, Ehsan. In the end, reverting it didn't work, so heycam wrote a fix. Ehsan, I suggest you try to modify your backout for this patch so that it doesn't back out the portion of bug 775304 noted in comment 37.
Assignee | ||
Comment 40•11 years ago
|
||
Alright, let's see how this one goes: https://hg.mozilla.org/releases/mozilla-beta/rev/5ae79f2880ff
Assignee | ||
Comment 41•11 years ago
|
||
Backed out from Aurora as well: https://hg.mozilla.org/releases/mozilla-aurora/rev/cf470cfd18ca
Assignee | ||
Updated•11 years ago
|
Whiteboard: [description of the UpdateOverflow bug in comment 14]
![]() |
||
Updated•11 years ago
|
Assignee: bugs → jwatt
OS: Windows 7 → All
Hardware: x86_64 → All
![]() |
||
Comment 42•11 years ago
|
||
The current "Minimized test case" no longer seemed to work for me, so I had to reduce another from the original site. Much blood, sweat and tears (mostly tears) were shed getting this.
![]() |
||
Comment 43•11 years ago
|
||
Using the testcase I just attached, I tracked this issue down to the fact that nsBlockFrame::UpdateOverflow does not account for the overflow of its floats in the overflow areas it passes when it calls SetOverflowAreas on its lines. dbaron did a bit of code digging for me and confirmed that during reflow we do add the overflow areas of a block's floats to the overflow areas of its lines. After digging into the code myself to expand on dbaron's pointers, this is my current understanding: nsBlockFrame::Reflow creates an nsBlockReflowState on the stack. The overflow rects of the nsBlockFrame's floats are unioned into the nsBlockReflowState's mFloatOverflowAreas member when its nsBlockReflowState::FlowAndPlaceFloat method is called. Such calls can happen in various ways under nsBlockFrame::Reflow. Three examples, in the order in which they would occur are: nsBlockReflowState::FlowAndPlaceFloat nsBlockFrame::ReflowPushedFloats nsBlockFrame::Reflow nsBlockReflowState::FlowAndPlaceFloat nsBlockReflowState::AddFloat nsLineLayout::AddFloat nsLineLayout::ReflowFrame nsBlockFrame::ReflowInlineFrame nsBlockFrame::DoReflowInlineFrames nsBlockFrame::ReflowInlineFrames nsBlockFrame::ReflowLine nsBlockFrame::ReflowDirtyLines nsBlockFrame::Reflow nsBlockReflowState::FlowAndPlaceFloat nsBlockReflowState::PlaceBelowCurrentLineFloats nsBlockFrame::PlaceLine nsBlockFrame::DoReflowInlineFrames nsBlockFrame::ReflowInlineFrames nsBlockFrame::ReflowLine nsBlockFrame::ReflowDirtyLines nsBlockFrame::Reflow The overflow areas of an nsBlockFrame's lines can then be set under: nsLineBox::SetOverflowAreas nsBlockFrame::PlaceLine nsBlockFrame::DoReflowInlineFrames nsBlockFrame::ReflowInlineFrames nsBlockFrame::ReflowLine nsBlockFrame::ReflowDirtyLines nsBlockFrame::Reflow Here we're interested in the second SetOverflowAreas() call in PlaceLine (the call from the |if (aLine->HasFloats())| block). The overflow areas here come from the mFloatOverflowAreas member of the nsBlockReflowState created by nsBlockFrame::Reflow. In other words, the union of the overflow areas of the nsBlockFrame's floats are added to the overflow areas of each of its non-block lines that contain at least one of the floats. I believe this is what nsBlockFrame::UpdateOverflow needs to do to fix this bug.
![]() |
||
Comment 44•11 years ago
|
||
On IRC dbaron also noted that UpdateOverflow seems to fail to account for other things that are accounted for in overflow areas during reflow, such as not accounting for bullets, pushed floats, abs pos elements, overflow containers, and the bottom-edge-of-children. I've yet to find time to dig into that and understand the reflow code sufficiently to account for those things in UpdateOverflow though. Maybe I should have just fixed the issue detailed in the last paragraph of comment 43 to at least fix this bug, but right now I don't have time. Ehsan, feel free to take this back if you have cycles and see if you can use comment 43 to come up with a fix so we can at least get the perf work you did in bug 157681 out to our users.
Assignee | ||
Comment 45•11 years ago
|
||
OK, I have a patch based on comment 43 which fixes both the reduced test case and lexus.com locally. Let's see what the try server thinks about it: https://tbpl.mozilla.org/?tree=Try&rev=487d183d9b40
Assignee | ||
Comment 46•11 years ago
|
||
Comment on attachment 678778 [details] [diff] [review] Patch (v1) The try server was quite happy with this!
Attachment #678778 -
Attachment description: WIP → Patch (v1)
Attachment #678778 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Assignee: jwatt → ehsan
Comment 47•11 years ago
|
||
Try run for 487d183d9b40 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=487d183d9b40 Results (out of 20 total builds): success: 20 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-487d183d9b40
Comment 48•11 years ago
|
||
Try run for 487d183d9b40 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=487d183d9b40 Results (out of 21 total builds): success: 20 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-487d183d9b40
Comment 49•11 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #44) > On IRC dbaron also noted that UpdateOverflow seems to fail to account for > other things that are accounted for in overflow areas during reflow, such as > not accounting for bullets, pushed floats, abs pos elements, overflow > containers, and the bottom-edge-of-children. I've yet to find time to dig > into that and understand the reflow code sufficiently to account for those > things in UpdateOverflow though. Maybe I should have just fixed the issue > detailed in the last paragraph of comment 43 to at least fix this bug, but > right now I don't have time. Actually, I was wrong here; it's actually the base class's UpdateOverflow method that does the actual updating of the overflow, and that's generic and handles all thes things correctly, except for the bottom-edge-of-children, which I guess we've sort of decided not to worry about. The nsBlockFrame::UpdateOverflow override only exists to update cached information that will cause us to get things wrong after the next reflow, so it only needs to update the line box's cached overflow areas correctly.
Comment 50•11 years ago
|
||
Comment on attachment 678778 [details] [diff] [review] Patch (v1) (#developers) [2012-11-07 14:03:53] <dbaron> ehsan, so, actually, now that I'm looking at it, I think it's not quite right [2012-11-07 14:04:11] <ehsan> dbaron: I'm not surprised :) How so? [2012-11-07 14:04:19] <dbaron> ehsan, so are you familiar with what pushed floats are? [2012-11-07 14:04:31] <ehsan> dbaron: not really [2012-11-07 14:04:41] <ehsan> dbaron: (I mostly tried to copy what the reflow path does) [2012-11-07 14:05:15] <dbaron> ehsan, so, basically, what's supposed to happen is that the floats anchored in a particular line contribute to that line's overflow areas [2012-11-07 14:05:31] <dbaron> ehsan, (we clear out mFloatOverflowAreas for each successive line in DoReflowInlineFrames) [2012-11-07 14:06:08] <dbaron> ehsan, pushed floats, on the other hand, are funny -- they're what happens when a float gets pushed entirely to the next column/page or gets split across multiple columns/pages [2012-11-07 14:06:23] <dbaron> ehsan, so what your patch is doing is accumulating the overflow area of the pushed floats (which are rare) [2012-11-07 14:06:37] <dbaron> ehsan, and then adding it to the overflow area of any line that has floats anchored in it [2012-11-07 14:06:51] <dbaron> ehsan, though I think with the wrong offset [2012-11-07 14:07:11] <ehsan> hmm [2012-11-07 14:07:40] <ehsan> dbaron: are pushed floats the stuff in mFloats? [2012-11-07 14:07:48] <dbaron> ehsan, they're a subset of it [2012-11-07 14:07:56] <ehsan> oh [2012-11-07 14:08:00] <ehsan> the flag check! [2012-11-07 14:08:06] <ehsan> ok [2012-11-07 14:08:19] <dbaron> ehsan, and they're always at the start, which allows the flag check to be done that way [2012-11-07 14:08:25] <ehsan> dbaron: so let's say I inverted the flag check, would that address half of your concern? [2012-11-07 14:08:49] <dbaron> ehsan, no [2012-11-07 14:08:52] <ehsan> hmm [2012-11-07 14:08:59] <ehsan> wait [2012-11-07 14:09:02] <dbaron> ehsan, you need to accumulate the overflow area of the pushed floats, but you want to add that directly to the block rather than to a line [2012-11-07 14:09:08] <ehsan> so I guess I need to get the floats from the line boxes [2012-11-07 14:09:17] <dbaron> ehsan, but you also need to accumulate the other floats that are in lines and add them to the line they're in [2012-11-07 14:09:27] <ehsan> I see [2012-11-07 14:10:39] <ehsan> hmm [2012-11-07 14:10:47] <dbaron> ehsan, and I sort of suspect the line's overflow area is relative to the line origin [2012-11-07 14:10:51] <dbaron> ehsan, actually, wait a sec [2012-11-07 14:10:52] <ehsan> dbaron: how do I add an overflow area to the block itself? [2012-11-07 14:10:59] <dbaron> ehsan, you don't [2012-11-07 14:11:06] <dbaron> ehsan, that's what I just remembered [2012-11-07 14:11:27] <dbaron> ehsan, so the tricking thing about nsBlockFrame::UpdateOverflow is that actually the base class does all the work [2012-11-07 14:11:40] <ehsan> yeah [2012-11-07 14:11:46] <dbaron> ehsan, the only reason for the line-updating code is to make things correct for when a later reflow updates the overflow based on the cached stuff for the lines [2012-11-07 14:12:03] <dbaron> ehsan, so actually all that needs to get fixed is updating the lines correctly [2012-11-07 14:12:21] <ehsan> ok good [2012-11-07 14:12:30] <ehsan> dbaron: now, how do I get the floats for a linebox? [2012-11-07 14:13:11] <dbaron> ehsan, nsLineBox::GetFirstFloat [2012-11-07 14:13:32] <ehsan> dbaron: cool... let me see if I can get this right... [2012-11-07 14:13:36] <ehsan> dbaron: give me a few mins [2012-11-07 14:15:36] <ehsan> dbaron: how should I know what the origin of the linebox floats are? [2012-11-07 14:15:54] <dbaron> ehsan, so the origin of nsIFrame::GetRect() is always the frame's parent [2012-11-07 14:16:02] <dbaron> ehsan, the question is what coordinate space you want the line box's overflow area in [2012-11-07 14:16:23] <dbaron> ehsan, I suspect it's relative to the line box's top-left so that the line box can be slid around without affecting it, but that needs to be checked by code inspection
Attachment #678778 -
Flags: review?(dbaron) → review-
Assignee | ||
Comment 51•11 years ago
|
||
Attachment #678778 -
Attachment is obsolete: true
Attachment #679378 -
Flags: review?(dbaron)
Comment 52•11 years ago
|
||
(In reply to David Baron [:dbaron] from comment #50) > [2012-11-07 14:16:02] <dbaron> ehsan, the question is what coordinate space > you want the line box's overflow area in > [2012-11-07 14:16:23] <dbaron> ehsan, I suspect it's relative to the line > box's top-left so that the line box can be slid around without affecting it, > but that needs to be checked by code inspection Turns out I was wrong here, and it's relative to the block. This is clear from: * nothing funny happening to nsBlockReflowState::mFloatOverflowAreas * the correction being done in nsLineBox::SlideBy
Comment 53•11 years ago
|
||
Should this have tests that catch the issues raised by the first patch?
Assignee | ||
Comment 54•11 years ago
|
||
(In reply to comment #53) > Should this have tests that catch the issues raised by the first patch? I don't necessarily know how to write those tests, and I can't really spend time on doing that right now. :( The only reason I picked this up was to make sure that I won't have to spend time backing out the offending patch from Gecko 19 too.
Comment 55•11 years ago
|
||
Comment on attachment 679378 [details] [diff] [review] Patch (v2) This looks unnecessarily complicated, but otherwise fine. I don't see why there's any need for floatOverflowAreas as a temporary, or for unioning the temporay into lineAreas one piece at a time. You should be able to add only: if (line->HasFloats()) { for (nsFloatCache* fc = line->GetFirstFloat(); fc; fc = fc->Next()) { ConsiderChildOverflow(lineAreas, fc->mFloat); } } r=dbaron with that
Attachment #679378 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 56•11 years ago
|
||
Right, will do that before landing. Thanks!
Assignee | ||
Comment 57•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/539bb6ca633a
Comment 58•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/539bb6ca633a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 59•11 years ago
|
||
Verified fixed on the latest beta, Firefox 18 beta 3. User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 Build ID: 20121205060959
QA Contact: manuela.muntean
You need to log in
before you can comment on or make changes to this bug.
Description
•