Bug 122985 – Rubber-banding is often not smooth on infinitely scrolling websites

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

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.

[Beth Dakin]

Created attachment 214484 [details]
Patch

[Simon Fraser (smfr)]

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.

[Beth Dakin]

Created attachment 214524 [details]
Patch

Simon's suggestion seems to work well.

[Simon Fraser (smfr)]

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?

[Beth Dakin]

(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.

[Beth Dakin]

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