Closed Bug 428680 Opened 16 years ago Closed 16 years ago

Sometimes, the document node is the event target for mousemove events now

Categories

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

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: martijn.martijn, Assigned: kinetik)

References

()

Details

(Keywords: regression, testcase)

Attachments

(2 files, 1 obsolete file)

Attached file testcase
See testcase, which shows the nodeName of the target node of the mousemove event at that moment. It will only display new nodeNames when the nodeName has changed.

In current trunk builds this will also show "#document" as result when you move from the document into the chrome area. 

This regressed between 2008-03-11 and 2008-03-13:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-03-11+04&maxdate=2008-03-13+09&cvsroot=%2Fcvsroot
I think a regression from bug 297080.

I think this is the cause of the js errors that can be seen on hi5.com. Example of js error:
Error: target.getAttribute is not a function
Source File: http://static.hi5.com/friend/js/photos/photo_tagging_1204594622.js
Line: 1
I think this is a dupe of bug 426630, but the patch I posted in that bug doesn't fix the problem shown in the testcase attached to this bug.

Taking.
Assignee: nobody → kinetik
The #document event target is showing up for mouse exit events that were previously treated as top-level exits by nsEventStateManager (before my patch from but #297080 was checked in) that we now treat as child-level exits.  After the exit is munged into a move in nsEventStateManager::PreHandleEvent, we dispatch it into the DOM at PresShell::HandleEventInternal step #2, but via the fall-through path using mDocument as the event target.
Attached patch patch v1 (obsolete) — Splinter Review
The cause of this bug is that we convert a mouse exit (indicating we have left the content area and moved into chrome) into a mouse move.  The event widget is the content area widget we have left, so the event is handled by the content view manager.  PresShell::HandlePositionedEvent is called with the target frame being the viewport.  Once we convert the event to a move inside nsEventStateManager::PreHandleEvent, the DOM event generation step (#2) in PresShell::HandleEventInternal results in the events targeting the top level document content (mDocument) because there is no content under the event location (since we have exited the area where anything could be targeted for this view manager).

What should a mouse exit that has been converted into a mouse move target? We have already moved outside the area where it could target anything sensible, I think.  So I propose that when we convert the exit into a move, we convert it into a synthesized move.  This prevents regular mouse move handling on the event, but still allows mouse out and mouse over handling to take place.

This patch fixes the testcase in this bug, and also fixes bug 426630 and bug 420804.

The change to the windows widget code is not necessary to fix any of these, but I noticed the top-level exit detection was incorrect when I initially debugged bug 426630 (and attached this chunk of the patch there)--the problem is we were treating exits into the window frame as a child exit.  This could mean we would mis a top-level exit if the pointer moved slowly into the window frame (generating a child exit) then out of the window (generating no further exit events).  There was also a subtle bug visible where the pointer would change to the resizer and back again within a single pixel move as we hit the inner edge of the window frame--this was caused by treating the exit as a child rather than a top-level exit.
Attachment #316905 - Flags: superreview?(roc)
Attachment #316905 - Flags: review?(roc)
+  HWND mouseWnd = ::WindowFromPoint(mp);
+  HWND mouseTopLevel = nsWindow::GetTopLevelHWND(mouseWnd);
+  if (mouseWnd == mouseTopLevel)
+    return PR_TRUE;

Is this correct? We return true if the window containing the point is a toplevel window. But what if the mouse moves from a child window into the toplevel window, won't this return true then?

I think the synthesized event approach makes sense here, but CCing Olli and Ere in case they have an opinion.
(In reply to comment #4)
> +  HWND mouseWnd = ::WindowFromPoint(mp);
> +  HWND mouseTopLevel = nsWindow::GetTopLevelHWND(mouseWnd);
> +  if (mouseWnd == mouseTopLevel)
> +    return PR_TRUE;
> 
> Is this correct? We return true if the window containing the point is a
> toplevel window. But what if the mouse moves from a child window into the
> toplevel window, won't this return true then?

It is, because GetTopLevelHWND (with a default aStopOnFirstTopLevel parameter of false) ends up returning the HWND for the window frame (which includes the non-client area).  So any exit into this window frame should be treated as a top level exit.  If you pass aStopOnFirstTopLevel = true, GetTopLevelHWND would return the HWND for the top level of the client area, if I understand correctly.
Comment on attachment 316905 [details] [diff] [review]
patch v1

OK, please add comments to that effect.
Attachment #316905 - Flags: superreview?(roc)
Attachment #316905 - Flags: superreview+
Attachment #316905 - Flags: review?(roc)
Attachment #316905 - Flags: review?(Olli.Pettay)
Attached patch patch v2Splinter Review
With comment.
Attachment #316905 - Attachment is obsolete: true
Attachment #316914 - Flags: review?(Olli.Pettay)
Attachment #316905 - Flags: review?(Olli.Pettay)
Attachment #316914 - Flags: review?(Olli.Pettay) → review+
Blocks: 426630
Blocks: 420804
Comment on attachment 316914 [details] [diff] [review]
patch v2

Requesting approval.  Fixes a nasty web content visible regression introduced by my patch for bug 297080, and also fixes bug 426630 and bug 420804.
Attachment #316914 - Flags: approval1.9?
Comment on attachment 316914 [details] [diff] [review]
patch v2

a1.9+=damons
Attachment #316914 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
mozilla/widget/src/windows/nsWindow.cpp 	3.741
mozilla/content/events/src/nsEventStateManager.cpp 	1.741
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: