297561 - onmouseover, javascript alert shows twice

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[José Jeria]

Build id: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050613 Firefox/1.0+

Steps to reproduce:
1. load testcase
2. hover text, it should show an alert
3. Note that after pressing again, the alert comes up again

This works fine in Firefox 1.0.x, so it seems like a regression. Will try to
find out when this happened.

Comment 1

[José Jeria]

Attachment: Testcase

Comment 2

[José Jeria]

Attachment: Testcase showing that onmouseover is only triggered once

Martijn Wargers pointed out in the mozillazine forums that the event is
triggered only once, so it seems to be specific to alert dialogs

Comment 3

[Peter van der Woude [:Peter6]]

Attachment: replacement testcase 1

Comment 4

[José Jeria]

Had fun looking in the archives and found that it works in:
2005-03-28-08

but is broken in:
2005-03-29-07

There are build in between to test, but not for windows that i use

Comment 5

[Peter van der Woude [:Peter6]]

works 20050328-08 build
fails 20050329-07 build

regressionwindow
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-03-28+07%3A00%3A00&maxdate=2005-03-29+07%3A00%3A00&cvsroot=%2Fcvsroot

bug 284664 ?

cc bzbarsky / roc

Comment 6

[Boris Zbarsky [:bzbarsky]]

Possible; there is weird focus-type stuff going on when modal windows are
involved, and it could be confusing the ESM....

Comment 7

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

doesn't seem to fail on Linux/GTK2. Can you confirm that this is Windows only?

Comment 8

[José Jeria]

(In reply to comment #7)
> doesn't seem to fail on Linux/GTK2. Can you confirm that this is Windows only?

Just tested this on Mac OS X 10.3.9, build 20050621. 

The alert is shown once, but once you dismiss it, the window will not respond
any more, hovering the div will not cause an alert to appear, pressing the
reload button does nothing etc. This is really a blocker issue on mac, should I
file a new bug for that or does this bug cover that as well? 

Comment 9

[José Jeria]

To clarify my last comment. Firefox doesn't freeze. The menues still work, so I
dont need to kill Firefox, if I open a new window via the file menu, it will
work just fine. But the window that the testcase was run on, has to be closed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, this works on Linux/GTK1 too.  Ccing some win32 folks for help.

Comment 11

[Ere Maijala (slow)]

On Win32 a mouse exit event is posted by the mouse trailer right after the
dialog is displayed. nsEventStateManager notes the cursor is still inside the
view bounds and converts it to a mouse move event. This, I believe, triggers
another dialog. Maybe the mouse trailer needs tweaking..

Comment 12

[[e-mail]]

(In reply to comment #10)
>OK, this works on Linux/GTK1 too.  Ccing some win32 folks for help.
Actually, on GTK1, if I hover the yellow div, and then move the alert to
partially overlap the yellow div, then mouse over the yellow div and then
directly over the alert then a further alert is triggered. Subsequent mousing
between the yellow div and the first alert opens two further alerts each time.

Comment 13

[Ere Maijala (slow)]

Hmm.. I tried disabling mouse trailer but for some reason everything went
haywire (dozens of dialogs...). Anyway, shouldn't we prevent onmouseover from
firing at all at least when a modal dialog is open?

Comment 14

[Martijn Wargers (dead)]

(In reply to comment #13)
> Anyway, shouldn't we prevent onmouseover from firing at all at least when a
modal dialog is open?
Yes, at least, that is what IE6 is doing.

Comment 15

[Martijn Wargers (dead)]

Attachment: patch

It seems to me the alert window wasn't a modal dialog at all.
This doesn't seem to fix the bug, though.

Comment 16

[Asa Dotzler [:asa]]

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

johnny, can you review this for us?

Comment 17

[Ere Maijala (slow)]

Attachment: Win32 patch

This is the best I can do without poking nsEventStateManager and others I'm
afraid to touch. This will just fake the position of the NS_MOUSE_EXIT message
sent by the mouse trailer so that nsEventStateManager won't convert it to
NS_MOUSE_MOVE. Ok?

Comment 18

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

Can you tell me what widgets are involved here? In bug 297080 I made it so that
MOUSE_EXIT events from a toplevel window are always treated as exits.

Comment 19

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

I think the mouse trailer should only watch top-level windows ... if that was
done right, I think the patch in bug 297080 would make this unnecessary.

Comment 20

[Ere Maijala (slow)]

Well, I can try, but last time I tested just disabling the mouse trailer, it
went completely haywire. I've spent quite some time trying to  find the reason
for this behavior, but these are just a pain to debug..

Comment 21

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

Comment on attachment 190594 [details] [diff] [review]
Win32 patch

I think this should be done differently.

Comment 22

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

Basically, I think mCaptureWindow and mMouseHoldWindow should always be
top-level windows only. MouseTrailer can find the top-level window in
SetMouseTrailerWindow, and I guess mCaptureWindow wherever that gets set.

Comment 23

[Ere Maijala (slow)]

I tried this approach, but for some reason I couldn't quite get hold on the test
case starts creating dialogs like crazy when the mouse cursor is moved to the
first dialog after hovering the text. It stops when I manage to close all the
dialogs. At the same time this is displayed in the console:

WARNING: event queue chain length is 13. this is almost certainly a leak., file
c:/mozilla_source/mozilla/xpcom/threads/nsEventQueue.cpp, line 553

The chain length is incremented for every dialog created. roc, any ideas?

Comment 24

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

That error is related to 303484.

Maybe what's happening is that the mouseover fired by the mouse trailer is
creating a new alert window which reenters the mouse trailer before you've
updated it to indicate the mouse has left the main window?

See also Dainis' window-parenting issue in 300297 ... don't accidentally
traverse the ownership chain :-)

Comment 25

[Ere Maijala (slow)]

Ok, I found the culprit. When we get the first move event for the dialog,
nsWindow::DispatchMouseEvent heplfully dispatches NS_MOUSE_EXIT for the previous
window at the point of last mouse message in the previous window. This happens
to be on top of the item that fired the dialog in the first place. Then comes
nsEventStateManager that even more helpfully converts the NS_MOUSE_EXIT to
NS_MOUSE_MOVE and causes another dialog to be fired by the onmouseover event.
This is repeated for every move event because the dialog we're on top is a new
one all the time.. Now, I can't see the reason why the exit should be fired for
the previous window. As far as I can see, mouse trailer takes care of that
(unless I've got myself totally confused).

Comment 26

[Ere Maijala (slow)]

Wow, that one's been there since rev 1.1. Dare I propose it be removed?

Comment 27

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

When it dispatches the MOUSE_EXIT, it also calls
   MouseTrailer::GetSingleton().IgnoreNextCycle();
so you'd need to remove that too, right?

In the situation you describe, I'd expect the fix in bug 297080 to ensure the
MOUSE_EXIT was *not* converted to a MOUSE_MOVE. Is it because gCurrentWindow is
an inner window, not a top-level window?

Maybe the thing to do is make sure the mouse trailer only deals with toplevel
windows, as I described, and then use it here instead of firing the MOUSE_EXIT
from DispatchMouseEvent.

Comment 28

[Ere Maijala (slow)]

Attachment: Win32 patch v2

Ok, this changes mouse trailer to only deal with top level windows and changes
the mouse exit generation in nsWindow to use the actual mouse location instead
of the last known location in the previous window. I've borrowed and adapted
some code by Dainis from bug 300297.

Comment 29

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

Comment on attachment 192008 [details] [diff] [review]
Win32 patch v2

this looks like the right thing. Thanks!!!

Comment 30

[Martijn Wargers (dead)]

I've tested the patch, and I think it also fixes bug 297080

Comment 31

[Ere Maijala (slow)]

Checked in to trunk, still waiting for approval for branch.

Comment 32

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

I think this caused bug 304680.

Comment 33

[Asa Dotzler [:asa]]

Ere, can you look into this regression?

Comment 34

[Ere Maijala (slow)]

I have and it doesn't seem related. I've commented in bug 304680. 

Comment 35

[Boris Zbarsky [:bzbarsky]]

This might have caused bug 304955

Comment 36

[Chris Beard]

approval1.8b4 pending clearing up any possible regressions caused by this patch
as seen on the trunk

Comment 37

[Ere Maijala (slow)]

Patch for bug 304955 waiting for approval. No other known regressions (I don't
believe 304680 is really related).

Comment 38

[Ere Maijala (slow)]

Fix checked in to branch too.

Comment 39

[José Jeria]

Verified fixed with branch build 20050825

Comment 40

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

(In reply to comment #39)
> Verified fixed with branch build 20050825

Verification for branch is the 'verified1.8' keyword.  Status resolution itself
is for the trunk only.

Comment 41

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

Verified FIXED using:

https://bugzilla.mozilla.org/attachment.cgi?id=186120
-and-
https://bugzilla.mozilla.org/attachment.cgi?id=186113

on Windows XP SeaMonkey trunk build 2005-09-01-05.

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

might have caused bug 307563