Bug 122985 - Rubber-banding is often not smooth on infinitely scrolling websites
Summary: Rubber-banding is often not smooth on infinitely scrolling websites
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-17 12:02 PDT by Beth Dakin
Modified: 2013-10-18 12:10 PDT (History)
11 users (show)

See Also:


Attachments
Patch (5.69 KB, patch)
2013-10-17 12:12 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff
Patch (4.95 KB, patch)
2013-10-17 15:41 PDT, Beth Dakin
no flags 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-10-17 12:02:20 PDT
Often if you are rubber-banding at the bottom of a page on a website like Facebook, Twitter, or Pinterest while there is more content loading, the normally-smooth rubber-band animation becomes very jarring and it will snap to a new location. This is unfortunate.
Comment 1 Beth Dakin 2013-10-17 12:12:10 PDT
Created attachment 214484 [details]
Patch
Comment 2 Simon Fraser (smfr) 2013-10-17 13:54:39 PDT
Comment on attachment 214484 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:66
> +    if (state->hasChangedProperty(ScrollingStateScrollingNode::TotalContentsSize)) {
> +        if (scrollingTree()->isRubberBandInProgress())
> +            m_totalContentsSizeForRubberBand = m_totalContentsSize;
> +        else
> +            m_totalContentsSizeForRubberBand = state->totalContentsSize();

Hmm, I think it might be cleaner to just store m_totalContentsSize in m_totalContentsSizeForRubberBand at the start of a rubberband, and unconditionally use m_totalContentsSizeForRubberBand here.
Comment 3 Beth Dakin 2013-10-17 15:41:32 PDT
Created attachment 214524 [details]
Patch

Simon's suggestion seems to work well.
Comment 4 Simon Fraser (smfr) 2013-10-17 15:53:04 PDT
Comment on attachment 214524 [details]
Patch

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

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:276
> +    // Since the rubberband timer has stopped, totalContentsSizeForRubberBand can be synchronized with totalContentsSize.
> +    setTotalContentsSizeForRubberBand(totalContentsSize());

If it weren't for maximumScrollPosition, it would be OK to let m_totalContentsSizeForRubberBand go stale, right?

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:388
> +    IntPoint position(totalContentsSizeForRubberBand().width() - viewportRect().width(),
> +                      totalContentsSizeForRubberBand().height() - viewportRect().height());

Hmm, there's no guarantee that this is being called during a rubberband, right? Maybe totalContentSize should be passed in?
Comment 5 Beth Dakin 2013-10-17 15:56:18 PDT
(In reply to comment #4)
> (From update of attachment 214524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214524&action=review
> 
> > Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:276
> > +    // Since the rubberband timer has stopped, totalContentsSizeForRubberBand can be synchronized with totalContentsSize.
> > +    setTotalContentsSizeForRubberBand(totalContentsSize());
> 
> If it weren't for maximumScrollPosition, it would be OK to let m_totalContentsSizeForRubberBand go stale, right?
> 

Yes, but see below.

> > Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:388
> > +    IntPoint position(totalContentsSizeForRubberBand().width() - viewportRect().width(),
> > +                      totalContentsSizeForRubberBand().height() - viewportRect().height());
> 
> Hmm, there's no guarantee that this is being called during a rubberband, right? Maybe totalContentSize should be passed in?

Yeah, I suppose it's not guaranteed. Right now we happen to only call this during a rubber band, but I can reasonably foresee reasons why people might add calls to it at other times…which kind of makes me like my original patch better since that would ensure that totalContentsSizeForRubberBand() was always as up-to-date as possible.
Comment 6 Beth Dakin 2013-10-18 12:10:42 PDT
Thanks! Simon and I feel like there should ultimately be a state machine that actually keeps track of whether or not you are in the middle of a rubber band. That will make all of this code a lot clearer. But for the time being, this  will do: http://trac.webkit.org/changeset/157642