312566 - Fix MouseTrailer and other mouse handling issues (Was: Try to get rid of MouseTrailer)

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Ere Maijala (slow)]

Move MouseTrailer somewhere from nsToolkit to be able to use nsCOMPtr<nsWindow>
in it.

Comment 1

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

Remind me why we can't use TrackMouseEvent?
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/userinput/mouseinput/mouseinputreference/mouseinputfunctions/trackmouseevent.asp
(assuming we're willing to drop support for Windows 95 on trunk, which we are)

Comment 2

[gabe]

actully it has already droped windows 95 (firefox 1.0) and firefox 2.0 is planed too drop 98

Comment 3

[Ere Maijala (slow)]

Too bad it seems we can't use TrackMouseEvent anyway. It seems to behave badly when dragging. It will post WM_MOUSELEAVE right away when the drag is started. 

One possibility would be a global hook using SetWindowsHookEx, but this might cause context switches with every single mouse event. So I guess our best bet is to continue with a timer based implementation, but I'm going to change it a bit.

Comment 4

[Ere Maijala (slow)]

Attachment: Patch v1

Ok, so we're not going to get rid of MouseTrailer. Let's just try to make it work well. 

This is my first stab at solving a few issues with the mouse handling. The changes include:

Switched MouseTrailer to use HWND's instead of nsWindow pointers. This lets us get rid of reference counting and related problems. 

Mouse trailer posts the exit message WM_MOUSELEAVE to the window's message queue instead of trying to call DispatchMouseEvent directly. This avoids possible problems if the window is destroyed.

Position in LPARAM is used for mouse messages. Makes sure the position is always correct avoiding the need to call GetMessagePos and makes synthesizing events easier.

Added WM_CANCELMODE handling which cancels mouse capture. This avoids for example the situation where a dialog is displayed during capture and the capture should be released. Can be tested for example with the replacement testcase of bug 297561. Press left mouse button and move over the yellow text.

And more... This one needs testing. Could you guys give it a shot and try to find any problems?

Comment 5

[Ere Maijala (slow)]

One important addition: I doomed the switch to top level windows in MouseTrailer incorrect. Now it's back to child windows or whatever it's given. This makes the correct window receive the EXIT event.

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase1

I've tried the patch and it seems to cause this issue.
When quickly hovering from right to left in the iframe, I get a brief flash of red in the top box, which should not happen.

Comment 7

[Martijn Wargers (dead)]

The patch seems to fix bug 297080 and bug 307399.

Comment 8

[Martijn Wargers (dead)]

Another regression in the Firefox autoscroll function with this patch:
- Scroll to the bottom of any page (this page will do)
- Middle-click to open the autoscroll image
With the patch, the page will already start to scroll upwards without even moving the mouse (this should not happen when the mouse has not moved). When you move the mouse, this is corrected.
The speed of how fast the page will scroll up seems related to the spot where you've middle-clicked to get the autoscroll image. Middle-clicking at the top of the browser window, the page scroll upwards very slowly. Middle-clicking at the bottom, and the page scrolls upwards very fast.

Comment 9

[Ere Maijala (slow)]

Comment on attachment 202006 [details] [diff] [review]
Patch v1

Sounds like wrong coordinate set somewhere. I'll investigate.

Comment 10

[Ere Maijala (slow)]

Attachment: Patch v1.1

This patch fixes coordinate set handling. The test case works with these fixes, I've yet to compile Firefox to test the scrolling, but I'm quite confident it'll be good too. Doug, would you be able to review this one? This should also fix the simulated mouse input using PostMessage.

Comment 11

[Martijn Wargers (dead)]

Yes, Patch v1.1 fixes the autoscroll problem.

Comment 12

[Ere Maijala (slow)]

Changing summary to reflect reality. This will also fix bug 316123.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 202827 [details] [diff] [review]
Patch v1.1

Please ifdef out the calls to WM_MOUSELEAVE on the WINCE platform.

I have review this change and it looks fine.  Most of my questions when reading through this patch were about the use of mIsInCaptureMode (like when is it set to true or false).

I haven't done extenstive testing, but it seams to work fine in FF.

Comment 14

[Ere Maijala (slow)]

I'll add the ifdef. mIsInCaptureMode is set by call to nsWindow::CaptureMouse. It's called whenever a mouse button is pressed or released, or when the capture mode is lost (WM_CANCELMODE).

Comment 15

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

How does mMouseCaptureWindow get cleared if that window is destroyed?

Comment 16

[Ere Maijala (slow)]

Comment on attachment 202827 [details] [diff] [review]
Patch v1.1

Good catch. Well, it would have been reset by the next click in another window but meanwhile mouse trailer would have misbehaved. I'll fix it so that it's cleared in ~nsWindow.

I noticed another problem. It's again the same problem as in bug 297561. The exit message must be sent to the trailed window, not the toplevel window, for everything to work correctly, but this makes nsEventStateManager to convert it to MOVE if the coordinates are inside the window. I can work around this by making WM_MOUSELEAVE use MAXDWORD as the coordinates (they are separately constructed anyway), but why does ESM do that to us?

Comment 17

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

(In reply to comment #16)
> I noticed another problem. It's again the same problem as in bug 297561. The
> exit message must be sent to the trailed window, not the toplevel window, for
> everything to work correctly, but this makes nsEventStateManager to convert it
> to MOVE if the coordinates are inside the window. I can work around this by
> making WM_MOUSELEAVE use MAXDWORD as the coordinates (they are separately
> constructed anyway), but why does ESM do that to us?

Why does the exit message need to be sent to the trailed window?

Comment 18

[Ere Maijala (slow)]

I think it's the right thing to do to send it to whoever was seeing the mouse last. It doesn't make sense to send exit to a window that hasn't even seen the mouse. In practice sending it to wrong window might leave a dragged link to pushed state or hover effect on (this happens for example with menus where there's no space between the menu and title bar). 

Comment 19

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

(In reply to comment #18)
> I think it's the right thing to do to send it to whoever was seeing the mouse
> last. It doesn't make sense to send exit to a window that hasn't even seen the
> mouse. In practice sending it to wrong window might leave a dragged link to
> pushed state or hover effect on (this happens for example with menus where
> there's no space between the menu and title bar). 

This case shouldn't be a problem because the ESM should unset hover on all content. However it probably won't work on content in unlinked subdocuments.

> why does ESM do that to us?

Suppose we have a content widget in the page, say a scrolled textarea. Suppose there's a normal DIV overlapping it which doesn't have a widget, and the mouse exits the textarea widget while staying within the DIV. We have to convert this "exit" event to a "move" otherwise we fire a mouseout DOM event at the DIV which is totally wrong.

Maybe the best solution is to define two mouse exit messages:
-- "Mouse left the widget, is still over a Mozilla widget in the same toplevel window" (so the ESM should convert to a MOVE)
-- "Mouse left the widget, is no longer over the same toplevel window" (so the ESM should not convert to a MOVE)

Alternatively use a flag to distinguish these.

Comment 20

[Ere Maijala (slow)]

> This case shouldn't be a problem because the ESM should unset hover on all
> content. However it probably won't work on content in unlinked subdocuments.

Is the menu bar an unlinked subdocument? 

> Maybe the best solution is to define two mouse exit messages:
> -- "Mouse left the widget, is still over a Mozilla widget in the same toplevel
> window" (so the ESM should convert to a MOVE)
> -- "Mouse left the widget, is no longer over the same toplevel window" (so the
> ESM should not convert to a MOVE)
> 
> Alternatively use a flag to distinguish these.

Can I just set the position to MAXDWORD? That'd do the trick.

Comment 21

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

(In reply to comment #20)
> > This case shouldn't be a problem because the ESM should unset hover on all
> > content. However it probably won't work on content in unlinked subdocuments.
> 
> Is the menu bar an unlinked subdocument?

No.

> Can I just set the position to MAXDWORD? That'd do the trick.

Okay, as long as you clearly document that it means "mouse has left the Mozilla toplevel window" ... it's ambiguous otherwise.

Comment 22

[Ere Maijala (slow)]

(In reply to comment #21)
> (In reply to comment #20)
> > > This case shouldn't be a problem because the ESM should unset hover on all
> > > content. However it probably won't work on content in unlinked subdocuments.
> > 
> > Is the menu bar an unlinked subdocument?
> 
> No.

Then I wonder why it doesn't work. 

Comment 23

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

so do I :-)

Comment 24

[Ere Maijala (slow)]

Attachment: Patch v1.2

Added ifdefs for WM_MOUSELEAVE as Doug requested. Fixed WM_MOUSELEAVE by setting the mouse position to MAXDWORD and added a comment to that. Changed the nsWindow destructor to clear out capture window if it was the window being destroyed.

Comment 25

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

+  int inx = 0;
+  while (gAllEvents[inx].mId != (long)pluginEvent.event && gAllEvents[inx].mStr != NULL) {
+    inx++;
+  }
+  printf("** %p %6d - %ld 0x%04X %s\n", this, gEventCounter++, aEventType, pluginEvent.event, gAllEvents[inx].mStr ? gAllEvents[inx].mStr : "Unknown");
 
What's this for? Shouldn't it be #ifdef DEBUG at least?

Comment 26

[Ere Maijala (slow)]

That's something I accidentally forgot there. It's not supposed to go in even in #ifdef and I'll remove it.

Comment 27

[Masatoshi Kimura [:emk]]

>> Then I wonder why it doesn't work. 
> so do I :-)
When mouse cursor moved from menu to title bar,
mouse trailer doesn't fire NS_MOUSE_EXIT because mouse cursor is still on the top level window's title bar.
And WM_MOUSEMOVE handler is not even called because title bar is a nonclient area.
Therefore you have no chance to fire NS_MOUSE_EXIT event.
My solution was adding WM_NCMOUSEMOVE handler. See my attachment 202733 [details] [diff] [review] for bug 316123 (which depends on this bug).

Comment 28

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

Comment on attachment 204286 [details] [diff] [review]
Patch v1.2

okay, let's do this and then fix bug 316123

Comment 29

[Masatoshi Kimura [:emk]]

Comment on attachment 204286 [details] [diff] [review]
Patch v1.2

Actually, this will also fix bug 316123. Menu widget is differ from title bar widget (new mouse trailer will do the right thing), but both widgets share the same top level window (current mouse trailer wouldn't work).

Comment 30

[Ere Maijala (slow)]

Fix checked in to trunk.

Comment 31

[Peter van der Woude [:Peter6]]

this probably caused Bug 321335

Comment 32

[Dave Townsend [:mossop]]

Probably a bit late and probably this patch is too in depth for this release, but it's be nice to see bug 307399 sorted in a release build.

Comment 33

[Daniel Veditz [:dveditz]]

Doesn't sound like a security/stability release candidate, and the known regressions are a bit scary.

Comment 34

[Uri Bernstein]

(In reply to comment #33)
> Doesn't sound like a security/stability release candidate, and the known
> regressions are a bit scary.
> 

But perhaps we can consider it for Firefox 2.0?

Comment 35

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

Not going to block 1.8.1 for this; it's hard to accept something as a blocker when the bug doesn't have any description of the symptoms.  It's also caused a bunch of regressions, so the patch seems scary.

Comment 36

[Johnny Stenback (:jst)]

Attachment: 1.8 version of the diff.

This is the above patch merged to the 1.8 branch, fixes this problem and also bug 373344.

Comment 37

[Johnny Stenback (:jst)]

Taking this on the 1.8 branch fixes bug 373344 as well, so nominating.

Comment 38

[Martijn Wargers (dead)]

This patch caused various regressions on trunk. It doesn't seem like a good idea to me to port this to branch (at least the regressions should also be taken into account, I think).

Comment 39

[Johnny Stenback (:jst)]

Ere, do you think it's reasonable to try to pull together all the regression fixes for this to land on the 1.8 branch? If so, can you look into that?

Comment 40

[Ere Maijala (slow)]

It's certainly a mess, but there are nice benefits, so I'd say yes. I can try to do it if it would be approved to the branch. There is certainly some risk, but the test cases are also there.

Comment 41

[Daniel Veditz [:dveditz]]

Does the 1.8 version patch include the various regression fixes? if not could someone please create a combined patch?

Comment 42

[Johnny Stenback (:jst)]

Dan, no it doesn't, but Ere said in comment 40 that he could come up with a combined patch. Ere, you're on! :)

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 266005 [details] [diff] [review]
1.8 version of the diff.

Not approving partial fix, for the branches we really want to combine all known regression fixes.

Comment 44

[Ere Maijala (slow)]

Attachment: Combined patch for 1.8 branch

It wasn't too easy to gather all the pieces together, but I believe this patch should contain everything. I couldn't access bug 373344, but the other test cases work properly.

Comment 45

[Martijn Wargers (dead)]

(In reply to comment #44)
> should contain everything. I couldn't access bug 373344, but the other test

I just CC-ed you to that bug, so you should be able to access it now.

Comment 46

[Daniel Veditz [:dveditz]]

Does the combined patch fix bug 373344 as well?

Comment 47

[Ere Maijala (slow)]

No, unfortunately it doesn't. More about it in that bug.

Comment 48

[Daniel Veditz [:dveditz]]

If this patch doesn't fix bug 373344 on the branch then it probably isn't something we want to take, unless there's something else we missed.

Of course if the eventual fix for bug 373344 requires this as a base then we can approve it.