Bug 226986 – Refactor MacOS keyboard scrolling and use KeyboardScroll struct

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Estra]

Renavigate MacOS keyboard scrolling and use KeyboardScroll struct

[Dana Estra]

Created attachment 431359 [details]
Patch

[Tim Horton]

Comment on attachment 431359 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:4302
> +    else if (event.keyIdentifier() == "PageDown")
> +        key = Key::PageDown;
> +    else if (event.keyIdentifier() == "PageUp")
> +        key = Key::PageUp;

PageDown is not going to get to defaultArrowEventHandler, likely this all needs to be factored out and called from multiple places (some kind of "maybe handle keyboard scroll" function)

> Source/WebCore/page/EventHandler.cpp:4344
> +    WebCore::KeyboardScroll scroll;
> +
> +    scroll.offset = unitVector(direction).scaled(scrollDistance);
> +    scroll.increment = increment;
> +    scroll.direction = direction;
> +    scroll.maximumVelocity = scroll.offset.scaled(this->parameters().maximumVelocityMultiplier);
> +
> +    scroll.force = scroll.maximumVelocity.scaled(this->parameters().springMass / this->parameters().timeToMaximumVelocity);

Maybe we can add KeyboardScroll in a second patch? First patch computes the bits but calls scrollRecursively directly, second patch introduces KeyboardScroll for the arguments.

[Tim Horton]

