775350 - Lexus.com navigational menu item expands and collapses, when it should open and remain open

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

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

[Alice0775 White]

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

Comment 2

[Martijn Wargers (dead)]

Attachment: unminimized testcase

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.

Comment 3

[(no longer active)]

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.

Comment 4

[(no longer active)]

Attachment: Minimized test case

Comment 5

[(no longer active)]

Attachment: Minimized test case

Attached the wrong file!

Comment 6

[(no longer active)]

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

[Timothy Nikkel (:tnikkel)]

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

[Boris Zbarsky [:bzbarsky]]

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()).

Comment 9

[(no longer active)]

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.

Comment 10

[(no longer active)]

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...

Comment 11

[(no longer active)]

(Oh, also Timothy demonstrated to me in person that my theory about views is incorrect.)

Comment 12

[Timothy Nikkel (:tnikkel)]

If the frame tree is the same then the hit testing should be the same so I would think that the problem is elsewhere.

Comment 13

[(no longer active)]

(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.

Comment 14

[(no longer active)]

Attachment: Frame tree diff (created using wdiff)

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?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes. I guess that's not working...

Comment 16

[(no longer active)]

Matt, do you know why the UpdateOverflow hint is not working correctly here?  Can you please take a look at this?  Thanks!

Comment 17

[Timothy Nikkel (:tnikkel)]

Mats might be a better person to ask.

Comment 18

[(no longer active)]

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!

Comment 19

[(no longer active)]

Firefox 16 will go to Beta today.  We need to backout bug 157681 from Beta since this wasn't fixed in time.  :(

Comment 20

[(no longer active)]

Attachment: Backout bug 157681

[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

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 655694 [details] [diff] [review]
Backout bug 157681

[Triage Comment]
Will prevent web regression in FF16 - approved for beta.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3d7907e9e5a9

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

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

[Alice0775 White]

Fixed in Firefox16beta only.
And I can reproduce in Aurora17.0a2 and Nightly18.0a1

Comment 25

[(no longer active)]

Comment on attachment 655694 [details] [diff] [review]
Backout bug 157681

Sigh, need to back out from Aurora as well...

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 655694 [details] [diff] [review]
Backout bug 157681

Thanks Ehsan, let's get this in before merge.

Comment 27

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ff2abbedea7

Comment 28

[(no longer active)]

The backout broke the build, so I backed it out: https://hg.mozilla.org/releases/mozilla-aurora/rev/37e18e9c9ca9

Comment 29

[(no longer active)]

Fixed and relanded: https://hg.mozilla.org/releases/mozilla-aurora/rev/151ad4f02509

Comment 30

[(no longer active)]

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?

Comment 31

[(no longer active)]

Specifically, could this be because of bug 614732?

Comment 32

[Jonathan Watt [:jwatt]]

(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.]

Comment 33

[(no longer active)]

(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

[Jonathan Watt [:jwatt]]

(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

[Daniel Holbert [:dholbert]]

(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.

Comment 36

[(no longer active)]

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

[Jonathan Watt [:jwatt]]

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.

Comment 38

[(no longer active)]

OK, great!  Could you please comment here when that happens so that I don't forget?

Thanks!

Comment 39

[Jonathan Watt [:jwatt]]

(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.

Comment 40

[(no longer active)]

Alright, let's see how this one goes:

https://hg.mozilla.org/releases/mozilla-beta/rev/5ae79f2880ff

Comment 41

[(no longer active)]

Backed out from Aurora as well: https://hg.mozilla.org/releases/mozilla-aurora/rev/cf470cfd18ca

Comment 42

[Jonathan Watt [:jwatt]]

Attachment: reduced testcase (rect should not disappear after 2 seconds)

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

[Jonathan Watt [:jwatt]]

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

[Jonathan Watt [:jwatt]]

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.

Comment 45

[(no longer active)]

Attachment: Patch (v1)

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

Comment 46

[(no longer active)]

Comment on attachment 678778 [details] [diff] [review]
Patch (v1)

The try server was quite happy with this!

Comment 47

[Mozilla RelEng Bot]

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/[e-mail]-487d183d9b40

Comment 48

[Mozilla RelEng Bot]

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/[e-mail]-487d183d9b40

Comment 49

[David Baron :dbaron:]

(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

[David Baron :dbaron:]

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

Comment 51

[(no longer active)]

Attachment: Patch (v2)

Comment 52

[David Baron :dbaron:]

(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

[Ryan VanderMeulen [:RyanVM]]

Should this have tests that catch the issues raised by the first patch?

Comment 54

[(no longer active)]

(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

[David Baron :dbaron:]

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

Comment 56

[(no longer active)]

Right, will do that before landing.  Thanks!

Comment 57

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/539bb6ca633a

Comment 58

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/539bb6ca633a

Comment 59

[Manuela Muntean [Away]]

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