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.
Created attachment 214484 [details] Patch
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.
Created attachment 214524 [details] Patch Simon's suggestion seems to work well.
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?
(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.
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