(In reply to Tim Horton from comment #2)
> Comment on attachment 431359 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431359&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:4302
> > +    else if (event.keyIdentifier() == "PageDown")
> > +        key = Key::PageDown;
> > +    else if (event.keyIdentifier() == "PageUp")
> > +        key = Key::PageUp;
> 
> PageDown is not going to get to defaultArrowEventHandler, likely this all
> needs to be factored out and called from multiple places (some kind of
> "maybe handle keyboard scroll" function)

Also don't forget about the spacebar.

[Dana Estra]

Created attachment 431361 [details]
Patch

[Aditya Keerthi]

You'll need to add 'KeyboardScroll.h' to the Xcode project.

[Darin Adler]

Comment on attachment 431361 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Renavigate MacOS keyboard scrolling and use KeyboardScroll struct

What is "Renavigate"?

[Dana Estra]

(In reply to Darin Adler from comment #6)
> Comment on attachment 431361 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431361&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Renavigate MacOS keyboard scrolling and use KeyboardScroll struct
> 
> What is "Renavigate"?

I couldn't think of a better word. I meant that I am changing the route through functions that is taken to make keyboard scrolling happen.

[Dana Estra]

Created attachment 431719 [details]
Patch

[Tim Horton]

Comment on attachment 431719 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:461
> +    // // First give accessibility a chance to handle the event.
> +    // Frame* frame = frameForEvent(event);
> +    // frame->eventHandler().handleKeyboardSelectionMovementForAccessibility(*event);
> +    // if (event->defaultHandled())
> +    //     return true;

Probably need to leave THIS code alone.

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:468
> +    // if (selector == "moveUp:")

Instead of leaving these commented out, I think you should add an off-by-default (until you're further along) setting (see WebPreferences*.yaml) or maybe a compile-time flag (see PlatformEnable.h); here you'll use it to guard the ones you don't want to be evaluated as "non-editing editing command behaviors" anymore, and will bail in EventHandler::handleKeyboardScrolling.

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:487
> +    // else if (selector == "moveToLeftEndOfLine:")
> +    //     didPerformAction = m_userInterfaceLayoutDirection == WebCore::UserInterfaceLayoutDirection::LTR ? m_page->backForward().goBack() : m_page->backForward().goForward();

For now, you've not moved goForward/goBack, so need to put this one back

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:493
> +    //     didPerformAction = m_userInterfaceLayoutDirection == WebCore::UserInterfaceLayoutDirection::LTR ? m_page->backForward().goForward() : m_page->backForward().goBack();

For now, you've not moved goForward/goBack, so need to put this one back

[Tim Horton]

Comment on attachment 431719 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:4244
> +const WebCore::KeyboardScrollParameters& EventHandler::parameters()
> +{
> +    static const KeyboardScrollParameters parameters;
> +    return parameters;
> +}

I think KeyboardScrollParameters could maybe hang on a static method off KeyboardScroll? EventHandler is not the right place, anyway

> Source/WebCore/page/EventHandler.h:379
> +

Too many newlines around this paragraph

> Source/WebCore/page/EventHandler.h:380
> +    const WebCore::KeyboardScrollParameters& parameters();

Not sure why this is here. Don't think it needs to be.

> Source/WebCore/page/KeyboardScroll.h:34
> +enum class ScrollingIncrement : uint8_t {
> +    Line,
> +    Page,
> +    Document
> +};

Instead of this, you should just make all this code use ScrollTypes.h's ScrollGranularity

> Source/WebCore/page/KeyboardScroll.h:41
> +enum class ScrollingDirection : uint8_t {
> +    Up,
> +    Down,
> +    Left,
> +    Right
> +};

Instead of this, you should just make all this code use ScrollTypes.h's ScrollDirection

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:783
> +    

Should be able to revert this and not touch this file at all :D

[Aditya Keerthi]

Comment on attachment 431719 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Renavigate MacOS keyboard scrolling and use KeyboardScroll struct

Nit: Update to match bug title.

> Source/WebCore/page/EventHandler.cpp:4193
> +    

You should configure your editor to remove trailing spaces :)

> Source/WebCore/page/EventHandler.cpp:4209
> +    

Nit: Unnecessary newline.

> Source/WebCore/page/EventHandler.cpp:4250
> +        return {0, -1};

Nit: Add spaces around the values, like `{ 0, -1 };`.

> Source/WebCore/page/EventHandler.cpp:4260
> +CGFloat EventHandler::ScrollDistance(WebCore::ScrollingDirection direction, WebCore::ScrollingIncrement increment)

Nit: `scrollDistance`.

> Source/WebCore/page/EventHandler.cpp:4263
> +    FrameView* view = frame->view();

`m_frame.view()`. No need for the local variable.

> Source/WebCore/page/EventHandler.cpp:4351
> +    scroll.maximumVelocity = scroll.offset.scaled(this->parameters().maximumVelocityMultiplier);
> +
> +    scroll.force = scroll.maximumVelocity.scaled(this->parameters().springMass / this->parameters().timeToMaximumVelocity);

Remove `this`.

[Radar WebKit Bug Importer]

<rdar://problem/79577430>

[Dana Estra]

Created attachment 431916 [details]
Patch

[Dana Estra]

Created attachment 431918 [details]
Patch

[Dana Estra]

Created attachment 431919 [details]
Patch

[Dana Estra]

Created attachment 431979 [details]
Patch

[Tim Horton]

Comment on attachment 431979 [details]
Patch

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

Need to get the bots not red and clean up some random files that you don't need to touch, but this seems pretty much ready to go.

> Source/WebCore/page/EventHandler.cpp:4192
> -
> +    

What editor are you using that is making all these spurious changes :) If it's Xcode, check "including white-space only lines" in the "editing" tab of "text editing"

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:315
> +        eventWasHandled = executeKeypressCommandsInternal(commands, &event); 

?

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:456
> +    // // First give accessibility a chance to handle the event.

What's this!

[Aditya Keerthi]

Comment on attachment 431979 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Renavigate MacOS keyboard scrolling and use KeyboardScroll struct

Nit: You can remove this line.

It would be nice to summarize the refactoring here (and in the WebKit changelog).

> Source/WebCore/page/EventHandler.cpp:-3802
> -

Nit: Undo this change.

> Source/WebCore/page/EventHandler.cpp:4192
> +    

Nit: Remove trailing whitespace (also a couple more instances below).

> Source/WebCore/page/EventHandler.cpp:4206
> +    if (m_frame.settings().smoothKeyboardScrollingEnabled()) {

This could be a ternary: `m_frame.settings().smoothKeyboardScrollingEnabled() ? handleKeyboardScrolling(event) : view->logicalScroll(direction, ScrollByPage)`

> Source/WebCore/page/EventHandler.cpp:4338
> +    WebCore::KeyboardScroll scroll;

No need for `WebCore::`, since you're in the WebCore namespace.

> Source/WebCore/page/KeyboardScroll.h:28
> +#pragma once
> +#include "ScrollTypes.h"
> +namespace WebCore {

Nit: Add some newlines here.

```
#pragma once

#include "ScrollTypes.h"

namespace WebCore {
```

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:456
> +    // // First give accessibility a chance to handle the event.

Nit: Remove added `//`.

[Aditya Keerthi]

Comment on attachment 431979 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:4304
> +    auto granularity = ^() {

Use lambda syntax (`[] {`) rather than block syntax since this is a C++ file.

> Source/WebCore/page/EventHandler.cpp:4321
> +    auto direction = ^() {

Ditto.

[Dana Estra]

Created attachment 431992 [details]
Patch

[Dana Estra]

Created attachment 431997 [details]
Patch

[Sam Weinig]

Comment on attachment 431997 [details]
Patch

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

Seems like there is a bunch of code duplication going on. Instead, it would be better to make the code useable by both WKKeyboardScrollingAnimator and EventHandler, and have those classes called shared functionality.

> Source/WebCore/ChangeLog:8
> +        Renavigate MacOS keyboard scrolling and use KeyboardScroll struct

I don't think you mean Renavigate here. Refactor?

> Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:696
> +   default: false

Given we do support smooth keyboard scrolling on iOS, this seems like an incorrect default. If this setting does not effect the iOS functionality, we should either update things so this does, or rename this to be more clear about what it does.

> Source/WebCore/page/EventHandler.cpp:4240
> +FloatSize EventHandler::unitVector(ScrollDirection direction)

This seems like it should be a free function rather than a static class function.

> Source/WebCore/page/EventHandler.cpp:4254
> +CGFloat EventHandler::scrollDistance(ScrollDirection direction, ScrollGranularity granularity)

We try to avoid using platform specific types like CGFloat in platform independent files if we can avoid it. This should just return a float or double.

> Source/WebCore/page/EventHandler.cpp:4261
> +    Scrollbar* scrollbar;
> +
> +    if (direction == ScrollDirection::ScrollUp || direction == ScrollDirection::ScrollDown) 
> +        scrollbar = m_frame.view()->verticalScrollbar();
> +    else 
> +        scrollbar = m_frame.view()->horizontalScrollbar();

A trick we do for things like this with an uninitialized variable is to use an anonymous lambda that is called immediately.  
auto scrollbar = [&] {
    if (direction == ScrollDirection::ScrollUp || direction == ScrollDirection::ScrollDown) 
        return m_frame.view()->verticalScrollbar();
     return m_frame.view()->horizontalScrollbar();
}();

> Source/WebCore/page/EventHandler.cpp:4278
> +    switch (granularity) {
> +    case ScrollGranularity::ScrollByLine:
> +        step = scrollbar->lineStep();
> +        break;
> +    case ScrollGranularity::ScrollByPage:
> +        step = scrollbar->pageStep();
> +        break;
> +    case ScrollGranularity::ScrollByDocument:
> +        step = scrollbar->totalSize();
> +        break;
> +    case ScrollGranularity::ScrollByPixel:
> +        step = scrollbar->pixelStep();
> +        break;
> +    }

These should just return the value directly.

> Source/WebCore/page/EventHandler.cpp:4301
> +    auto granularity = ^() {

This should use a c++ lambda, not a block. Blocks should only be used when lambdas absolutely cannot.

> Source/WebCore/page/EventHandler.cpp:4330
> +        };

; is not needed here.

> Source/WebCore/page/KeyboardScroll.h:28
> +#pragma once
> +#include "ScrollTypes.h"
> +namespace WebCore {

Please add newlines between these.

> Source/WebCore/page/KeyboardScroll.h:33
> +    WebCore::FloatSize offset; // Points per increment.
> +    WebCore::FloatSize maximumVelocity; // Points per second.
> +    WebCore::FloatSize force;

WebCore:: are not needed.

> Source/WebCore/page/KeyboardScroll.h:47
> +    CGFloat springMass { 1 };
> +    CGFloat springStiffness { 109 };
> +    CGFloat springDamping { 20 };
> +
> +    CGFloat maximumVelocityMultiplier { 25 };
> +    CGFloat timeToMaximumVelocity { 1 };
> +
> +    CGFloat rubberBandForce { 5000 };

Should not be using CGFloat here. Use float instead.

Do these ever change? Seems weird to have these constants just sitting here as default parameters. If we think they might vary, perhaps they should be on Settings? If not, perhaps just some constexprs would suffice?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:48
> +- (CGFloat)distanceForIncrement:(ScrollGranularity)increment inDirection:(ScrollDirection)direction;

I would add the explicit WebCore:: here for the types.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:127
> +    case ScrollDirection::ScrollUp:
>          return { 0, -1 };
> -    case WebKit::ScrollingDirection::Down:
> +    case ScrollDirection::ScrollDown:
>          return { 0, 1 };
> -    case WebKit::ScrollingDirection::Left:
> +    case ScrollDirection::ScrollLeft:
>          return { -1, 0 };
> -    case WebKit::ScrollingDirection::Right:
> +    case ScrollDirection::ScrollRight:

This seems like it is a duplicate of the code in WebCore. Why not just use that?

[Dana Estra]

Created attachment 432012 [details]
Patch

[Tim Horton]

(In reply to Sam Weinig from comment #22)
> Comment on attachment 431997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431997&action=review
> 
> Seems like there is a bunch of code duplication going on. Instead, it would
> be better to make the code useable by both WKKeyboardScrollingAnimator and
> EventHandler, and have those classes called shared functionality.

Ideally, yes. In practice this is a bit annoying, because the input to this function is an event, and I don't think there is any event type that is *commonly* used both in WebCore in the Web Content process and WebKit in the UI process. Ideally we could make the code deal in terms of WebCore events (WebCore::KeyboardEvent), but it's a bit weird to use the DOMmy KeyboardEvent in the UI process, no?

I think it might be better if Dana doesn't try to solve this all the way in the first patch, since it will involve lots of thoughtful refactoring of the iOS code. But there is a good bit of sharing that she /can/ do; e.g. `unitVector` like you said, also the code to fill out a KeyboardScroll instance once you have a ScrollGranularity+ScrollDirection.

> > Source/WebCore/page/KeyboardScroll.h:47
> > +    CGFloat springMass { 1 };
> > +    CGFloat springStiffness { 109 };
> > +    CGFloat springDamping { 20 };
> > +
> > +    CGFloat maximumVelocityMultiplier { 25 };
> > +    CGFloat timeToMaximumVelocity { 1 };
> > +
> > +    CGFloat rubberBandForce { 5000 };
> 
> Do these ever change? Seems weird to have these constants just sitting here
> as default parameters. If we think they might vary, perhaps they should be
> on Settings? If not, perhaps just some constexprs would suffice?

This was my doing; back in the day they used to be adjustable with some UI, and I never constexpr'd them. Dana, you're welcome to do so, though I don't think it's vital for this patch.

> > Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:127
> > +    case ScrollDirection::ScrollUp:
> >          return { 0, -1 };
> > -    case WebKit::ScrollingDirection::Down:
> > +    case ScrollDirection::ScrollDown:
> >          return { 0, 1 };
> > -    case WebKit::ScrollingDirection::Left:
> > +    case ScrollDirection::ScrollLeft:
> >          return { -1, 0 };
> > -    case WebKit::ScrollingDirection::Right:
> > +    case ScrollDirection::ScrollRight:
> 
> This seems like it is a duplicate of the code in WebCore. Why not just use
> that?

Agreed.

[Sam Weinig]

(In reply to Tim Horton from comment #24)
> (In reply to Sam Weinig from comment #22)
> > Comment on attachment 431997 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=431997&action=review
> > 
> > Seems like there is a bunch of code duplication going on. Instead, it would
> > be better to make the code useable by both WKKeyboardScrollingAnimator and
> > EventHandler, and have those classes called shared functionality.
> 
> Ideally, yes. In practice this is a bit annoying, because the input to this
> function is an event, and I don't think there is any event type that is
> *commonly* used both in WebCore in the Web Content process and WebKit in the
> UI process. Ideally we could make the code deal in terms of WebCore events
> (WebCore::KeyboardEvent), but it's a bit weird to use the DOMmy
> KeyboardEvent in the UI process, no?

I would solve this with a new struct that contains the input to the algorithm that both event types can be be constructed from or adding a shared interface for both event types.

> 
> I think it might be better if Dana doesn't try to solve this all the way in
> the first patch, since it will involve lots of thoughtful refactoring of the
> iOS code. But there is a good bit of sharing that she /can/ do; e.g.
> `unitVector` like you said, also the code to fill out a KeyboardScroll
> instance once you have a ScrollGranularity+ScrollDirection.

Yeah, let's do what sharing we can. Free functions on the platform types will go a long way.

[Dana Estra]

Created attachment 432194 [details]
Patch

[Aditya Keerthi]

Comment on attachment 432194 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No tests yet

Nit: Add a period at the end of the sentence.

Also might be good to summarize the changes.

> Source/WebCore/page/EventHandler.cpp:4237
> +static FloatSize unitVector(ScrollDirection direction)

The IOS_FAMILY build is failing because this method is not in a header (and is not WEBCORE_EXPORT).

I think you should move this to a new KeyboardScroll.cpp, and WEBCORE_EXPORT the method in KeyboardScroll.h. And probably rename the method to unitVectorForScrollDirection.

You'll need to add the new file to Source/WebCore/Sources.txt (and we should probably add the header to Source/WebCore/Headers.cmake).

> Source/WebCore/page/EventHandler.cpp:4332
> +    CGFloat distance = scrollDistance(direction, granularity);
> +    
> +    KeyboardScroll scroll;
> +
> +    scroll.offset = unitVector(direction).scaled(distance);
> +    scroll.granularity = granularity;
> +    scroll.direction = direction;
> +    scroll.maximumVelocity = scroll.offset.scaled(KeyboardScrollParameters::parameters().maximumVelocityMultiplier);
> +
> +    scroll.force = scroll.maximumVelocity.scaled(KeyboardScrollParameters::parameters().springMass / KeyboardScrollParameters::parameters().timeToMaximumVelocity);

It looks like none of these variables are actually used?

[Dana Estra]

Created attachment 432287 [details]
Patch

[Dana Estra]

Created attachment 432293 [details]
Patch

[Tim Horton]

Comment on attachment 432293 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:4259
> +    // FIXME: This logic does not account for writing-mode.

This should have a bugzilla number in parens (FIXME (123456): ...)

> Source/WebCore/page/EventHandler.cpp:4318
> +    KeyboardScroll scroll;
> +
> +    scroll.offset = unitVectorForScrollDirection(direction).scaled(distance);
> +    scroll.granularity = granularity;
> +    scroll.direction = direction;
> +    scroll.maximumVelocity = scroll.offset.scaled(KeyboardScrollParameters::parameters().maximumVelocityMultiplier);
> +
> +    scroll.force = scroll.maximumVelocity.scaled(KeyboardScrollParameters::parameters().springMass / KeyboardScrollParameters::parameters().timeToMaximumVelocity);

Not the end of the world, but ideally you would stash this unused code aside for patch 2 instead of including it here.

> Source/WebCore/page/KeyboardScroll.cpp:1
> +// Copyright (C) 2021 Apple Inc. All rights reserved.

This copyright header should look like the one in your .h (with a multi-line comment); not sure where this version came from.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:-299
> -    WebKit::KeyboardScroll scroll;

Should be a FIXME in this general vicinity about adopting the WebCore version of this.

[Dana Estra]

Created attachment 432423 [details]
Patch

[Darin Adler]

Comment on attachment 432423 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:489
> +        didPerformAction = scroll(m_page.get(), ScrollRight, ScrollByPage);

Wrong indentation here.

[Dana Estra]

Created attachment 432426 [details]
Patch

[Tim Horton]

Comment on attachment 432426 [details]
Patch

There's more we can do to clean up and deduplicate, but let's start here.

[EWS]

Committed r279355 (239223@main): <https://commits.webkit.org/239223@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432426 [details].