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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: khuey)

Tracking

({regression, testcase})

Trunk
mozilla1.9.3a1
x86
Windows XP
regression, testcase
Points:
---
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 397619 [details]
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.
Assignee: nobody → roc
Flags: blocking1.9.2?
Interestingly if you scroll the dropdown then the content underneath does not hover until you move off of it and back on again.
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.
You're probably right, but where exactly do you think we should be using GetViewFor instead of GetRootView?
Ah, woops, that is an important detail.  PresShell::DispatchSynthMouseMove is where the change would be made.
Kyle, were you planning to patch this, or should I work on it?
Yes I was, I just won't get around to it till the weekend.
Awesome, thanks!
Assignee: roc → me
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Created attachment 400296 [details] [diff] [review]
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.
Attachment #400296 - Flags: review?(roc)
Status: NEW → ASSIGNED
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?
(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?
(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...
Created attachment 400421 [details] [diff] [review]
Patch

As before, but sans test.
Attachment #400296 - Attachment is obsolete: true
Attachment #400421 - Flags: review?(roc)
Attachment #400421 - Flags: approval1.9.2?
Attachment #400296 - Flags: review?(roc)
Comment on attachment 400421 [details] [diff] [review]
Patch

Doesn't need approval since it's a blocker.
Attachment #400421 - Flags: approval1.9.2?
Duplicate of this bug: 514901
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 :-)
Flags: in-testsuite?
http://hg.mozilla.org/mozilla-central/rev/a2d46929bcbd
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(In reply to comment #19)
> http://hg.mozilla.org/mozilla-central/rev/a2d46929bcbd

Thanks Dao!
Keywords: checkin-needed
Whiteboard: [needs landing] → [needs 1.9.2 landing]
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.
backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs 1.9.2 landing]
This is my test, I'll look into it.
Created attachment 403066 [details] [diff] [review]
remove known failures from the test

The test failures were of the "unexpected pass" kind, the test just doesn't detect that properly.
Keywords: checkin-needed
Pushed as http://hg.mozilla.org/mozilla-central/rev/9ed976f3d466 (last 2 patches combined).
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
This makes it in‑testsuite+ on Mac.
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Whiteboard: [needs 192 landing]
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4788f1c39085
status1.9.2: --- → beta1-fixed
Keywords: checkin-needed
Whiteboard: [needs 192 landing]
You need to log in before you can comment on or make changes to this bug.