Bug 229733 – REGRESSION(r280928) The smooth keyboard scrolling is unconditionally enabled for PageUp and PageDown keys

• Component: Scrolling
• Status: RESOLVED FIXED

[Fujii Hironori]

REGRESSION(r280928) The smooth keyboard scrolling is enabled only for PageUp and PageDown keys

Since: r280928 (Bug 228156)

1. Start WinCairo MiniBrowser
2. Press PageDown key

Actual: scolling smoothly
Expected: scolling not smoothly

NOTE: Space key and Arrow key don't trigger the smooth keyboard scrolling.

[Fujii Hironori]

Created attachment 436938 [details]
Patch

[Darin Adler]

Comment on attachment 436938 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:3812
> +        else if (m_frame.settings().eventHandlerDrivenSmoothKeyboardScrollingEnabled()
> +            && (event.keyIdentifier() == "PageUp" || event.keyIdentifier() == "PageDown"))
>              startKeyboardScrolling(event);

This looks like it will make PageUp and PageDown not scroll at all? Is that right?

[Fujii Hironori]

Comment on attachment 436938 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:3812
>>              startKeyboardScrolling(event);
> 
> This looks like it will make PageUp and PageDown not scroll at all? Is that right?

This change restores the previous behavior before r280928.
If the key event isn't consumed by WebCore's handleKeyEvent, WebKit's WebPage::performDefaultBehaviorForKeyEvent handles it.

[Tim Horton]

Comment on attachment 436938 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        REGRESSION(r280928) The smooth keyboard scrolling is enabled only for PageUp and PageDown keys

From the wording of the title ("enabled only for") I assumed the fix would be "enable it for more things". But actually it is fixing the fact that it was accidentally enabled unconditionally for pageup/down. Current title is fine, but a rewording could make it more clear. Regardless, this seems fine.

>>> Source/WebCore/page/EventHandler.cpp:3812
>>>              startKeyboardScrolling(event);
>> 
>> This looks like it will make PageUp and PageDown not scroll at all? Is that right?
> 
> This change restores the previous behavior before r280928.
> If the key event isn't consumed by WebCore's handleKeyEvent, WebKit's WebPage::performDefaultBehaviorForKeyEvent handles it.

Possible we should just move the setting check /inside/ startKeyboardScrolling?

[Fujii Hironori]

Comment on attachment 436938 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        REGRESSION(r280928) The smooth keyboard scrolling is enabled only for PageUp and PageDown keys
> 
> From the wording of the title ("enabled only for") I assumed the fix would be "enable it for more things". But actually it is fixing the fact that it was accidentally enabled unconditionally for pageup/down. Current title is fine, but a rewording could make it more clear. Regardless, this seems fine.

Fixed.

>>>> Source/WebCore/page/EventHandler.cpp:3812
>>>>              startKeyboardScrolling(event);
>>> 
>>> This looks like it will make PageUp and PageDown not scroll at all? Is that right?
>> 
>> This change restores the previous behavior before r280928.
>> If the key event isn't consumed by WebCore's handleKeyEvent, WebKit's WebPage::performDefaultBehaviorForKeyEvent handles it.
> 
> Possible we should just move the setting check /inside/ startKeyboardScrolling?

Good idea. Will fix.

[Fujii Hironori]

Created attachment 436971 [details]
Patch

[Darin Adler]

Comment on attachment 436971 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:3812
> +                event.setDefaultHandled();

What was calling setDefaultHandled here before? Change log doesn’t mention this change, and it’s a change that affects only the other ports, not WinCairo.

> Source/WebCore/page/EventHandler.cpp:4312
> +            event.setDefaultHandled();

Ditto.

[Fujii Hironori]

Comment on attachment 436971 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:3812
>> +                event.setDefaultHandled();
> 
> What was calling setDefaultHandled here before? Change log doesn’t mention this change, and it’s a change that affects only the other ports, not WinCairo.

Yes. It's another change. I'm going to split the patch for the part.

[Fujii Hironori]

Created attachment 436975 [details]
Patch

[Fujii Hironori]

Comment on attachment 436975 [details]
Patch

Clearing flags on attachment: 436975

Committed r281867 (241197@main): <https://commits.webkit.org/241197@main>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/82641834>

[Fujii Hironori]

(In reply to Fujii Hironori from comment #8)
> Yes. It's another change. I'm going to split the patch for the part.

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

[Adrian Perez]

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