Using the keyboard arrow keys to scroll a webpage is very slow, not smooth, takes too long

[Tim Horton]

Using the keyboard arrow keys to scroll a webpage is very slow, not smooth, takes too long

[Tim Horton]

Created attachment 346302 [details]
Patch

[Tim Horton]

<rdar://problem/22997654>

[Tim Horton]

Created attachment 346304 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 346304 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3647
> +    if ([_keyboardScrollingAnimator handleKeyEvent:theEvent])
> +        return;

If a page does preventDefault on arrow key events, should it be able to override keyboard scrolling?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3753
> +        return false;

NO

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3756
> +        return false;

NO

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3758
> +    return true;

YES

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:114
> +    if ([charactersIgnoringModifiers isEqualToString:UIKeyInputUpArrow])

Blank line above please.

This code is pretty hard to read. Would be nice to be able to just have one call to computeOffset().

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:136
> +

Remove blank line.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:141
> +    _scrollOffsetPerIncrement = *offset;

or offset.value(); that doesn't make me go looking for pointers.

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:174
> +    _displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(displayLinkFired:)];

OK to not retain this?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:187
> +    const CFTimeInterval secondsPerScrollIncrement = 0.05; // Seconds it should take to cover one increment when at full speed.

Odd that this isn't some multiple of 16.667ms?

> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:188
> +    const float maximumVelocity = 2000; // Maximum velocity in pixels per second. Empirically determined.

How well does this work on differently sized devices?

[Tim Horton]

Comment on attachment 346304 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:114
>> +    if ([charactersIgnoringModifiers isEqualToString:UIKeyInputUpArrow])
> 
> Blank line above please.
> 
> This code is pretty hard to read. Would be nice to be able to just have one call to computeOffset().

I rewrote this, see how you like it.

>> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:174
>> +    _displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(displayLinkFired:)];
> 
> OK to not retain this?

It sits in a retainptr. This is autoreleased so it's all good.

>> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:187
>> +    const CFTimeInterval secondsPerScrollIncrement = 0.05; // Seconds it should take to cover one increment when at full speed.
> 
> Odd that this isn't some multiple of 16.667ms?

It is exactly 3 frames @ 60fps, actually! That was unintentional.

>> Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:188
>> +    const float maximumVelocity = 2000; // Maximum velocity in pixels per second. Empirically determined.
> 
> How well does this work on differently sized devices?

We'll probably need to tune this, because I don't know that what feels reasonable to me makes sense. That said, I don't think you hit this cap on iPhone, for example.

[Tim Horton]

Created attachment 346312 [details]
Patch

[Tim Horton]

Created attachment 346313 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 346313 [details]
Patch

Clearing flags on attachment: 346313

Committed r234488: <https://trac.webkit.org/changeset/234488>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.