Bug 188239

Summary: Using the keyboard arrow keys to scroll a webpage is very slow, not smooth, takes too long
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, cying, mitz, simon.fraser, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Tim Horton 2018-08-01 14:44:01 PDT
Using the keyboard arrow keys to scroll a webpage is very slow, not smooth, takes too long
Comment 1 Tim Horton 2018-08-01 14:44:56 PDT
Created attachment 346302 [details]
Patch
Comment 2 Tim Horton 2018-08-01 14:45:43 PDT
<rdar://problem/22997654>
Comment 3 Tim Horton 2018-08-01 14:59:02 PDT
Created attachment 346304 [details]
Patch
Comment 4 Simon Fraser (smfr) 2018-08-01 15:34:24 PDT
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?
Comment 5 Tim Horton 2018-08-01 16:20:37 PDT
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.
Comment 6 Tim Horton 2018-08-01 16:24:47 PDT
Created attachment 346312 [details]
Patch
Comment 7 Tim Horton 2018-08-01 16:47:29 PDT
Created attachment 346313 [details]
Patch
Comment 8 WebKit Commit Bot 2018-08-01 17:30:17 PDT
Comment on attachment 346313 [details]
Patch

Clearing flags on attachment: 346313

Committed r234488: <https://trac.webkit.org/changeset/234488>
Comment 9 WebKit Commit Bot 2018-08-01 17:30:19 PDT
All reviewed patches have been landed.  Closing bug.