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.
<rdar://problem/63220368>
Should we be calling EventHandler::updateCursor() under Page::updateRendering() and schedule a page rendering instead of using m_cursorUpdateTimer.startOneShot(cursorUpdateInterval)?
Created attachment 399346 [details] Patch
Created attachment 399347 [details] Patch
Created attachment 399348 [details] Patch
Created attachment 399349 [details] Patch
*** Bug 209494 has been marked as a duplicate of this bug. ***
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?
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?
(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.
Committed r261686: <https://trac.webkit.org/changeset/261686>
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().
(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.
Reopening to attach new patch.
Created attachment 399371 [details] Patch
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.
(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.
Committed r261741: <https://trac.webkit.org/changeset/261741>
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
Oops, I guess this test makes no sense on iOS and needs to be skipped!
Committed r261760: <https://trac.webkit.org/changeset/261760>