Bug 211884 – Cursor should not update on a 20ms timer

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

[Antoine Quint]

In EventHandler, the cursor is updated on a timer scheduled with 20ms:

    // The amount of time to wait for a cursor update on style and layout changes
    // Set to 50Hz, no need to be faster than common screen refresh rate
    static const Seconds cursorUpdateInterval { 20_ms };

If the cursor should be updated only as the display refreshes, then it should be timed when the display refreshes and not use some canned timer.

[Radar WebKit Bug Importer]

<rdar://problem/63220368>

[Antoine Quint]

Should we be calling EventHandler::updateCursor() under Page::updateRendering() and schedule a page rendering instead of using m_cursorUpdateTimer.startOneShot(cursorUpdateInterval)?

[Antoine Quint]

Created attachment 399346 [details]
Patch

[Antoine Quint]

Created attachment 399347 [details]
Patch

[Antoine Quint]

Created attachment 399348 [details]
Patch

[Antoine Quint]

Created attachment 399349 [details]
Patch

[Antoine Quint]

*** Bug 209494 has been marked as a duplicate of this bug. ***

[Antti Koivisto]

Comment on attachment 399349 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8520
> +void Document::updateCursorIfNeeded()
> +{
> +    if (frame())
> +        frame()->eventHandler().updateCursorIfNeeded();
> +}

Maybe this function is not needed at all?

> Source/WebCore/page/EventHandler.cpp:3153
> +    m_scheduledCursorUpdate = true;

m_hasScheduledCursorUpdate?

[Antti Koivisto]

Comment on attachment 399349 [details]
Patch

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

> LayoutTests/fast/events/mouse-cursor-no-mousemove.html:52
> +    await new Promise(resolve => setTimeout(resolve, 100));

Why not use rAF here too?

[Antoine Quint]

(In reply to Antti Koivisto from comment #9)
> Comment on attachment 399349 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399349&action=review
> 
> > LayoutTests/fast/events/mouse-cursor-no-mousemove.html:52
> > +    await new Promise(resolve => setTimeout(resolve, 100));
> 
> Why not use rAF here too?

This part of the test tests an ancient change where a synthetic "mousemove" was seemingly dispatched when changing "cursor" styles and we want to make sure it doesn't happen. I'm not sure what a good timeout is here because I don't know what the old timer was, but at any rate nothing should happen during that time. I just reduced it from by 50ms since this was the timer for the check that is now performed with a rAF.

[Antoine Quint]

Committed r261686: <https://trac.webkit.org/changeset/261686>

[Simon Fraser (smfr)]

Comment on attachment 399349 [details]
Patch

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

> Source/WebCore/page/Page.cpp:1367
> +    forEachDocument([] (Document& document) {
> +        document.updateCursorIfNeeded();
> +    });

Shouldn't this happen after anything that can change style? If style is changed in a rAF callback, it should be reflected.

>>> LayoutTests/fast/events/mouse-cursor-no-mousemove.html:52
>>> +    await new Promise(resolve => setTimeout(resolve, 100));
>> 
>> Why not use rAF here too?
> 
> This part of the test tests an ancient change where a synthetic "mousemove" was seemingly dispatched when changing "cursor" styles and we want to make sure it doesn't happen. I'm not sure what a good timeout is here because I don't know what the old timer was, but at any rate nothing should happen during that time. I just reduced it from by 50ms since this was the timer for the check that is now performed with a rAF.

await UIHelper.delayFor() or await UIHelper.animationFrame().

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #12)
> Comment on attachment 399349 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399349&action=review
> 
> > Source/WebCore/page/Page.cpp:1367
> > +    forEachDocument([] (Document& document) {
> > +        document.updateCursorIfNeeded();
> > +    });
> 
> Shouldn't this happen after anything that can change style? If style is
> changed in a rAF callback, it should be reflected.

That makes sense, I'll address this in a followup.

> >>> LayoutTests/fast/events/mouse-cursor-no-mousemove.html:52
> >>> +    await new Promise(resolve => setTimeout(resolve, 100));
> >> 
> >> Why not use rAF here too?
> > 
> > This part of the test tests an ancient change where a synthetic "mousemove" was seemingly dispatched when changing "cursor" styles and we want to make sure it doesn't happen. I'm not sure what a good timeout is here because I don't know what the old timer was, but at any rate nothing should happen during that time. I just reduced it from by 50ms since this was the timer for the check that is now performed with a rAF.
> 
> await UIHelper.delayFor() or await UIHelper.animationFrame().

Why? Just changing one line with another line doesn't simplify the test, and I don't need to pull in another test file. This code is straightforward.

[Antoine Quint]

Reopening to attach new patch.

[Antoine Quint]

Created attachment 399371 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 399371 [details]
Patch

I think you should add a test that changes cursor style in rAF, and then uses internals.getCurrentCursorInfo to see if got the new cursor.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #16)
> Comment on attachment 399371 [details]
> Patch
> 
> I think you should add a test that changes cursor style in rAF, and then
> uses internals.getCurrentCursorInfo to see if got the new cursor.

Good call! Will do.

[Antoine Quint]

Committed r261741: <https://trac.webkit.org/changeset/261741>

[Ryan Haddad]

The test added with this change is consistently failing on iOS:

--- /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/fast/events/mouse-cursor-udpate-during-raf-expected.txt
+++ /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/fast/events/mouse-cursor-udpate-during-raf-actual.txt
@@ -5,13 +5,13 @@
 
 
 Moved pointer over the target, cursor should be Pointer.
-Cursor info: type=Pointer hotSpot=0,0
+Cursor info: FAIL: Cursor details not available on this platform.
 
 Setting cursor via CSS during next animation frame, cursor should be Pointer still.
-Cursor info: type=Pointer hotSpot=0,0
+Cursor info: FAIL: Cursor details not available on this platform.
 
 Waited until next run loop, cursor should be Help.
-Cursor info: type=Help hotSpot=0,0
+Cursor info: FAIL: Cursor details not available on this platform.
 
 PASS successfullyParsed is true

[Antoine Quint]

Oops, I guess this test makes no sense on iOS and needs to be skipped!

[Antoine Quint]

Committed r261760: <https://trac.webkit.org/changeset/261760>