Bug 228009 – Add key-driven smooth scrolling to macOS

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

[Dana Estra]

Add Smooth Scrolling to macOS

[Dana Estra]

Created attachment 433709 [details]
Patch

[Tim Horton]

Comment on attachment 433709 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:93
> +#include "ScrollAnimatorMac.h"

Can't do this, because this is cross-platform code.

> Source/WebCore/page/EventHandler.cpp:4314
> +    KeyboardScroll* scroll = new KeyboardScroll;

A manually-managed heap-allocated object is an odd choice for this; I would go with something more like the iOS implementation, which maintains a `std::optional<KeyboardScroll>` instead (unique_ptr<KeyboardScroll> would also be fine, but a raw pointer is bad news and easy to get wrong/cause leaks/etc.)

> Source/WebCore/platform/ScrollController.cpp:71
> +    setIsAnimatingSmoothScrolling(true);

Probably want to say "keyboard" most of the places you say "smooth"

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:70
> +    bool beginKeyboardScrollAnimation(KeyboardScroll*, KeyboardEvent&) override;
> +    void handleKeyUpEventWhileScrolling() override;
> +    void stopAnimatedScroll();

Possible we can just put our implementation on ScrollAnimator proper, instead of the mac specific subclass? I'm not sure, should see what smfr thinks. Generally cross-platform-first-unless-there's-a-reason-not-to is the WebKit approach, though.

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:173
> +    KeyboardScroll *m_currentScroll;

