396869 - [linux]switch menu causes a spurious mouseout event to mouse over widget

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Ginn Chen]

Steps:
1) Move mouse pointer over View->Toolbar, submenu popup.
2) Move mouse pointer down to "Status Bar" slowly
You will see "Status Bar" menuitem is selected, then unselected.
Move mouse pointer a bit, it is selected.

I can't reproduce this with Firefox 2.
I can't reproduce this with Firefox 3 on Windows, either.

I guess it's the same cause of Bug 387990.
But I reproduced Bug 387990 with Firefox 2 and Mozilla 1.7 on Solaris 10 Sparc.

Comment 1

[Ginn Chen]

Attachment: patch

Comment 2

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

There's a problem with this. For example, suppose we have a testcase like this:

<div style="overflow:auto; width:400px; height:400px;"></div>
<div style="margin-top:-400px; width:300px; height:400px; background:yellow;" onmouseout="alert('bang')"></div>

When the mouse is over the second div and then moves out of it to the right, I'm guessing the alert won't fire with your patch. Because the mouse event is delivered by the widget for the first div which is not associated with the second div.

Comment 3

[Ginn Chen]

Comment on attachment 281636 [details] [diff] [review]
patch

roc, you're right.

Sorry, I missed your comment.

Comment 4

[Ginn Chen]

Attachment: patch v2

check parents as well,
solved the case in comment #2.

Comment 5

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

That won't work either.

<div onmouseout="alert('bang')" style="overflow:auto; width:400px; height:400px; position:relative; z-index:1;"></div>
<div style="margin-top:-400px; width:300px; height:400px; background:yellow; overflow:auto"></div>

Now the same problem happens. The mouse is over the first DIV but events are associated with the second DIV's widget.

Basically the relationship between elements and widgets is not useful or reliable.

Comment 6

[Ginn Chen]

roc,

I didn't notice any different for your testcase, w/ or w/o my patch.

I think
mLastMouseOverFrame->GetWindow() is the window contains both <div>s.
aEvent->widget should be a descendant of the window.
otherwise we don't need that MouseOut event.

Comment 7

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

> mLastMouseOverFrame->GetWindow() is the window contains both <div>s.

No, in this case (or in some testcase, anyway), it will be the widget for the second DIV.

I'm not sure how I can explain this well. Basically there is no reliable relationship between the frame the mouse is over and the widget the mouse is over. The only thing that might work is to find the top-level widget for the frame and the top-level widget for the event and check if they're the same. Here "top-level" would mean nsIWidget->GetParent() returns null.

Comment 8

[Ginn Chen]

(In reply to comment #7)
> > mLastMouseOverFrame->GetWindow() is the window contains both <div>s.
> 
> No, in this case (or in some testcase, anyway), it will be the widget for the
> second DIV.
> 
> I'm not sure how I can explain this well. Basically there is no reliable
> relationship between the frame the mouse is over and the widget the mouse is
> over. 

I guess so. But I just don't know what's the case.

>The only thing that might work is to find the top-level widget for the
> frame and the top-level widget for the event and check if they're the same.
> Here "top-level" would mean nsIWidget->GetParent() returns null.
> 

I thought mLastMouseOverFrame->GetWindow() is top enough.
But your idea is much safer, I think.

Comment 9

[Ginn Chen]

Attachment: patch v3

Comment 10

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

Can you factor out the loop to find the top-level widget, and make it a helper function in nsContentUtils? We probably have similar loops elsewhere and they should be consolidated. Call it GetTopLevelWidget or something like that.

Comment 11

[Ginn Chen]

It's not only a problem of menuitem unselect.

Take the testcase in comment #2.
Move mouse over the yellow area.
Now press Alt+F, and then press right arrow.
"bang"

Firefox doesn't have this issue.

Comment 12

[Ginn Chen]

(In reply to comment #11)
> Firefox doesn't have this issue.
oops, Firefox 2 doesn't have this issue. 

Comment 13

[Ginn Chen]

Attachment: patch v4

Comment 14

[Reed Loden [:reed]]

I see this all the time, and it's very annoying. Nice to know there's a patch to fix it. :)

Comment 15

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

+  /**
+   * Return top-level widget in the parent chain.
+   */
+  static nsIWidget* GetTopLevelWidget(nsIWidget* aWidget)
+  {

Don't make this inline. And don't include nsIWidget.h in nsContentUtils.h; with the function not inline, you can just declare 'class nsIWidget;'. Then you don't need to add gfx to those makefiles. Although I'm not sure why you had to add gfx, I thought you'd need to add widget.

+    if (!aWidget) {
+      return nsnull;
+    }
+
+    nsIWidget* currWidget = aWidget;
+    nsIWidget* parentWidget;
+    while (parentWidget = currWidget->GetParent()) {

Make this (parentWidget = currWidget->GetParent()) != nsnull to be clear.

Can you really call nsContentUtils from toolkit/components/remote? In a non-static build?

Comment 16

[Ginn Chen]

(In reply to comment #15)
> Although I'm not sure why you had to
> add gfx, I thought you'd need to add widget.
Because nsIWidget.h requires gfx

> Can you really call nsContentUtils from toolkit/components/remote? In a
> non-static build?
No, if it's not inline.
 

Comment 17

[Ginn Chen]

Attachment: patch v5