Bug 190345 – Adjust keyboard scrolling animator to be a bit more physical

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Adjust keyboard scrolling animator to be a bit more physical

[Tim Horton]

Created attachment 351763 [details]
Patch

[Tim Horton]

I'll flesh out the changelog a bit. Also I think maybe we can unit test the class independent of a scroll view, which would be nice, so I'll try that.

[Tim Horton]

Created attachment 351794 [details]
Patch

[Tim Horton]

Created attachment 351795 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 351795 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Adjust keyboard scrolling animator to be a bit more physical

"a bit more"

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3854
> +        return self.bounds.size.height * selfScale;

Wouldn't that be the content view height?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3858
> +        return WebCore::Scrollbar::pixelsPerLineStep() * selfScale;

Maybe get this from the content view too?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:46
> +    WebCore::FloatSize offset; // Points per increment.
> +    WebCore::FloatSize maximumVelocity; // Points per second.

The comments indicate that the members could be named better (or maybe some typedef types)

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:142
> +        return WebCore::FloatSize(0, -1);

return { 0., -1. } ?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:292
> +        _velocity = WebCore::FloatSize();

= { }

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:380
> +    _velocity = WebCore::FloatSize();

= { }

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:391
> +        WebCore::RectEdges<bool> scrollableDirections = [_scrollable scrollableDirectionsFromOffset:_currentPosition];

auto

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:421
> +    // Compute the spring's force, and apply it in allowed directions.
> +    // F_spring = -k * x - c * v
> +    auto springForce = - displacement.scaled(self.parameters.springStiffness) - _velocity.scaled(self.parameters.springDamping);
> +    force += springForce * axesToApplySpring;

I kinda feel like all the spring logic should be packaged up in a separate helper class.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:435
> +        _velocity = WebCore::FloatSize();

= { }

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:580
> +    return CGSizeMake(scrollView._horizontalVelocity * 1000, scrollView._verticalVelocity * 1000);

Please document the * 1000

[Tim Horton]

Created attachment 351799 [details]
Patch

[Tim Horton]

Created attachment 351806 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351806 [details]
Patch

Clearing flags on attachment: 351806

Committed r236935: <https://trac.webkit.org/changeset/236935>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45101935>

[Daniel Bates]

(In reply to WebKit Commit Bot from comment #8)
> Comment on attachment 351806 [details]
> Patch
> 
> Clearing flags on attachment: 351806
> 
> Committed r236935: <https://trac.webkit.org/changeset/236935>

This regressed key repeats for arrow keys on iOS device. See bug #191408 for more details.