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.
Created attachment 436938 [details] Patch
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?
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.
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?
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.
Created attachment 436971 [details] Patch
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.
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.
Created attachment 436975 [details] Patch
Comment on attachment 436975 [details] Patch Clearing flags on attachment: 436975 Committed r281867 (241197@main): <https://commits.webkit.org/241197@main>
All reviewed patches have been landed. Closing bug.
<rdar://problem/82641834>
(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
*** Bug 232701 has been marked as a duplicate of this bug. ***