Closed
Bug 297561
Opened 20 years ago
Closed 19 years ago
onmouseover, javascript alert shows twice
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: bugzilla, Assigned: emaijala+moz)
References
Details
(Keywords: regression, testcase, verified1.8, Whiteboard: [no l10n impact])
Attachments
(3 files, 3 obsolete files)
405 bytes,
text/html
|
Details | |
394 bytes,
text/html
|
Details | |
6.14 KB,
patch
|
roc
:
review+
roc
:
superreview+
cbeard
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
Reporter | ||
Comment 2•20 years ago
|
||
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•20 years ago
|
||
Attachment #186111 -
Attachment is obsolete: true
Updated•20 years ago
|
Summary: onmouseover showig alert shows it twice → onmouseover , javascript alert shows twice
Reporter | ||
Comment 4•20 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
Comment 5•20 years ago
|
||
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•20 years ago
|
||
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•20 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•20 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•20 years ago
|
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Comment 10•20 years ago
|
||
OK, this works on Linux/GTK1 too. Ccing some win32 folks for help.
Assignee | ||
Comment 11•20 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..
Comment 12•20 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•20 years ago
|
Whiteboard: [no l10n impact]
Assignee | ||
Comment 13•20 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?
Comment 14•20 years ago
|
||
(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•20 years ago
|
||
It seems to me the alert window wasn't a modal dialog at all.
This doesn't seem to fix the bug, though.
Updated•20 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Comment 16•20 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•19 years ago
|
||
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•19 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•19 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•19 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•19 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•19 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•19 years ago
|
Attachment #189871 -
Flags: review?(jst)
Assignee | ||
Comment 28•19 years ago
|
||
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•19 years ago
|
Attachment #192008 -
Flags: approval1.8b4?
Assignee | ||
Updated•19 years ago
|
Whiteboard: [no l10n impact] Almost there.. → [no l10n impact] Seeking approval
Assignee | ||
Comment 31•19 years ago
|
||
Checked in to trunk, still waiting for approval for branch.
Status: NEW → ASSIGNED
I think this caused bug 304680.
Comment 33•19 years ago
|
||
Ere, can you look into this regression?
Assignee | ||
Comment 34•19 years ago
|
||
I have and it doesn't seem related. I've commented in bug 304680.
Comment 35•19 years ago
|
||
This might have caused bug 304955
Comment 36•19 years ago
|
||
approval1.8b4 pending clearing up any possible regressions caused by this patch
as seen on the trunk
Assignee | ||
Comment 37•19 years ago
|
||
Patch for bug 304955 waiting for approval. No other known regressions (I don't
believe 304680 is really related).
Updated•19 years ago
|
Attachment #192008 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 38•19 years ago
|
||
Fix checked in to branch too.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: [no l10n impact] Seeking approval → [no l10n impact]
Reporter | ||
Comment 39•19 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 → ---
Updated•19 years ago
|
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
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
Comment 42•19 years ago
|
||
might have caused bug 307563
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•