513684 - Hovering over drop down popup can get content underneath in a hovered state

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, between 2009-07-20 and 2009-07-22:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-07-20+04%3A00%3A00&enddate=2009-07-22+06%3A00%3A00
I think a regression from bug 352093.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Interestingly if you scroll the dropdown then the content underneath does not hover until you move off of it and back on again.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Regressing changeset is http://hg.mozilla.org/mozilla-central/rev/dfa7ab1fe97c

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So it seems this bug and Bug 514026 are caused by using GetRootView instead of GetViewFor.  I don't understand Bug 352093 well enough to know using GetViewFor is simply undoing what was intended to be done there.  Comment 9 on that bug seems to indicate that calling GetViewFor was seen as unnecessary but not something that needed to be removed.  If my understanding is correct, the fix for this is pretty straightforward.

Comment 4

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

You're probably right, but where exactly do you think we should be using GetViewFor instead of GetRootView?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah, woops, that is an important detail.  PresShell::DispatchSynthMouseMove is where the change would be made.

Comment 6

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

Ah yes, that makes sense.

Comment 7

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

Kyle, were you planning to patch this, or should I work on it?

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes I was, I just won't get around to it till the weekend.

Comment 9

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

Awesome, thanks!

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Proposed patch w/test

Switch back to using GetViewFor instead of GetRootView to ensure that hover events end up where they are supposed to.

The test is a little odd because synthesizeMouse does funny things.  Immediately checking the style after calling synthesizeMouse gives you the hovered style before, during, and after the regression, while waiting a few milliseconds catches only the regression.  This only happens with synthesizeMouse, not with an actual mouse.  This is why all of the checks are wrapped in a setTimeout call.

Comment 11

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

The code looks good.

+<div id="underdiv" style="height: 700px; width=200px">

incorrect CSS syntax

> Immediately checking the style after calling synthesizeMouse gives you the
> hovered style before, during, and after the regression

Hmm. I'd rather not have this test if it depends on inexplicable timeouts, that's usually a recipe for sporadic test failures.

One thing you may not have realized is that DispatchSynthMouseMove is not dealing with events generated by synthesizeMouse --- synthesizeMouse generates "real" mouse events. DispatchSynthMouseMove is dealing with mouse events we synthesized because reflow happened and we want to make sure that if an element moved or appeared or disappeared then it gets appropriate mouseenter/exit events.

What may be happening here is that the dropdown doesn't appear instantly so without timeouts your mouse moves get fired while the dropdown is not showing. nsComboboxControlFrame::ShowPopup fires a popupshowing event so try listening it before you start your test?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #11)
> The code looks good.
> 
> +<div id="underdiv" style="height: 700px; width=200px">
> 
> incorrect CSS syntax

Noted.

> > Immediately checking the style after calling synthesizeMouse gives you the
> > hovered style before, during, and after the regression
> 
> Hmm. I'd rather not have this test if it depends on inexplicable timeouts,
> that's usually a recipe for sporadic test failures.
> 
> One thing you may not have realized is that DispatchSynthMouseMove is not
> dealing with events generated by synthesizeMouse --- synthesizeMouse generates
> "real" mouse events. DispatchSynthMouseMove is dealing with mouse events we
> synthesized because reflow happened and we want to make sure that if an element
> moved or appeared or disappeared then it gets appropriate mouseenter/exit
> events.

Yes I was not aware of that.  Certainly explains why they are called synthetic mouse events then.  That this patch fixes this bug seems to imply that reflow is happening everytime the mouse is moved.  I know very little about how layout works internally but that seems undesirable.
 
> What may be happening here is that the dropdown doesn't appear instantly so
> without timeouts your mouse moves get fired while the dropdown is not showing.
> nsComboboxControlFrame::ShowPopup fires a popupshowing event so try listening
> it before you start your test?

That does appear to be the case, in a way.  If I place a timeout only on the first move but not on the subsequent ones the mouse moves still appear to get fired while the dropdown is not showing.  That seems to me to be consistent with reflowing everytime the mouse moves.  What is weird is that when using a regular mouse I don't see this behavior in the test at all.

I will try the popupshowing event later.  Do you want me to go ahead and just post the actual patch by itself?

Comment 13

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

(In reply to comment #12)
> I will try the popupshowing event later.  Do you want me to go ahead and just
> post the actual patch by itself?

Yes please.

I'm not sure why we're firing synthetic mouse events here, it would be nice to know...

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

As before, but sans test.

Comment 15

[Nochum Sossonko [:Natch]]

Comment on attachment 400421 [details] [diff] [review]
Patch

Doesn't need approval since it's a blocker.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't have commit access so I can't check this in.  I asked if anyone was willing to check this in on IRC yesterday but it seems nobody got around to it (tree being closed part of the day probably didn't help).  If anyone reading this wants to check this in that would be awesome :-)

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a2d46929bcbd

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #19)
> http://hg.mozilla.org/mozilla-central/rev/a2d46929bcbd

Thanks Dao!

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I pushed this to try a few hours ago, and the mac unit test build showed:

7917 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_native_mouse_mac.xul | didn't receive expected event: {"type":"mouseout","target":"panel","screenX":387,"screenY":170}
7918 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_native_mouse_mac.xul | didn't receive expected event: {"type":"mouseover","target":"window","screenX":387,"screenY":170}
7921 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_native_mouse_mac.xul | 51 | wrong event type for event {"type":"mousemove","target":"panel","screenX":387,"screenY":171} - got "mousemove", expected "mouseout"
7922 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_native_mouse_mac.xul | 51 | wrong target for event {"type":"mousemove","target":"panel","screenX":387,"screenY":171} - got [object XULElement], expected [object XULElement]
7924 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_native_mouse_mac.xul | didn't receive expected event: {"type":"mouseover","target":"panel","screenX":387,"screenY":171}
7925 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_native_mouse_mac.xul | didn't receive expected event: {"type":"mousemove","target":"panel","screenX":387,"screenY":171}


It looks like the same errors just came up on mozilla-central; I think dao is backing it out.

Comment 22

[Dão Gottwald [:dao]]

backed out

Comment 23

[Markus Stange [:mstange]]

This is my test, I'll look into it.

Comment 24

[Markus Stange [:mstange]]

Attachment: remove known failures from the test

The test failures were of the "unexpected pass" kind, the test just doesn't detect that properly.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Pushed as http://hg.mozilla.org/mozilla-central/rev/9ed976f3d466 (last 2 patches combined).

Comment 26

[Markus Stange [:mstange]]

This makes it in‑testsuite+ on Mac.

Comment 27

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4788f1c39085