[Webkit-unassigned] [Bug 228009] Add key-driven smooth scrolling to macOS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=228009

--- Comment #8 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 433969
  --> https://bugs.webkit.org/attachment.cgi?id=433969
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210722/70bd624c/attachment-0001.htm>