297080 - Mouse cursor stays in pointer form, moving from content area to other window

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P1)

Description

[Martijn Wargers (dead)]

To reproduce this bug:
- Open testcase
- Open Javascript Console, DOM Inspector or any other Firefox window
- Position that window on top, somewhere in the area of the testcase (the green
block)
- Move the mouse from the testcase area into the window 'favicon'.

Result:
The cursor stays at a pointer form.
When the tooltip appears (while you're still at the window 'favicon'), the
pointer form of the cursor disappears.

The cursor should directly become the regular form, and no tooltip should appear
in this case.

I'll attach a screenshot.

This could very well be related to bug 291507.

It doesn't happen with a 2005-02-23 build, it happens with a 2005-02-24 build.
So my guess is, this is a regression from the patch that got checked in at that
time (fix #2) in bug 125386.

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: Screenshot of what I mean

This is a screenshot of what I mean.
I was not able to actually make a screenshot of the mouse in the pointer form
(the mouse turned back into it's normal form when I take the screenshot).

To reproduce, you have to move the mouse cursor from the left into the window
title area in the 'favicon' of that window.

Comment 3

[Martijn Wargers (dead)]

Attachment: screenshot2

In fact when moving from content in a Mozilla window into the whole title box
of another window, doesn't generate a mouseout, so content in the Mozilla
window is still in the hovered state.

Comment 4

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

Attachment: fix

NS_MOUSE_EXIT events targeted at a top-level window should be treated as
genuine exits even if the mouse position is still inside the window bounds, for
exactly the reason Martijn stated in comment #3.

Comment 5

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

Comment on attachment 189378 [details] [diff] [review]
fix

fixes a UI regression. Fairly simple and low-risk.

Comment 6

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

checked in

Comment 7

[Bryan]

If I understand the issue correctly I'm still seeing this with the latest
pacifica build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4)
Gecko/20050717 Firefox/1.0+ ID:2005071716

Comment 8

[Bryan]

Grabbed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050717
Firefox/1.0+ ID:2005071717 and still see this issue.  Also, I don't recall
seeing the hand pointer issue before with the previous build (1716) but now see
it quite a bit using this build.

Comment 9

[Martijn Wargers (dead)]

I'm still seeing this in 20050718 build, reopening.

Comment 10

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

Is it WIndows-only now?

Comment 11

[José Jeria]

(Sorry for changing the status, i did it by mistake)

Comment 12

[[e-mail]]

(In reply to comment #10)
> Is it Windows-only now?
The tooltip still appears in my 9-hour-old GTK1 linux build.

Comment 13

[Martijn Wargers (dead)]

Ok, bug 297561 didn't fix this bug, although it is fixed with the steps to
reproduce in comment 0 (which bug 297561 did seem to fix).

New steps to reproduce are the same as in comment 0, except the last one:
- Move the mouse from the testcase are into the window.

Result:
The tooltip from the testcase appears.

Expected:
No appearing of the tooltip from the testcase.

Comment 14

[Martijn Wargers (dead)]

Attachment: screenshot3

So I'm getting something like this.

Comment 15

[Martijn Wargers (dead)]

This is also something when moving into a flash file:
- Open this url in a new tab:
https://bugzilla.mozilla.org/attachment.cgi?id=83959
- Hover over the tab, wait for the tooltip to appear.
- Hover over the flash file.

Result:
Tooltip stays, when it should disappear.

Comment 16

[Martijn Wargers (dead)]

I suspect this could also be a widget->win32 bug (I'm still seeing this with a 2006-11-20 trunk build).

Comment 17

[Ere Maijala (slow)]

I'm failing to see any problem at the widget level. I think we should never show tooltips for a window that's not focused, but it's not widget layer that makes the decision.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It it not only tooltip issue, There are other issues.
Please test with the URL. The mouseout event is not fired, and the hover state is not reset by moving to another window.

Comment 19

[Martijn Wargers (dead)]

Attachment: testcase2

I just sort of rediscovered this issue.

Comment 20

[Martijn Wargers (dead)]

Ok, this is related to bug 311558, so probably caused by bug 125386, too.

Comment 21

[Martijn Wargers (dead)]

Attachment: seems to work

This seems to fix it without breaking bug 125386, it seems. I only patched windows so this is not enough for the other platforms.
Basically, it is backing out some stuff that was added in bug 125386, and instead it does a check in the widget code.

Comment 22

[Ere Maijala (slow)]

Comment on attachment 290695 [details] [diff] [review]
seems to work

It's always scary to touch these parts of the code, but it looks like Martijn has it right and the code in ESM is just wrong trying to decide something with the window coordinates. I don't know either how/if this affects other platforms.

Comment 23

[Martijn Wargers (dead)]

Chris, Robert, is the "seems to work" patch any good? If it's any good, someone on linux (and mac?) also needs to fix something in the widget code.

Comment 24

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

I guess that makes sense. The comment in nsEventStateManager above the removed code would need to be changed. 

Comment 25

[Sylvain Pasche]

I tried that patch (the part in nsEventStateManager) on Linux which fixes bug 408723.

I didn't do anything in widget/gtk, but the mouseout event are still dispatched while running the testcase here (the tooltip disappears).

Comment 26

[Wayne Mery (:wsmwk)]

Should this be considered an accessibility issue?

does it fix these:
bug 404124
bug 345219
bug 352749
bug 350679
bug 350354
bug 311558
bug 315904

related?
bug 366815
bug 392462

similar but doesn't fit regression
bug 45497
bug 50511
bug 148820 

Comment 27

[Aaron Leventhal]

I don't see any major accessibility issues here.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The Windows issue sounds like what I ran into in bug 20022 comment 101 / bug 233374 -- NS_MOUSE_ENTER and NS_MOUSE_EXIT events on Windows, when exiting both a parent and a child at the same time, seem to go to the child, and we have a widget that's a child of the toplevel covering the same area.  Or something like that -- my general statement might be slightly incorrect (and could probably be made a bit more general too), but it's a possible explanation for what I saw.

Comment 29

[Matthew Gregan [:kinetik]]

Attachment: gtk2 widget patch

Variant of Martijn's fix for the gtk2 code.  Fixes the problems shown by the bug's testcases and doesn't seem to regress anything in related areas (checked with testcases from other bugs referred to in this bug and general use).

Comment 30

[Matthew Gregan [:kinetik]]

We still need a fix for the Cocoa widget code, at least.  I can reproduce the bug using Martijn's testcase2 if I move the mouse fast enough.

Comment 31

[Martijn Wargers (dead)]

Would be great if someone could fix it for the Mac!

Comment 32

[Matthew Gregan [:kinetik]]

I'm wrong, the Cocoa widget stuff is okay on current trunk.  I was testing with my main browser installation, which turned out to be beta2 not nightlies like I thought.  Looking at the changes to nsChildView.mm between beta2 and now, I'd guess the relevant Cocoa change is the patch for bug #407876, but I haven't verified that.

The behaviour on trunk is different with Cocoa compared to GTK2/Win32 for Martijn's testcase (comment #19), because browser window does not handle mouse move/enter/exit when unfocused any longer, so it's not possible for the div to turn blue and show the tooltip when the error console has focus, unlike GTK2/Win32.  I'm not sure if this is a bug or an intentional change for closer OS-native behaviour, though.

Comment 33

[Matthew Gregan [:kinetik]]

Attachment: patch for gtk2, win32, and ESM

Complete patch, includes GTK2 and Win32 widget changes plus change to nsEventStateManager. The nsEventStateManager change reverts roc's fix from comment #4.

Comment 34

[Matthew Gregan [:kinetik]]

Comment on attachment 296430 [details] [diff] [review]
patch for gtk2, win32, and ESM

Requesting a1.9+.  This fixes a number of bugs, including bug #408723 which has been annoying quite a few people.  The effects of changing this code can be subtle, so there is a chance of causing regressions, but I haven't noticed any problems during my testing so far.  The sooner we get this in the more chance we have of finding any regressions.

Comment 35

[Mike Schroepfer]

Comment on attachment 296430 [details] [diff] [review]
patch for gtk2, win32, and ESM

As a 1.9+ blocker bug you do not need approval for the patch.  If you have the right reviews go ahead and check in!

Comment 36

[Matthew Gregan [:kinetik]]

Removing check-needed, I just discovered that the patch regresses bug #125386.  I thought I had tested that one in particular, but I must've accidentally loaded one of the other testcases from the bug that looked the same but did not behave the same (I was expecting to see an alert() if my patch had regressed that bug, but perhaps I accidentally loaded the testcase that sets window.status instead).

Comment 37

[Matthew Gregan [:kinetik]]

The patch only regresses bug #125386 on Linux/GTK2.  Martijn's part of the patch on Win32 is fine.

Comment 38

[Daniel Holbert [:dholbert]]

Is it possible to get a final patch checked in for this before next Tuesday's code freeze?

Bug 408723 (which this bug's patch fixes) is really annoying for Linux users, and it'd be awesome to have it fixed for the next beta.

Comment 39

[Reed Loden [:reed]]

kinetik, any update?

Comment 40

[Matthew Gregan [:kinetik]]

I have been working on this recently, and I hope to have something ready before b3 freeze, but I'm not really familiar with all of the code involved and I really don't want to cause any regressions in this area.

The problem with Martijn and I's earlier patch is that part of the fix involved removing code from nsEventStateManager that I now think we need to keep (in some form).  The problem is that we generate mouse exit events in the GTK+ widget code from leave notify events in a bunch of cases, including the case when the mouse traverses obscured widgets within a single toplevel.  I don't think it's possible to suppress these at the widget level because it has no knowledge of views and it's just not possible to distinguish between events we want to suppress or allow in this case.

Comment 41

[Matthew Gregan [:kinetik]]

Attachment: patch v3

Third attempt.  I ended up going with a pretty simple additional change to the ESM on top of the existing changes.  I've tested this a fair bit and it seems to work well--it fixes this bug and #408723 (same root cause) without regressing bug #125386 or anything else that I'm aware of.

Rather than convert mouse exits for non-toplevels when the exit is within the view into moves, we mark the exit as potentially bogus and giving GenerateMouseEnterExit the option to discard the exit if it is truly bogus.  GenerateMouseEnterExit determines this by looking at the current content under the exit and comparing it to the last content seen by a mouse move.

There are still mouse exit issues that this patch does not fix, e.g. bug #311558 can still occur (and my comment in that bug is wrong).

roc, I think I mentioned that the fix for this bug would allow me to remove the fix for the combo box inside :hover problem on Linux/GTK+, but it doesn't, because the leave notify generated in that case (which was suppressed by my patch in that bug) would still be considered real exits.

Comment 42

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

A bit more context in the diff makes reviewing easier.

> PRBool discardIfSameContent)

Follow the stupid aParam convention

   case NS_MOUSE_EXIT:
     {
+      if (discardIfSameContent && targetElement == mLastMouseOverElement) {
+        break;
+      }

In what situations is it important to not break here when the target element is unchanged?

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 301085 [details] [diff] [review]
patch v3

>+static nsWindow         *gCurrentWindow        = NULL;

Might be good to give this a different name, since "current" could mean lots of different things.  This is the one the mouse pointer is in.


Also, I'm a little skeptical of this patch in general because of what I wrote in bug 408723 comment 30; it (or at least some part of it) almost feels like a core change to work around a front-end bug.

Comment 44

[Reed Loden [:reed]]

Moving this to P1 since it also supposedly fixes P1 blocker bug 408723.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #43)
> Also, I'm a little skeptical of this patch in general because of what I wrote
> in bug 408723 comment 30; it (or at least some part of it) almost feels like a
> core change to work around a front-end bug.

That said: I agree there's a real core bug causing bug 408723, and I agree that the fix could be in cross-platform code given what this code does.

I think in this bug, there's additionally an issue that's Windows only, which is that there's a bug in roc's earlier patch, because it doesn't handle the difference in which widget gets NS_MOUSE_EXIT and NS_MOUSE_ENTER events when there are multiple widgets with the same coordinates (as I said in comment 28).  This bug is that null-checking parentWidget to determine whether this is "toplevel" is going to be incorrect on Windows; your patch doesn't remove that part of roc's change, and I think explains why some of the symptoms of this bug are more pronounced on Windows.

I think it would be useful to better understand the sequence of NS_MOUSE_EXIT and NS_MOUSE_ENTER events that you get on different platforms when the mouse moves between widgets or when widgets are created/destroyed under the mouse pointer.  It seems pretty clear to me that they're different, but I'm not sure how.  (Or maybe you already know how, but just didn't explain how above.)  If possible, the best way to deal with those differences may be to make the platforms do the same thing and document it in appropriate widget headers; if not, we should document the difference and work around it.

That said, why are you discarding the event only if you're above the same content?  Shouldn't you always discard it?  If it's discardable, why would it ever be at a different position?  (But roc already said that in comment 42.)

Comment 46

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

Not sure about this one. I think we could probably live with shipping this bug. If we do take a patch, it probably should be in beta since this code is fragile.

Comment 47

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

Actually I talked to Matthew, and he says assuming we have a front-end fix for 408723, this bug is not that bad. So we shouldn't block on it. We might still take a patch if Matthew thinks the patch is good for beta4.

Comment 48

[Matthew Gregan [:kinetik]]

> In what situations is it important to not break here when the target
> element is unchanged?

If the mouse moves out of a top-level window, we may think the content is
the same because we use the result of mDocument->GetRootContent() if
GetEventTargetContent returned NULL.  If the last real mouse move target was
the document content root, we'd discard these mouse exits incorrectly
without the mark-as-fake machinery.  I'm not sure if we could stop using the
document content root (which the mouse move case presumably has a good
reason for doing) when GetEventTargetContent returned NULL for the exit
case, but that may be another option.

After a fair bit of testing on each major platform, it turns out the current
patch doesn't fix all of the cases it's supposed to, so I'm marking it
obsolete.  I'm trying to come up with a better patch at the moment,
hopefully I'll have something soon.

I'm using the following Google Spreadsheet to track different tests and
their results on each platform.  It's a bit of a mess at the moment, I'll
clean it up as I go.  I've probably missed some cases, too, so suggestions
are welcome.

http://spreadsheets.google.com/pub?key=pyhu4VNTXF0fitCu0_ZCucw

Some of the tests might be able to be made into Mochitests, but I'm not
sure.  It'd be really nice if we had a way to write tests for the widget
code.

Comment 49

[Matthew Gregan [:kinetik]]

Attachment: prototype of patch v4

Add a new flag to nsMouseEvent to distinguish mouse exits between top level windows versus child widgets.  nsEventStateManager uses this flag (and drops the existing code to compare the event location to view bounds and widget ancestry) to decide which mouse exits should be converted into mouse moves.

(An alternate approach would be to change the widget code for each platform to dispatch exit or move events directly rather than setting a flag on the nsMouseEvent, but this involves duplicating a little code.)

I also changed the NS_MOUSE_EXIT dispatch path that is invoked by the MouseTrailer stuff to synthesize an event location for the exit. This is necessary to solve a problem where we can generate mouse exit events that cause spurious mouseouts to the wrong content when a widget appears underneath the cursor when the mouse have moved recently.

As an aside, the fact that the enums in nsMouseEvent are also bitfields seems like a loaded gun aimed at our feet.  I made the new flag a plain enum without attempting to pack it, but I can change that if necessary.

This is a prototype patch of the new approach I'm taking.  It fixes the bugs I'm trying to solve, and a couple of other related ones I found and mentioned in the spreadsheet.  I've only made the changes for Windows so far, working on the GTK+2 changes now.

Comment 50

[Matthew Gregan [:kinetik]]

Attachment: patch v4

Make required changes to the widget code for Cocoa and GTK+2.  Also fixed a bug in my earlier changes to the Windows widget code when synthesizing event coordinates.

The widget code for other platforms needs to be updated too, but I have no way to test it, so I haven't even looked at doing this.

I think this patch is done, but I want to do a bunch more testing before I request review.

Comment 51

[Matthew Gregan [:kinetik]]

Attachment: patch v5

I think this is done.

Final version includes a change to the is_top_level_exit logic in the GTK+2 widget code to fix a problem with exit type determination when a GTK grab is active.  Rather than use the widget passed in from the leave notify callback (which can be redirected due to grabs), we use the inner_window of our MozDrawingArea to find our top level window.

I've tested this fairly thoroughly on each of the three major platforms.  With the above bug fixed, I'm not aware of any regressions introduced by this patch.

Comment 52

[Matthew Gregan [:kinetik]]

Comment on attachment 308363 [details] [diff] [review]
patch v5

I did forget to mention one thing.  The code used to find and compare top level windows in the GTK+2 widget code only works for GTK widgets.  Because of this, it is possible that exit events could be mistakenly marked as top level exits (only, we'll never mistakenly mark them as child exits) when they are not.  The most common case where this could occur is a non-GTK plugin.  I've tested with GTK and non-GTK plugins and can't detect any problems as a result of this, but there is a chance that spurious top level exits could still occur in some cases.

Requesting review.

Comment 53

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

+                                                    PRBool isTopLevelExit = PR_FALSE)

Don't use default parameters here, where it doesn't buy us much. They make it a bit harder to read code. And why not make this parameter be the event enum? That would save a tiny bit of code and be more descriptive.

+    GdkWindow* winAtPt = gdk_display_get_window_at_pointer(display, &x, &y);
+    if (!winAtPt)
+        return PR_TRUE;

This introduces an X server roundtrip, right? And there's also possibly race conditions where the X server sends us a mouse event that was inside our window, but by the time we get here windows have moved and the pointer is outside. I guess I can live with that.

+static PRBool IsTopLevelMouseExit(HWND aWnd)
+{

Why do we need to do this? Can we not rely on WM_MOUSELEAVE being sent and just pass the exit type to DispatchMouseEvent?

Comment 54

[Matthew Gregan [:kinetik]]

Attachment: patch v6

Updated patch with the suggested changes to the Cocoa widget code.

> This introduces an X server roundtrip, right? And there's also possibly race conditions where the X server sends us a mouse event that was inside our window, but by the time we get here windows have moved and the pointer is outside. I guess I can live with that.

Right.  It's not ideal, but I don't have a better solution right now.

> Why do we need to do this? Can we not rely on WM_MOUSELEAVE being sent and just pass the exit type to DispatchMouseEvent?

We dispatch WM_MOUSELEAVE events ourselves via the MouseTrailer timer when the window under the pointer is different from the window we're tracking.  We also dispatch NS_MOUSE_EXIT events directly from the mouse move handler when the we detect a window crossing.  In both cases it's possible for these exit events to be caused by child window crossings, so we need to classify them somewhere.

Comment 55

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

Comment on attachment 308782 [details] [diff] [review]
patch v6

+  exitType     exit;

You'd better document that this is only initialized for NS_MOUSE_EXIT events.

Comment 56

[Matthew Gregan [:kinetik]]

Comment on attachment 308782 [details] [diff] [review]
patch v6

Requesting approval.  Fixes this bug and all of the ones currently blocked by this bug.  This was a blocker before beta 4, but I didn't have a patch ready before the tree froze.  If we're going to take this, it needs exposure in beta 5 because this code is easy to cause regressions in and not well covered by tests (sadly).

I've tested this on all three major platforms quite a bit and it seems okay to me.  Mochitests pass on Linux.

Comment 57

[Matthew Gregan [:kinetik]]

File bug #422303 (BeOS), bug #422304 (OS/2), and bug #422305 (Photon) to remind someone to update widget code for other platforms.

Comment 58

[Damon Sicore (:damons)]

Comment on attachment 308782 [details] [diff] [review]
patch v6

a1.9+=damons

Comment 59

[Reed Loden [:reed]]

Woohoo!

Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.732; previous revision: 1.731
done
Checking in widget/public/nsGUIEvent.h;
/cvsroot/mozilla/widget/public/nsGUIEvent.h,v  <--  nsGUIEvent.h
new revision: 3.159; previous revision: 3.158
done
Checking in widget/src/cocoa/nsChildView.mm;
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.319; previous revision: 1.318
done
Checking in widget/src/gtk2/nsCommonWidget.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsCommonWidget.cpp,v  <--  nsCommonWidget.cpp
new revision: 1.36; previous revision: 1.35
done
Checking in widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.262; previous revision: 1.261
done
Checking in widget/src/windows/nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.734; previous revision: 3.733
done

Comment 61

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008033005 Minefield/3.0pre

Thanks for fixing this, Matthew!

Comment 62

[[e-mail]]

At least on Windows, the mouseleave calculation is not quite correct, so that if you move the mouse over the title bar then your mouseleave gets converted to a mousemove with negative coordinates...

Comment 63

[Masahiro YAMADA]

Bug 423590 may be regression of this bug.

Comment 64

[Masahiro YAMADA]

Sorry, Bug 423590 is dup of Bug 426630.

Comment 65

[Matthew Gregan [:kinetik]]

(In reply to comment #62)
> At least on Windows, the mouseleave calculation is not quite correct, so that
> if you move the mouse over the title bar then your mouseleave gets converted to
> a mousemove with negative coordinates...

That will be fixed by my patch for bug 428680.