onmouseover, javascript alert shows twice

VERIFIED FIXED

Status

()

VERIFIED FIXED
13 years ago
a year ago

People

(Reporter: bugzilla, Assigned: emaijala+moz)

Tracking

(Depends on: 1 bug, {regression, testcase, verified1.8})

Trunk
x86
Windows XP
regression, testcase, verified1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b3 -
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [no l10n impact])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 186111 [details]
Testcase
(Reporter)

Comment 2

13 years ago
Created attachment 186113 [details]
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
Created attachment 186120 [details]
replacement testcase 1
Attachment #186111 - Attachment is obsolete: true
Summary: onmouseover showig alert shows it twice → onmouseover , javascript alert shows twice
(Reporter)

Comment 4

13 years ago
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
Possible; there is weird focus-type stuff going on when modal windows are
involved, and it could be confusing the ESM....
Flags: blocking1.8b3?
doesn't seem to fail on Linux/GTK2. Can you confirm that this is Windows only?
(Reporter)

Comment 8

13 years ago
(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? 
(Reporter)

Comment 9

13 years ago
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.

Updated

13 years ago
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking1.8b3-
OK, this works on Linux/GTK1 too.  Ccing some win32 folks for help.
(Assignee)

Comment 11

13 years ago
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..
Blocks: 291507
(Reporter)

Updated

13 years ago
Blocks: 300057

Comment 12

13 years ago
(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.

Updated

13 years ago
Whiteboard: [no l10n impact]
(Assignee)

Comment 13

13 years ago
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?
(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.
Created attachment 189871 [details] [diff] [review]
patch

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

Updated

13 years ago
Flags: blocking1.8b4? → blocking1.8b4+

Comment 16

13 years ago
Comment on attachment 189871 [details] [diff] [review]
patch

johnny, can you review this for us?
Attachment #189871 - Flags: review?(jst)
(Assignee)

Comment 17

13 years ago
Created attachment 190594 [details] [diff] [review]
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?
Attachment #190594 - Flags: review?(roc)

Updated

13 years ago
Assignee: events → emaijala
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.
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.
(Assignee)

Comment 20

13 years ago
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 on attachment 190594 [details] [diff] [review]
Win32 patch

I think this should be done differently.
Attachment #190594 - Flags: review?(roc)
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.
(Assignee)

Comment 23

13 years ago
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?
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 :-)
(Assignee)

Comment 25

13 years ago
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).
(Assignee)

Comment 26

13 years ago
Wow, that one's been there since rev 1.1. Dare I propose it be removed?
Whiteboard: [no l10n impact] → [no l10n impact] Almost there..
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.
(Assignee)

Updated

13 years ago
Attachment #189871 - Flags: review?(jst)
(Assignee)

Comment 28

13 years ago
Created attachment 192008 [details] [diff] [review]
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.
Attachment #189871 - Attachment is obsolete: true
Attachment #190594 - Attachment is obsolete: true
Attachment #192008 - Flags: superreview?(roc)
Comment on attachment 192008 [details] [diff] [review]
Win32 patch v2

this looks like the right thing. Thanks!!!
Attachment #192008 - Flags: superreview?(roc)
Attachment #192008 - Flags: superreview+
Attachment #192008 - Flags: review+
(Assignee)

Updated

13 years ago
Attachment #192008 - Flags: approval1.8b4?
I've tested the patch, and I think it also fixes bug 297080
Blocks: 297080
(Assignee)

Updated

13 years ago
Whiteboard: [no l10n impact] Almost there.. → [no l10n impact] Seeking approval
(Assignee)

Updated

13 years ago
Blocks: 300297
(Assignee)

Comment 31

13 years ago
Checked in to trunk, still waiting for approval for branch.
Status: NEW → ASSIGNED
I think this caused bug 304680.

Comment 33

13 years ago
Ere, can you look into this regression?
(Assignee)

Comment 34

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

Comment 36

13 years ago
approval1.8b4 pending clearing up any possible regressions caused by this patch
as seen on the trunk
(Assignee)

Comment 37

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

Updated

13 years ago
Attachment #192008 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 38

13 years ago
Fix checked in to branch too.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: [no l10n impact] Seeking approval → [no l10n impact]
(Reporter)

Comment 39

13 years ago
Verified fixed with branch build 20050825
Status: RESOLVED → VERIFIED
(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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
Keywords: fixed1.8 → verified1.8
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.
Status: RESOLVED → VERIFIED
Summary: onmouseover , javascript alert shows twice → onmouseover, javascript alert shows twice
might have caused bug 307563
Depends on: 310778
Depends on: 324149
You need to log in before you can comment on or make changes to this bug.