Bug 107628 - Sometimes scroll position is jerky during rubber-band, affects nytimes.com
Summary: Sometimes scroll position is jerky during rubber-band, affects nytimes.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-01-22 21:58 PST by Beth Dakin
Modified: 2013-01-23 12:18 PST (History)
9 users (show)

See Also:


Attachments
Patch (11.31 KB, patch)
2013-01-22 22:13 PST, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2013-01-22 21:58:43 PST
To repro this issue at nytimes.com

1. Navigate to nytimes.com
2. Using a two-finger scroll gesture, pull the web page down to reveal linen at the top. Hold your fingers in place to keep the linen visible for a while.

Expected result: The linen keeps showing for as long as your fingers are down on the trackpad.
Actual result: After a moment, the top of the page will snap up and the linen will disappear.

<rdar://problem/12679549>
Comment 1 Beth Dakin 2013-01-22 22:13:51 PST
Created attachment 184140 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-22 22:16:32 PST
Attachment 184140 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/page/scrolling/ScrollingTree.h:82:  The parameter name "isRubberBanding" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Simon Fraser (smfr) 2013-01-22 22:17:45 PST
Comment on attachment 184140 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1929
> +    if (Page* page = m_frame->page()) {
> +        if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> +            if (!scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread())
> +                return scrollingCoordinator->isRubberBandInProgress();
> +        }
> +    }
> +#endif
> +
> +    // If the main thread updates the scroll position for this FrameView, we should return
> +    // ScrollAnimator::isRubberBandInProgress().
> +    if (ScrollAnimator* scrollAnimator = existingScrollAnimator())
> +        return scrollAnimator->isRubberBandInProgress();

It's a shame that the ScrollingCoordinator and ScrollAnimator don't know about eachother, otherwise you could just ask one of them for the answer. It's pretty unwieldy to have to add code like this in several places.
Comment 4 James Robinson 2013-01-22 22:20:44 PST
Comment on attachment 184140 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1915
> +#if ENABLE(THREADED_SCROLLING)

why this compile-time guard?  The code this depends on (ScrollingCoordinator, etc) are all available outside this feature define and platforms that don't set this (say chromium) will still need this code
Comment 5 Beth Dakin 2013-01-23 11:36:17 PST
(In reply to comment #3)
> (From update of attachment 184140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184140&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1929
> > +    if (Page* page = m_frame->page()) {
> > +        if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> > +            if (!scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread())
> > +                return scrollingCoordinator->isRubberBandInProgress();
> > +        }
> > +    }
> > +#endif
> > +
> > +    // If the main thread updates the scroll position for this FrameView, we should return
> > +    // ScrollAnimator::isRubberBandInProgress().
> > +    if (ScrollAnimator* scrollAnimator = existingScrollAnimator())
> > +        return scrollAnimator->isRubberBandInProgress();
> 
> It's a shame that the ScrollingCoordinator and ScrollAnimator don't know about eachother, otherwise you could just ask one of them for the answer. It's pretty unwieldy to have to add code like this in several places.

Agreed. :-/
Comment 6 Beth Dakin 2013-01-23 11:38:09 PST
(In reply to comment #4)
> (From update of attachment 184140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184140&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1915
> > +#if ENABLE(THREADED_SCROLLING)
> 
> why this compile-time guard?  The code this depends on (ScrollingCoordinator, etc) are all available outside this feature define and platforms that don't set this (say chromium) will still need this code

Alright, I can remove it. I added it because the ScrollingCoordinator function does not do anything on Chromium right now, and didn't know if the bug existed in Chromium anyway.
Comment 7 James Robinson 2013-01-23 12:12:33 PST
(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 184140 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184140&action=review
> > 
> > > Source/WebCore/page/FrameView.cpp:1915
> > > +#if ENABLE(THREADED_SCROLLING)
> > 
> > why this compile-time guard?  The code this depends on (ScrollingCoordinator, etc) are all available outside this feature define and platforms that don't set this (say chromium) will still need this code
> 
> Alright, I can remove it. I added it because the ScrollingCoordinator function does not do anything on Chromium right now, and didn't know if the bug existed in Chromium anyway.

Rubber-banding isn't implemented at all in chromium's implementation of off-main-thread scrolling today, so an implementation of ScrollingCoordinator that returns false for isRubberBandInProgress() will be accurate until we fix that.
Comment 8 Beth Dakin 2013-01-23 12:18:41 PST
Thanks! http://trac.webkit.org/changeset/140571