Bug 105729 – Mouse cursor is not reset when a new page is loaded

• Status: ASSIGNED
• Component: WebCore Misc.

[Chris Dumez]

The mouse cursor is not reset when a new page is loaded. As a result, the cursor from the previous page is shown until the cursor is moved.
This causes flakiness in our layout tests as well.

I was able to reproduce the flakiness by running:
Tools/Scripts/run-webkit-tests --efl --debug -2 -p --child-processes=1 fast/css/color-correction.html fast/css/crash-on-custom-cursor-when-loading.html fast/css/create_element_align.xhtml

fast/css/color-correction.html loads the purple-srgb.png resource in the cache,
then fast/css/crash-on-custom-cursor-when-loading.html shows a custom mouse cursor (purple-srgb.png),
finally the custom cursor is still shown on fast/css/create_element_align.xhtml, which is wrong.

As a consequence, fast/css/create_element_align.xhtml and others are flakey when running pixel tests.

[Chris Dumez]

Created attachment 180681 [details]
Patch

[WebKit Review Bot]

Attachment 180681 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:4236:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:55:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

I doubt this belongs in FrameLoader directly...

[Chris Dumez]

Sadly, I couldn't make it a reftest (the custom cursor would not show). I can reproduce the issue only for pixel tests so the new test is a pixel test.

[Chris Dumez]

Created attachment 180682 [details]
Patch

Fix style errors.

[Chris Dumez]

(In reply to comment #3)
> I doubt this belongs in FrameLoader directly...

Thanks for taking 

I did not know where else I should reset the mouse cursor from. The mouse cursor is currently set in Source/WebCore/page/EventHandler.cpp but I need to know when a new page is loaded to reset it. Therefore, I thought of the FrameLoader.

Do you have a suggestion where I should move it to if you think adding this to the FrameLoader is a bad idea?

I could do the same from the FrameLoader client but then it would be port-specific.

[WebKit Review Bot]

Comment on attachment 180682 [details]
Patch

Attachment 180682 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15507536

New failing tests:
fast/parser/document-write-ignores-later-network-bytes.html
fast/frames/frame-unload-crash2.html

[Chris Dumez]

Created attachment 180687 [details]
Patch

Add null check for m_frame->view(). Should fix regression for fast/frames/frame-unload-crash2.html

[Chris Dumez]

Created attachment 180688 [details]
Patch

[Alexey Proskuryakov]

It's weird for loading machinery to be also responsible for cursors. It is complicated enough already.

What is the easiest way to reproduce this in a browser? I don't think that I'm seeing this in Safari.

[Chris Dumez]

(In reply to comment #10)
> It's weird for loading machinery to be also responsible for cursors. It is complicated enough already.
> 
> What is the easiest way to reproduce this in a browser? I don't think that I'm seeing this in Safari.

I have created a small page to reproduce this easily in a browser:
http://hydr0g3n.free.fr/testcursor/

Just hover the link (custom cursor should appear), then click the link and keep the mouse cursor still. The custom cursor will stay after the new page is loaded.

I can reproduce the issue in EFL's MiniBrowser and with Chromium but not with Safari.

[Chris Dumez]

Created attachment 180987 [details]
Patch

Move cursor reset code to FrameLoader::transitionToCommitted() since it seems slightly better than FrameLoader::commitProvisionalLoad(). I know it would be nice to move this out of the FrameLoader but I haven't found the right place for this yet. It would be nice to keep the fix in non port-specific code.

[Eric Seidel (no email)]

Manually setting the cursor is pretty lame, but that may be our API in WebCore.  What happens if the cursor is already set to something else when the load is committed?

For example, what happens if you click the link, but before the load completes you move your cursor over the URL bar?  It shoudl be an i-bar, but this code is now going to reset it forcefully to a pointer when the page loads.

[Chris Dumez]

(In reply to comment #13)
> Manually setting the cursor is pretty lame, but that may be our API in WebCore.  What happens if the cursor is already set to something else when the load is committed?
> 
> For example, what happens if you click the link, but before the load completes you move your cursor over the URL bar?  It shoudl be an i-bar, but this code is now going to reset it forcefully to a pointer when the page loads.

Right, resetting to a pointer is not a good idea. We need to update the mouse cursor instead. I'll check EventHandler::dispatchFakeMouseMoveEventSoon(). It seems to be used to update the cursor in cases such as scrolling already.

[Alexey Proskuryakov]

So how does Safari do this?

[Darin Adler]

Comment on attachment 180987 [details]
Patch

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

Seems like a good idea to get the cursor set, but we need to do this some way other than setting it explicitly to the pointer cursor.

> Source/WebCore/loader/FrameLoader.cpp:1813
> +    // Make sure the mouse cursor is reset.
> +    if (isLoadingMainFrame())
> +        m_frame->page()->chrome()->client()->setCursor(pointerCursor());

This is not right. We want to set the cursor to the appropriate cursor for the new page, not unconditionally set it to a pointer. We don’t want the cursor to flash to a pointer every time a new page is loaded, if we’re going from one page to the next.

[Chris Dumez]

Unfortunately, EventHandler::dispatchFakeMouseMoveEventSoon() does not have the intended effect in this case because EventHandler::clear() is called by FrameLoader::clear() when a new page is loaded. EventHandler::clear() resets m_mousePositionIsUnknown to true and clears m_lastKnownMousePosition. EventHandler::dispatchFakeMouseMoveEventSoon() requires the last mouse position to be known in order to work.