star's on the wrong side.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:96
> +    if (m_currentScroll) {

It would be fantastic to think about how to implement some of the physics in terms of free functions that can be shared between the two implementations (maybe a structure to represent the velocity/acceleration/current+ideal positions/etc., and some functions that are like "please move this state forward one tick (of this duration) given this KeyboardScroll", that only operate on inputs and return the new state, etc. (We can talk about the details, but we should get to the place where there's very little interesting code in WKKeyboardScrollingAnimator for example).

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:186
> +void ScrollAnimatorMac::stopAnimatedScroll()

Probably needs to have a name that is somehow about the keyboard

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:902
> +    delete m_currentScroll;

See above

[Aditya Keerthi]

Comment on attachment 433709 [details]
Patch

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

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:34
> +#include "FrameView.h"

I don't think this import is necessary?

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:158
> +        IntPoint newPosition = IntPoint((currentPosition() + m_currentScroll->offset).x(), (currentPosition() + m_currentScroll->offset).y());

This can be `auto newPosition = IntPoint(currentPosition() + m_currentScroll->offset);`.

[Dana Estra]

Created attachment 433938 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/80911857>

[Dana Estra]

Created attachment 433969 [details]
Patch

[Aditya Keerthi]

Comment on attachment 433969 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +                https://bugs.webkit.org/show_bug.cgi?id=228009

Fix indentation.

> Source/WebCore/page/EventHandler.cpp:-70
> -#include "KeyboardEvent.h"

I would keep the `KeyboardEvent` include, since it's still used in this file.

> Source/WebCore/platform/ScrollAnimator.cpp:35
> +#import "EventNames.h"

#include.

> Source/WebCore/platform/ScrollAnimator.cpp:333
> +    ScrollPosition idealPosition = m_scrollableArea.constrainScrollPosition(m_currentScroll ? IntPoint(currentPosition().x(), currentPosition().y()) :  IntPoint(m_idealPosition.x(), m_idealPosition.y()));

This can be written as: `ScrollPosition idealPosition = m_scrollableArea.constrainScrollPosition(IntPoint(m_currentScroll ? currentPosition() : m_idealPosition));`

> Source/WebCore/platform/ScrollController.cpp:-57
> -

Nit: Preserve this whitespace for readability.

> Source/WebCore/platform/ScrollController.h:171
> +    void updateKeyboardScrollingAnimatingState(MonotonicTime);

Nit: Add a newline after this line to preserve the existing organization.

[Simon Fraser (smfr)]

Comment on attachment 433969 [details]
Patch

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

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

I feel like KeyboardScrollParameters should be consulted inside the ScrollAnimator code, not here in event handling code.

> Source/WebCore/page/EventHandler.cpp:4323
> +    return view->scrollAnimator().beginKeyboardScrollAnimation(scroll, event);

Are we always beginning here, or might the animation already be running from a previous event?

> Source/WebCore/platform/ScrollAnimator.cpp:272
> +static BoxSide boxSide(ScrollDirection direction)

Needs a more descriptive name. boxSideInDirection ?boxSideForDirection?

> Source/WebCore/platform/ScrollAnimator.cpp:302
> +void ScrollAnimator::updateKeyboardScrollPosition(MonotonicTime currentTime)

Rather than this collection of functions on ScrollAnimator with "keyboard" in the name, maybe these functions should be in a separate class stored as a member variable (like ScrollController). You have KeyboardScroll but I'm not sure what that is.

> Source/WebCore/platform/ScrollAnimator.cpp:305
> +    FloatSize force = { 0, 0 };
> +    FloatSize axesToApplySpring = {1, 1};

Inconsistent spacing (the first one is right).
We also like to write this as
auto force = FloatSize { };

> Source/WebCore/platform/ScrollAnimator.cpp:310
> +        ScrollDirection direction = m_currentScroll->direction;

auto

> Source/WebCore/platform/ScrollAnimator.cpp:316
> +            // Apply the scrolling force. Only apply the spring in the perpendicular axis,
> +            // otherwise it drags against the direction of motion.
> +            axesToApplySpring = perpendicularAbsoluteUnitVector(direction);

Why is anything happening on the perpendicular axis? Wouldn't a keyboard scroll just be on one axis?

> Source/WebCore/platform/ScrollAnimator.cpp:406
> +    KeyboardScrollParameters params = KeyboardScrollParameters::parameters();

auto

> Source/WebCore/platform/ScrollAnimator.h:180
> +    RectEdges<bool> scrollableDirectionsFromOffset(FloatPoint);

const function

> Source/WebCore/platform/ScrollAnimator.h:191
> +    std::optional<WebCore::KeyboardScroll> m_currentScroll;

m_currentKeyboardScroll

[Tim Horton]

Comment on attachment 433969 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:4323
>> +    return view->scrollAnimator().beginKeyboardScrollAnimation(scroll, event);
> 
> Are we always beginning here, or might the animation already be running from a previous event?

It's the beginning /of a scroll/ (or a scroll gesture, or whatever you want to call it), but you're right, the animation might already be running.

>> Source/WebCore/platform/ScrollAnimator.cpp:316
>> +            axesToApplySpring = perpendicularAbsoluteUnitVector(direction);
> 
> Why is anything happening on the perpendicular axis? Wouldn't a keyboard scroll just be on one axis?

(It took me a while to re-figure this out: while the key is down, the spring is not engaged in the axis of scrolling motion (instead, the force comes from the key), but we still want to let previous animations that might have stretched the string perpendicular to the current animation keep running to completion.)

[Dana Estra]

Created attachment 434141 [details]
Patch

[Dana Estra]

Created attachment 434144 [details]
Patch

[Tim Horton]

Comment on attachment 434144 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:4263
> +    return view->scrollAnimator().keyboardScrollingAnimator()->beginKeyboardScrollGesture(event);

If keyboardScrollingAnimator is guaranteed to be non-null, this getter should return a reference, and if not, you probably need to null-check it?

> Source/WebCore/platform/KeyboardScrollingAnimator.h:36
> +    WTF_MAKE_FAST_ALLOCATED;

probably also non-copyable

> Source/WebCore/platform/KeyboardScrollingAnimator.h:47
> +    std::optional<KeyboardScroll> keyboardScrollForKeyboardEvent(KeyboardEvent&);

const?

> Source/WebCore/platform/KeyboardScrollingAnimator.h:48
> +    float scrollDistance(ScrollDirection, ScrollGranularity);

const?

> Source/WebCore/platform/ScrollAnimator.cpp:71
> +    , m_keyboardScrollingAnimator(makeUnique<KeyboardScrollingAnimator>(*this, m_scrollController))

Any reason KeyboardScrollingAnimator isn't just a member (instead of a unique_ptr)? (either way, you can make the getter return a reference)

[Simon Fraser (smfr)]

Comment on attachment 434144 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:3809
> +    if (event.type() == eventNames().keyupEvent) {
> +        FrameView* view = m_frame.view();
> +        return view->scrollAnimator().keyboardScrollingAnimator()->handleKeyUpEventWhileScrolling();
> +    }

It's a bit weird that the call to beginKeyboardScrollGesture() is hidden down in handleKeyboardScrolling but handleKeyUpEventWhileScrolling() is handled here. Would be nice to make them more symmetrical.

> Source/WebCore/platform/KeyboardScrollingAnimator.cpp:98
> +

Remove this blank line.

> Source/WebCore/platform/KeyboardScrollingAnimator.cpp:104
> +

Same

> Source/WebCore/platform/KeyboardScrollingAnimator.cpp:121
> +    auto springForce = - displacement.scaled(params.springStiffness) - m_velocity.scaled(params.springDamping);

No space after the leading '-'

> Source/WebCore/platform/KeyboardScrollingAnimator.cpp:158
> +    RELEASE_ASSERT_NOT_REACHED();

No need for this to be a RELEASE assert. Just return 0.

> Source/WebCore/platform/KeyboardScrollingAnimator.cpp:196
> +        RELEASE_ASSERT_NOT_REACHED();

don't release assert.

> Source/WebCore/platform/KeyboardScrollingAnimator.cpp:212
> +        RELEASE_ASSERT_NOT_REACHED();

don't release assert.

[Dana Estra]

Created attachment 434249 [details]
Patch

[Tim Horton]

Comment on attachment 434249 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:3807
> +    if (event.type() == eventNames().keyupEvent)
> +        stopKeyboardScrolling();

It is odd that this one doesn't follow the pattern of the others (passing it to editor first, bailing if it ate the event). On the other hand, we know that we never got here with keyup before, so I assume editor's handleKeyboardEvent doesn't do anything with keyup? (please check) But in that case, maybe it's best to follow the pattern anyway to minimize surprise later. Would be interested to hear other people's opinions.

> Source/WebCore/platform/KeyboardScrollingAnimator.h:42
> +    void handleKeyUpEventWhileScrolling();

The caller doesn't actually know that this is "while scrolling", right, they're just plumbing us every keyup? So maybe drop to just "handleKeyUpEvent". The class name indicates who's handling it and why sufficiently.

[Dana Estra]

Created attachment 434452 [details]
Patch

[Dana Estra]

Created attachment 434649 [details]
Patch

[EWS]

Committed r280492 (240124@main): <https://commits.webkit.org/240124@main>

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

[Fujii Hironori]

This change introduce a new assertion failure for Windows ports.
Filed: Bug 228690 – [Win] ASSERTION FAILED: evt->type() == eventNames().keydownEvent || evt->type() == eventNames().keypressEvent in WebView::interpretKeyEvent since r280492

[Dana Estra]

Reopening to attach new patch.

[Dana Estra]

Created attachment 434941 [details]
Patch

[Fujii Hironori]

Comment on attachment 434649 [details]
Patch

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

> Source/WebCore/platform/KeyboardScrollingAnimator.cpp:126
> +    m_velocity += acceleration.scaled(frameDuration);

I have a problem for this code. Bug 229697 – keyboard scrolling spring damping animation doesn't finish in WinCairo Debug with debug logging