Bug 229784 – KeyboardEvent should setDefaultHandled if EventHandler::startKeyboardScrolling returns true

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Fujii Hironori]

KeyboardEvent should setDefaultHandled if EventHandler::startKeyboardScrolling returns true

[Fujii Hironori]

Created attachment 437094 [details]
WIP patch

[Darin Adler]

Can we create a test for this?

Given this is an improvement, I imagine the improvement is detectable, and we should add a test.

[Fujii Hironori]

Yup, that's what I want to do now.

[Fujii Hironori]

Created attachment 437101 [details]
WIP patch

[Fujii Hironori]

Created attachment 437246 [details]
WIP Patch

[Fujii Hironori]

Created attachment 437385 [details]
Patch

[Tim Horton]

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 :)

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 437572 [details]
Patch

[Darin Adler]

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?

[Fujii Hironori]

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

[Darin Adler]

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.

[Fujii Hironori]

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?

[Darin Adler]

(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.

[Fujii Hironori]

Comment on attachment 437572 [details]
Patch

Clearing flags on attachment: 437572

Committed r282165 (241457@main): <https://commits.webkit.org/241457@main>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/82887017>