Bug 85343 – Cursor change fires mousemove event

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: UI Events

[David Barr]

Upstream bug: http://crbug.com/103041

Simple reproduction:
http://jsfiddle.net/TRF7g/

Ran bisect-builds, bug present in WebKit since at least r37376.

-- Original report --

Chrome Version       : 16.0.912.21
OS Version: OS X 10.6.8 (also checked windows7)
URLs (if applicable) :
Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:
     Safari 5: FAIL
  Firefox 4.x: OK
     IE 7/8/9: FAIL*

What steps will reproduce the problem?
1. Create an event listener that is triggered by 'mousemove'
2. Change the cursor either with a CSS class change or element.style.cursor
3. Observe the event listener

What is the expected result?
Cursor changes without firing a mousemove event.

What happens instead?
Cursor changes and a mousemove event is fired.

Please provide any additional information below. Attach a screenshot if
possible.

Seems like it may be a webkit issue since Safari behaves exactly like Chrome for this issue. 
*IE9 (haven't tested others) does not change the cursor until the user moves the mouse (this also seems incorrect.)

UserAgentString: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.21 Safari/535.7

This issue is a thorn in the side of anyone wishing to change the cursor based on a mousemove timeout - such as hiding a cursor (during video playback, presentations, etc).

[Rick Byers]

Is this because currently the only way the mouse cursor ever gets changed is via EventHandler::handleMouseMoveEvent?  It looks like it shouldn't be too hard to add another function that sets the mouse cursor based on the current position...

[Aivo Paas]

No, it's actually because cursor change is fired through a fake mousemove event. I have a fix that doesn't use the fake mousemove event and thus doesn't fire the event. It also fixes bug 53341. I will propose a patch as soon as I get my environment set up correctly.

[Aivo Paas]

Created attachment 173305 [details]
Patch

[Rick Byers]

Something along these lines seems reasonable to me (although I'm not a webkit reviewer).  I was thinking of refactoring selectMouseCursor to not take a whole mouse event and hit test result, but I'm not sure if that's better (needs a position, node and a few other little things).

You'll need to add tests of course, but my bug 100550 should land soon making it easy to write a layout test for this scenario.

Mouse cursors seem like an area that hasn't been touched mouch, I'm not sure who the right reviewers are.

[Rick Byers]

Comment on attachment 173305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173305&action=review

> Source/WebCore/page/EventHandler.cpp:146
> +const double updateMouseCursorInterval = 0.05;

Are you sure a timer is necessary in this case?  I think fake mouse move codepath has a timer for scrolling where there are lots of updates in a row.  If we're getting lots of style updates in a row then I'd guess that there's already a performance problem and mouse cursors might be the least of the concerns.  But I guess this is the safer approach without more performance data.  But it's a shame to add this complexity if it's not really adding value.

> Source/WebCore/page/EventHandler.cpp:1364
> +    ASSERT_UNUSED(timer, timer == &m_updateMouseCursorTimer);

This function shares a lot of code with fakeMouseMoveTimerFired.  We should probably refactor to share common code.

[Aivo Paas]

Yes, it is safer with the timer, but I'll try how it behaves without it. Should be just fine I guess.

Regarding the shared code - should dig up why the fake mousemove event was added in the first place. Maybe that problem could also be solved in better way.

[Aivo Paas]

Created attachment 173492 [details]
Moved test to the fix in 101857

[Build Bot]

Comment on attachment 173492 [details]
Moved test to the fix in 101857

Attachment 173492 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14792669

New failing tests:
fast/events/mouse-cursor-no-mousemove.html

[Rick Byers]

Comment on attachment 173492 [details]
Moved test to the fix in 101857

View in context: https://bugs.webkit.org/attachment.cgi?id=173492&action=review

> LayoutTests/fast/events/mouse-cursor-no-mousemove.html:42
> +    }, 10);

We shouldn't rely on specific time outs like this in tests.  It's either going to be flaky (under load not long enough) or waste time doing nothing.  Is setTimeout 0 not enough here for the style change to take effect?

> LayoutTests/fast/events/mouse-cursor-no-mousemove.html:48
> +    }, 50);

If you've already verified you've got the new cursor, then it shouldn't be necessary to wait any longer, right?  Once you change it, does this test fail reliably before your fix?

[Rick Byers]

You probably want to combine the fix with all the tests that it fixes in the same patch (they should be submitted together).

[Aivo Paas]

(In reply to comment #11)
> You probably want to combine the fix with all the tests that it fixes in the same patch (they should be submitted together).

Tests are already combined with the fix in Bug 101857
I will upload an updated patch with updated timeouts.

[Allan Sandfeld Jensen]

*** This bug has been marked as a duplicate of bug 101857 ***