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 Bugs | Assignee: | 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
Tim Horton
2018-08-01 14:44:01 PDT
Created attachment 346302 [details]
Patch
Created attachment 346304 [details]
Patch
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 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. Created attachment 346312 [details]
Patch
Created attachment 346313 [details]
Patch
Comment on attachment 346313 [details] Patch Clearing flags on attachment: 346313 Committed r234488: <https://trac.webkit.org/changeset/234488> All reviewed patches have been landed. Closing bug. |