Changeset 269559 in webkit


Ignore:
Timestamp:
Nov 6, 2020 9:38:37 PM (21 months ago)
Author:
Simon Fraser
Message:

A programmatic scroll to a new location should stop rubberbanding
​https://bugs.webkit.org/show_bug.cgi?id=218672

Reviewed by Tim Horton.

This is a better version of the fix in r269373. That fix attempted to stop a rubberband if there
was a programmatic scroll that moved you away from the edge where any active rubberband was happening.
However, the code ran too late; by the time ScrollController::scrollPositionChanged() is called,
the position has already changed, but more importantly the requested scroll position had
already been clamped between min and max scroll position, where the behavior of
maximumScrollPosition() is affected by whether the rubberbanding is active (see ScrollingTreeFrameScrollingNodeMac::maximumScrollPosition()
and its use of totalContentsSizeForRubberBand()).

The result of this was that the active rubberband triggered clamping of the requested scroll position
before we got a chance to stop the rubberband, breaking some programmatic scrolls on netflix.com.

Fix by plumbing through willDoProgrammaticScroll() which stops the rubberband before maximumScrollPosition()
gets a chance to clamp the position.

Will be tested after one additional required patch to come.

  • page/scrolling/ScrollingTreeScrollingNode.cpp:

(WebCore::ScrollingTreeScrollingNode::scrollTo):
(WebCore::ScrollingTreeScrollingNode::isRubberBanding const): Deleted.

  • page/scrolling/ScrollingTreeScrollingNode.h:
  • page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:
  • page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:

(WebCore::ScrollingTreeFrameScrollingNodeMac::willDoProgrammaticScroll):
(WebCore::ScrollingTreeFrameScrollingNodeMac::currentScrollPositionChanged):

  • page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.h:
  • page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.mm:

(WebCore::ScrollingTreeOverflowScrollingNodeMac::willDoProgrammaticScroll):
(WebCore::ScrollingTreeOverflowScrollingNodeMac::currentScrollPositionChanged):

  • page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.h:
  • page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:

(WebCore::ScrollingTreeScrollingNodeDelegateMac::willDoProgrammaticScroll):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::scrollPositionIsNotRubberbandingEdge const):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::currentScrollPositionChanged):

  • platform/RectEdges.h:

(WebCore::operator<<):

  • platform/ScrollAnimator.cpp:

(WebCore::ScrollAnimator::notifyPositionChanged):

  • platform/cocoa/ScrollController.h:

(WebCore::ScrollController::rubberBandingEdges const):

  • platform/cocoa/ScrollController.mm:

(WebCore::ScrollController::scrollPositionChanged):
(WebCore::ScrollController::stopRubberbanding):

Location:
trunk/Source/WebCore
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r269558 r269559  
     12020-11-06  Simon Fraser  <simon.fraser@apple.com>
     2
     3        A programmatic scroll to a new location should stop rubberbanding
     4        https://bugs.webkit.org/show_bug.cgi?id=218672
     5
     6        Reviewed by Tim Horton.
     7
     8        This is a better version of the fix in r269373. That fix attempted to stop a rubberband if there
     9        was a programmatic scroll that moved you away from the edge where any active rubberband was happening.
     10        However, the code ran too late; by the time ScrollController::scrollPositionChanged() is called,
     11        the position has already changed, but more importantly the requested scroll position had
     12        already been clamped between min and max scroll position, where the behavior of
     13        maximumScrollPosition() is affected by whether the rubberbanding is active (see ScrollingTreeFrameScrollingNodeMac::maximumScrollPosition()
     14        and its use of totalContentsSizeForRubberBand()).
     15
     16        The result of this was that the active rubberband triggered clamping of the requested scroll position
     17        before we got a chance to stop the rubberband, breaking some programmatic scrolls on netflix.com.
     18
     19        Fix by plumbing through willDoProgrammaticScroll() which stops the rubberband before maximumScrollPosition()
     20        gets a chance to clamp the position.
     21
     22        Will be tested after one additional required patch to come.
     23
     24        * page/scrolling/ScrollingTreeScrollingNode.cpp:
     25        (WebCore::ScrollingTreeScrollingNode::scrollTo):
     26        (WebCore::ScrollingTreeScrollingNode::isRubberBanding const): Deleted.
     27        * page/scrolling/ScrollingTreeScrollingNode.h:
     28        * page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:
     29        * page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
     30        (WebCore::ScrollingTreeFrameScrollingNodeMac::willDoProgrammaticScroll):
     31        (WebCore::ScrollingTreeFrameScrollingNodeMac::currentScrollPositionChanged):
     32        * page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.h:
     33        * page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.mm:
     34        (WebCore::ScrollingTreeOverflowScrollingNodeMac::willDoProgrammaticScroll):
     35        (WebCore::ScrollingTreeOverflowScrollingNodeMac::currentScrollPositionChanged):
     36        * page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.h:
     37        * page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:
     38        (WebCore::ScrollingTreeScrollingNodeDelegateMac::willDoProgrammaticScroll):
     39        (WebCore::ScrollingTreeScrollingNodeDelegateMac::scrollPositionIsNotRubberbandingEdge const):
     40        (WebCore::ScrollingTreeScrollingNodeDelegateMac::currentScrollPositionChanged):
     41        * platform/RectEdges.h:
     42        (WebCore::operator<<):
     43        * platform/ScrollAnimator.cpp:
     44        (WebCore::ScrollAnimator::notifyPositionChanged):
     45        * platform/cocoa/ScrollController.h:
     46        (WebCore::ScrollController::rubberBandingEdges const):
     47        * platform/cocoa/ScrollController.mm:
     48        (WebCore::ScrollController::scrollPositionChanged):
     49        (WebCore::ScrollController::stopRubberbanding):
     50
    1512020-11-06  Simon Fraser  <simon.fraser@apple.com>
    252

  • trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp

    r269373 r269559  
    208208}
    209209
    210 bool ScrollingTreeScrollingNode::isRubberBanding() const
    211 {
    212     auto scrollPosition = currentScrollPosition();
    213     auto minScrollPosition = minimumScrollPosition();
    214     auto maxScrollPosition = maximumScrollPosition();
    215 
    216     return scrollPosition.x() < minScrollPosition.x()
    217         || scrollPosition.x() > maxScrollPosition.x()
    218         || scrollPosition.y() < minScrollPosition.y()
    219         || scrollPosition.y() > maxScrollPosition.y();
    220 }
    221 
    222210bool ScrollingTreeScrollingNode::isUserScrollProgress() const
    223211{
    … …  
    259247
    260248    scrollingTree().setIsHandlingProgrammaticScroll(scrollType == ScrollType::Programmatic);
     249
     250    if (scrollType == ScrollType::Programmatic)
     251        willDoProgrammaticScroll(position);
    261252
    262253    m_currentScrollPosition = adjustedScrollPosition(position, clamp);

  • trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h

    r269373 r269559  
    6767
    6868    RectEdges<bool> edgePinnedState() const;
    69     bool isRubberBanding() const;
    7069
    7170    bool isUserScrollProgress() const;
    … …  
    125124
    126125    FloatPoint clampScrollPosition(const FloatPoint&) const;
     126   
     127    virtual void willDoProgrammaticScroll(const FloatPoint&) { }
    127128   
    128129    virtual FloatPoint adjustedScrollPosition(const FloatPoint&, ScrollClamping = ScrollClamping::Clamped) const;

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h

    r269373 r269559  
    6565private:
    6666    void willBeDestroyed() final;
     67    void willDoProgrammaticScroll(const FloatPoint&) final;
    6768
    68     FloatPoint adjustedScrollPosition(const FloatPoint&, ScrollClamping) const override;
     69    FloatPoint adjustedScrollPosition(const FloatPoint&, ScrollClamping) const final;
    6970
    7071    void currentScrollPositionChanged(ScrollType, ScrollingLayerPositionAction) final;

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm

    r269373 r269559  
    134134}
    135135
     136void ScrollingTreeFrameScrollingNodeMac::willDoProgrammaticScroll(const FloatPoint& targetScrollPosition)
     137{
     138    m_delegate.willDoProgrammaticScroll(targetScrollPosition);
     139}
     140
    136141FloatPoint ScrollingTreeFrameScrollingNodeMac::adjustedScrollPosition(const FloatPoint& position, ScrollClamping clamp) const
    137142{
    … …  
    144149    LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreeFrameScrollingNodeMac " << scrollingNodeID() << " currentScrollPositionChanged to " << currentScrollPosition() << " min: " << minimumScrollPosition() << " max: " << maximumScrollPosition() << " sync: " << hasSynchronousScrollingReasons());
    145150
    146     m_delegate.currentScrollPositionChanged(scrollType);
     151    m_delegate.currentScrollPositionChanged();
    147152
    148153    if (isRootNode())

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.h

    r269373 r269559  
    4949
    5050    void currentScrollPositionChanged(ScrollType, ScrollingLayerPositionAction) final;
     51    void willDoProgrammaticScroll(const FloatPoint&) final;
    5152
    5253    void repositionScrollingLayers() override;

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.mm

    r269373 r269559  
    8787}
    8888
     89void ScrollingTreeOverflowScrollingNodeMac::willDoProgrammaticScroll(const FloatPoint& targetScrollPosition)
     90{
     91    m_delegate.willDoProgrammaticScroll(targetScrollPosition);
     92}
     93
    8994void ScrollingTreeOverflowScrollingNodeMac::currentScrollPositionChanged(ScrollType scrollType, ScrollingLayerPositionAction action)
    9095{
    9196    ScrollingTreeOverflowScrollingNode::currentScrollPositionChanged(scrollType, action);
    92     m_delegate.currentScrollPositionChanged(scrollType);
     97    m_delegate.currentScrollPositionChanged();
    9398}
    9499

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.h

    r269373 r269559  
    5252    bool handleWheelEvent(const PlatformWheelEvent&);
    5353   
    54     void currentScrollPositionChanged(ScrollType);
     54    void willDoProgrammaticScroll(const FloatPoint&);
     55    void currentScrollPositionChanged();
    5556
    5657#if ENABLE(CSS_SCROLL_SNAP)
    … …  
    8990    void adjustScrollPositionToBoundsIfNecessary() final;
    9091
     92    bool scrollPositionIsNotRubberbandingEdge(const FloatPoint&) const;
     93
    9194#if ENABLE(CSS_SCROLL_SNAP)
    9295    FloatPoint scrollOffset() const override;

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm

    r269373 r269559  
    161161}
    162162
    163 void ScrollingTreeScrollingNodeDelegateMac::currentScrollPositionChanged(ScrollType scrollType)
    164 {
    165     m_scrollController.scrollPositionChanged(scrollType);
     163void ScrollingTreeScrollingNodeDelegateMac::willDoProgrammaticScroll(const FloatPoint& targetPosition)
     164{
     165    if (scrollPositionIsNotRubberbandingEdge(targetPosition)) {
     166        LOG(Scrolling, "ScrollingTreeScrollingNodeDelegateMac::willDoProgrammaticScroll() - scrolling away from rubberbanding edge so stopping rubberbanding");
     167        m_scrollController.stopRubberbanding();
     168    }
     169}
     170
     171bool ScrollingTreeScrollingNodeDelegateMac::scrollPositionIsNotRubberbandingEdge(const FloatPoint& targetPosition) const
     172{
     173#if ENABLE(RUBBER_BANDING)
     174    if (!m_scrollController.isRubberBandInProgress())
     175        return false;
     176
     177    auto rubberbandingEdges = m_scrollController.rubberBandingEdges();
     178
     179    auto minimumScrollPosition = this->minimumScrollPosition();
     180    auto maximumScrollPosition = this->maximumScrollPosition();
     181
     182    for (auto side : allBoxSides) {
     183        if (!rubberbandingEdges[side])
     184            continue;
     185
     186        switch (side) {
     187        case BoxSide::Top:
     188            if (targetPosition.y() != minimumScrollPosition.y())
     189                return true;
     190            break;
     191        case BoxSide::Right:
     192            if (targetPosition.x() != maximumScrollPosition.x())
     193                return true;
     194            break;
     195        case BoxSide::Bottom:
     196            if (targetPosition.y() != maximumScrollPosition.y())
     197                return true;
     198            break;
     199        case BoxSide::Left:
     200            if (targetPosition.x() != minimumScrollPosition.x())
     201                return true;
     202            break;
     203        }
     204    }
     205#else
     206    UNUSED_PARAM(targetPosition);
     207#endif
     208    return false;
     209}
     210
     211void ScrollingTreeScrollingNodeDelegateMac::currentScrollPositionChanged()
     212{
     213    m_scrollController.scrollPositionChanged();
    166214}
    167215

  • trunk/Source/WebCore/platform/RectEdges.h

    r269228 r269559  
    2929#include <array>
    3030#include <wtf/OptionSet.h>
     31#include <wtf/text/TextStream.h>
    3132
    3233namespace WebCore {
    … …  
    9293};
    9394
     95
     96template<typename T>
     97TextStream& operator<<(TextStream& ts, const RectEdges<T>& edges)
     98{
     99    ts << "[top " << edges.top() << " right " << edges.right() << " bottom " << edges.bottom() << " left " << edges.left() << "]";
     100    return ts;
    94101}
     102
     103}

  • trunk/Source/WebCore/platform/ScrollAnimator.cpp

    r269373 r269559  
    236236
    237237#if ENABLE(CSS_SCROLL_SNAP) || ENABLE(RUBBER_BANDING)
    238     m_scrollController.scrollPositionChanged(ScrollType::Programmatic);
     238    m_scrollController.scrollPositionChanged();
    239239#endif
    240240}

  • trunk/Source/WebCore/platform/cocoa/ScrollController.h

    r269373 r269559  
    146146#endif
    147147
     148    void stopRubberbanding();
     149
    148150    bool usesScrollSnap() const;
    149151
    … …  
    152154    bool isScrollSnapInProgress() const;
    153155   
    154     void scrollPositionChanged(ScrollType);
     156#if ENABLE(RUBBER_BANDING)
     157    RectEdges<bool> rubberBandingEdges() const { return m_rubberBandingEdges; }
     158#endif
     159
     160    void scrollPositionChanged();
    155161
    156162#if ENABLE(CSS_SCROLL_SNAP)
    … …  
    179185private:
    180186#if ENABLE(RUBBER_BANDING)
    181     void stopRubberbanding();
    182 
    183187    void startSnapRubberbandTimer();
    184188    void stopSnapRubberbandTimer();
    … …  
    192196    void updateRubberBandingState();
    193197    void updateRubberBandingEdges(IntSize clientStretch);
    194    
    195     bool scrolledToRubberbandingEdge() const;
    196198#endif
    197199

  • trunk/Source/WebCore/platform/cocoa/ScrollController.mm

    r269373 r269559  
    460460#endif
    461461
    462 void ScrollController::scrollPositionChanged(ScrollType scrollType)
     462void ScrollController::scrollPositionChanged()
    463463{
    464464#if ENABLE(RUBBER_BANDING)
    465     if (scrollType == ScrollType::Programmatic && !scrolledToRubberbandingEdge())
    466         stopRubberbanding();
    467 
    468465    updateRubberBandingState();
    469 #else
    470     UNUSED_PARAM(scrollType);
    471466#endif
    472467}
    … …  
    511506}
    512507
     508void ScrollController::stopRubberbanding()
     509{
    513510#if ENABLE(RUBBER_BANDING)
    514 void ScrollController::stopRubberbanding()
    515 {
    516511    stopSnapRubberbandTimer();
    517512    m_stretchScrollForce = { };
    … …  
    520515    m_origVelocity = { };
    521516    updateRubberBandingState();
    522 }
    523 
     517#endif
     518}
     519
     520#if ENABLE(RUBBER_BANDING)
    524521void ScrollController::startSnapRubberbandTimer()
    525522{
    … …  
    607604    m_rubberBandingEdges.setTop(clientStretch.height() < 0);
    608605    m_rubberBandingEdges.setBottom(clientStretch.height() > 0);
    609 }
    610 
    611 bool ScrollController::scrolledToRubberbandingEdge() const
    612 {
    613     auto pinnedEdges = m_client.edgePinnedState();
    614    
    615     for (auto side : allBoxSides) {
    616         if (m_rubberBandingEdges[side] && !pinnedEdges[side])
    617             return false;
    618     }
    619    
    620     return true;
    621606}
    622607
Note: See TracChangeset for help on using the changeset viewer.