KeyboardEvent should setDefaultHandled if EventHandler::startKeyboardScrolling returns true
Created attachment 437094 [details] WIP patch
Can we create a test for this? Given this is an improvement, I imagine the improvement is detectable, and we should add a test.
Yup, that's what I want to do now.
Created attachment 437101 [details] WIP patch
Created attachment 437246 [details] WIP Patch
Created attachment 437385 [details] Patch
Comment on attachment 437385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437385&action=review > LayoutTests/fast/scrolling/keyboard-scrolling-last-timestamp.html:19 > + await UIHelper.delayFor(500); Is there any way to write this test without a 500ms pause? We generally try to avoid this at the scale of 60k tests :)
Comment on attachment 437385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437385&action=review >> LayoutTests/fast/scrolling/keyboard-scrolling-last-timestamp.html:19 >> + await UIHelper.delayFor(500); > > Is there any way to write this test without a 500ms pause? We generally try to avoid this at the scale of 60k tests :) Good point. UIHelper.waitForTargetScrollAnimationToSettle seems the one I want.
Created attachment 437572 [details] Patch
Comment on attachment 437572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437572&action=review > LayoutTests/ChangeLog:9 > + * fast/scrolling/keyboard-scrolling-last-timestamp-expected.txt: Added. > + * fast/scrolling/keyboard-scrolling-last-timestamp.html: Added. What happens in this test if default handled is not set?
Comment on attachment 437572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437572&action=review >> LayoutTests/ChangeLog:9 >> + * fast/scrolling/keyboard-scrolling-last-timestamp.html: Added. > > What happens in this test if default handled is not set? internals.lastHandledUserGestureTimestamp() didn't change by the key events before my change. EventHandler::keyEvent updates lastHandledUserGestureTimestamp if the default hander handled. https://github.com/WebKit/WebKit/blob/292ba371d52e1291c750185a82c7ce04e231e508/Source/WebCore/page/EventHandler.cpp#L3490
Comment on attachment 437572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437572&action=review >>> LayoutTests/ChangeLog:9 >>> + * fast/scrolling/keyboard-scrolling-last-timestamp.html: Added. >> >> What happens in this test if default handled is not set? > > internals.lastHandledUserGestureTimestamp() didn't change by the key events before my change. > > EventHandler::keyEvent updates lastHandledUserGestureTimestamp if the default hander handled. > https://github.com/WebKit/WebKit/blob/292ba371d52e1291c750185a82c7ce04e231e508/Source/WebCore/page/EventHandler.cpp#L3490 Creating finding that. I guess it’s hard to test since "defaultHandled" is an internal thing, not directly visible to the web platform. I suppose we primarily can detect it by seeing that certain default behavior didn’t happen.
Comment on attachment 437572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437572&action=review >>>> LayoutTests/ChangeLog:9 >>>> + * fast/scrolling/keyboard-scrolling-last-timestamp.html: Added. >>> >>> What happens in this test if default handled is not set? >> >> internals.lastHandledUserGestureTimestamp() didn't change by the key events before my change. >> >> EventHandler::keyEvent updates lastHandledUserGestureTimestamp if the default hander handled. >> https://github.com/WebKit/WebKit/blob/292ba371d52e1291c750185a82c7ce04e231e508/Source/WebCore/page/EventHandler.cpp#L3490 > > Creating finding that. I guess it’s hard to test since "defaultHandled" is an internal thing, not directly visible to the web platform. I suppose we primarily can detect it by seeing that certain default behavior didn’t happen. I don't understand. How can I do that? I think TestWebKitAPI can test this change by using WKPageUIClientV0::didNotHandleKeyEvent. Is it better?
(In reply to Fujii Hironori from comment #13) > I don't understand. How can I do that? I don’t know if you can. > I think TestWebKitAPI can test this change by using > WKPageUIClientV0::didNotHandleKeyEvent. Is it better? Probably better in some ways, but this is also OK.
Comment on attachment 437572 [details] Patch Clearing flags on attachment: 437572 Committed r282165 (241457@main): <https://commits.webkit.org/241457@main>
All reviewed patches have been landed. Closing bug.
<rdar://problem/82887017>