Clean up state maintenance around animated scrolls

https://bugs.webkit.org/show_bug.cgi?id=231347

Reviewed by Martin Robinson.

ScrollBehaviorStatus tracked whether an animated scroll is in progress. Rename it
for clarity, and remove the "non-native" term since this will never track native
vs. non-native animations.

Since ScrollBehaviorStatus is specifically about programmatic smooth scroll animations
triggered from script, it should be stored on ScrollableArea and is not relevant to the
scrolling tree. Remove it from the ScrollingTreeScrollingNodeDelegates.

The state becomes ScrollAnimationStatus::Animating immediately when JS triggers animations,
but the animation completion signal may come back in future from the scrolling thread.
Currently it comes via ScrollingEffectsController::scrollAnimationDidEnd().

We never need to call setScrollAnimationStatus(ScrollAnimationStatus::NotAnimating) other
than from scrollAnimationDidEnd(), because canceling an ongoing animation will always
call that.

* page/DOMWindow.cpp:
(WebCore::DOMWindow::scrollTo const):
* page/FrameView.cpp:
(WebCore::FrameView::scrollToPositionWithAnimation):
* page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.h:
* page/scrolling/nicosia/ScrollingTreeScrollingNodeDelegateNicosia.h:
* platform/ScrollAnimator.cpp:
(WebCore::ScrollAnimator::scrollToPositionWithAnimation):
(WebCore::ScrollAnimator::retargetRunningAnimation):
(WebCore::ScrollAnimator::willStartAnimatedScroll):
(WebCore::ScrollAnimator::didStopAnimatedScroll):
(WebCore::ScrollAnimator::setScrollBehaviorStatus): Deleted.
(WebCore::ScrollAnimator::scrollBehaviorStatus const): Deleted.
* platform/ScrollAnimator.h:
* platform/ScrollTypes.h:
* platform/ScrollView.cpp:
(WebCore::ScrollView::setScrollPosition):
* platform/ScrollableArea.cpp:
(WebCore::ScrollableArea::scrollToPositionWithAnimation):
(WebCore::ScrollableArea::resnapAfterLayout):
* platform/ScrollableArea.h:
(WebCore::ScrollableArea::scrollAnimationStatus):
(WebCore::ScrollableArea::setScrollAnimationStatus):
(WebCore::ScrollableArea::currentScrollBehaviorStatus): Deleted.
(WebCore::ScrollableArea::setScrollBehaviorStatus): Deleted.
* platform/ScrollingEffectsController.cpp:
(WebCore::ScrollingEffectsController::scrollAnimationDidEnd):
* platform/ScrollingEffectsController.h:
(WebCore::ScrollingEffectsControllerClient::willStartAnimatedScroll):
(WebCore::ScrollingEffectsControllerClient::didStopAnimatedScroll):
* rendering/RenderLayerScrollableArea.cpp:
(WebCore::RenderLayerScrollableArea::scrollToOffset):
(WebCore::RenderLayerScrollableArea::scrollTo):
(WebCore::RenderLayerScrollableArea::updateScrollPosition):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@283716 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,61 @@
+2021-10-06  Simon Fraser  <simon.fraser@apple.com>
+
+        Clean up state maintenance around animated scrolls
+        https://bugs.webkit.org/show_bug.cgi?id=231347
+
+        Reviewed by Martin Robinson.
+
+        ScrollBehaviorStatus tracked whether an animated scroll is in progress. Rename it
+        for clarity, and remove the "non-native" term since this will never track native
+        vs. non-native animations.
+
+        Since ScrollBehaviorStatus is specifically about programmatic smooth scroll animations
+        triggered from script, it should be stored on ScrollableArea and is not relevant to the
+        scrolling tree. Remove it from the ScrollingTreeScrollingNodeDelegates.
+
+        The state becomes ScrollAnimationStatus::Animating immediately when JS triggers animations,
+        but the animation completion signal may come back in future from the scrolling thread.
+        Currently it comes via ScrollingEffectsController::scrollAnimationDidEnd().
+
+        We never need to call setScrollAnimationStatus(ScrollAnimationStatus::NotAnimating) other
+        than from scrollAnimationDidEnd(), because canceling an ongoing animation will always
+        call that.
+
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::scrollTo const):
+        * page/FrameView.cpp:
+        (WebCore::FrameView::scrollToPositionWithAnimation):
+        * page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.h:
+        * page/scrolling/nicosia/ScrollingTreeScrollingNodeDelegateNicosia.h:
+        * platform/ScrollAnimator.cpp:
+        (WebCore::ScrollAnimator::scrollToPositionWithAnimation):
+        (WebCore::ScrollAnimator::retargetRunningAnimation):
+        (WebCore::ScrollAnimator::willStartAnimatedScroll):
+        (WebCore::ScrollAnimator::didStopAnimatedScroll):
+        (WebCore::ScrollAnimator::setScrollBehaviorStatus): Deleted.
+        (WebCore::ScrollAnimator::scrollBehaviorStatus const): Deleted.
+        * platform/ScrollAnimator.h:
+        * platform/ScrollTypes.h:
+        * platform/ScrollView.cpp:
+        (WebCore::ScrollView::setScrollPosition):
+        * platform/ScrollableArea.cpp:
+        (WebCore::ScrollableArea::scrollToPositionWithAnimation):
+        (WebCore::ScrollableArea::resnapAfterLayout):
+        * platform/ScrollableArea.h:
+        (WebCore::ScrollableArea::scrollAnimationStatus):
+        (WebCore::ScrollableArea::setScrollAnimationStatus):
+        (WebCore::ScrollableArea::currentScrollBehaviorStatus): Deleted.
+        (WebCore::ScrollableArea::setScrollBehaviorStatus): Deleted.
+        * platform/ScrollingEffectsController.cpp:
+        (WebCore::ScrollingEffectsController::scrollAnimationDidEnd):
+        * platform/ScrollingEffectsController.h:
+        (WebCore::ScrollingEffectsControllerClient::willStartAnimatedScroll):
+        (WebCore::ScrollingEffectsControllerClient::didStopAnimatedScroll):
+        * rendering/RenderLayerScrollableArea.cpp:
+        (WebCore::RenderLayerScrollableArea::scrollToOffset):
+        (WebCore::RenderLayerScrollableArea::scrollTo):
+        (WebCore::RenderLayerScrollableArea::updateScrollPosition):
+
 2021-10-07  Alan Bujtas  <zalan@apple.com>
 
         [LFC][IFC] InlineDisplay::Box should be able to tell if it's the first or the last box within the associated Layout::Box

Source/WebCore/page/DOMWindow.cpp

@@ -1712,7 +1712,7 @@ void DOMWindow::scrollTo(const ScrollToOptions& options, ScrollClamping clamping
 
     // This is an optimization for the common case of scrolling to (0, 0) when the scroller is already at the origin.
     // If an animated scroll is in progress, this optimization is skipped to ensure that the animated scroll is really stopped.
-    if (view->currentScrollBehaviorStatus() == ScrollBehaviorStatus::NotInAnimation && !scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition() == IntPoint(0, 0))
+    if (view->scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating && !scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition().isZero())
         return;
 
     document()->updateLayoutIgnorePendingStylesheets();

Source/WebCore/page/FrameView.cpp

@@ -3768,9 +3768,10 @@ void FrameView::scrollToPositionWithAnimation(const ScrollPosition& position, Sc
     auto previousScrollType = currentScrollType();
     setCurrentScrollType(scrollType);
 
-    if (currentScrollBehaviorStatus() == ScrollBehaviorStatus::InNonNativeAnimation)
+    if (scrollAnimationStatus() == ScrollAnimationStatus::Animating)
         scrollAnimator().cancelAnimations();
-    if (position != this->scrollPosition())
+
+    if (position != scrollPosition())
         ScrollableArea::scrollToPositionWithAnimation(position);
 
     setCurrentScrollType(previousScrollType);

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

@@ -81,8 +81,6 @@ class ScrollingTreeScrollingNodeDelegateMac : public ScrollingTreeScrollingNodeD
     RectEdges<bool> edgePinnedState() const final;
     bool allowsHorizontalScrolling() const final;
     bool allowsVerticalScrolling() const final;
-    void setScrollBehaviorStatus(ScrollBehaviorStatus status) final { m_scrollBehaviorStatus = status; }
-    ScrollBehaviorStatus scrollBehaviorStatus() const final { return m_scrollBehaviorStatus; }
 
     bool shouldRubberBandOnSide(BoxSide) const final;
     void immediateScrollBy(const FloatSize&, ScrollClamping = ScrollClamping::Clamped) final;
@@ -108,7 +106,6 @@ class ScrollingTreeScrollingNodeDelegateMac : public ScrollingTreeScrollingNodeD
 
     std::unique_ptr<RunLoop::Timer<ScrollingTreeScrollingNodeDelegateMac>> m_scrollControllerAnimationTimer;
 
-    ScrollBehaviorStatus m_scrollBehaviorStatus { ScrollBehaviorStatus::NotInAnimation };
     bool m_inMomentumPhase { false };
 };
 

Source/WebCore/page/scrolling/nicosia/ScrollingTreeScrollingNodeDelegateNicosia.h

@@ -63,9 +63,6 @@ class ScrollingTreeScrollingNodeDelegateNicosia : public ScrollingTreeScrollingN
     bool allowsHorizontalScrolling() const final;
     bool allowsVerticalScrolling() const final;
 
-    void setScrollBehaviorStatus(ScrollBehaviorStatus status) final { m_scrollBehaviorStatus = status; }
-    ScrollBehaviorStatus scrollBehaviorStatus() const final { return m_scrollBehaviorStatus; }
-
     void immediateScrollBy(const FloatSize&, ScrollClamping = ScrollClamping::Clamped) final;
 
     void adjustScrollPositionToBoundsIfNecessary() final;
@@ -80,7 +77,6 @@ class ScrollingTreeScrollingNodeDelegateNicosia : public ScrollingTreeScrollingN
 
     ScrollingEffectsController m_scrollController;
     std::unique_ptr<RunLoop::Timer<ScrollingTreeScrollingNodeDelegateNicosia>> m_animationTimer;
-    ScrollBehaviorStatus m_scrollBehaviorStatus { ScrollBehaviorStatus::NotInAnimation };
 
     bool m_scrollAnimatorEnabled { false };
 };

Source/WebCore/platform/ScrollAnimator.cpp

@@ -119,14 +119,12 @@ bool ScrollAnimator::scrollToPositionWithAnimation(const FloatPoint& newPosition
     if (!positionChanged && !scrollableArea().scrollOriginChanged())
         return false;
 
-    m_scrollController.startAnimatedScrollToDestination(offsetFromPosition(m_currentPosition), offsetFromPosition(newPosition));
-    setScrollBehaviorStatus(ScrollBehaviorStatus::InNonNativeAnimation);
-    return true;
+    return m_scrollController.startAnimatedScrollToDestination(offsetFromPosition(m_currentPosition), offsetFromPosition(newPosition));
 }
 
 void ScrollAnimator::retargetRunningAnimation(const FloatPoint& newPosition)
 {
-    ASSERT(scrollableArea().currentScrollBehaviorStatus() == ScrollBehaviorStatus::InNonNativeAnimation);
+    ASSERT(scrollableArea().scrollAnimationStatus() == ScrollAnimationStatus::Animating);
     m_scrollController.retargetAnimatedScroll(offsetFromPosition(newPosition));
 }
 
@@ -292,14 +290,14 @@ bool ScrollAnimator::allowsVerticalScrolling() const
     return m_scrollableArea.allowsVerticalScrolling();
 }
 
-void ScrollAnimator::setScrollBehaviorStatus(ScrollBehaviorStatus status)
+void ScrollAnimator::willStartAnimatedScroll()
 {
-    m_scrollableArea.setScrollBehaviorStatus(status);
+    m_scrollableArea.setScrollAnimationStatus(ScrollAnimationStatus::Animating);
 }
 
-ScrollBehaviorStatus ScrollAnimator::scrollBehaviorStatus() const
+void ScrollAnimator::didStopAnimatedScroll()
 {
-    return m_scrollableArea.currentScrollBehaviorStatus();
+    m_scrollableArea.setScrollAnimationStatus(ScrollAnimationStatus::NotAnimating);
 }
 
 #if HAVE(RUBBER_BANDING)

Source/WebCore/platform/ScrollAnimator.h

@@ -145,8 +145,8 @@ class ScrollAnimator : private ScrollingEffectsControllerClient {
     bool allowsHorizontalScrolling() const final;
     bool allowsVerticalScrolling() const final;
 
-    void setScrollBehaviorStatus(ScrollBehaviorStatus) final;
-    ScrollBehaviorStatus scrollBehaviorStatus() const final;
+    void willStartAnimatedScroll() final;
+    void didStopAnimatedScroll() final;
 
     void immediateScrollBy(const FloatSize&, ScrollClamping = ScrollClamping::Clamped) final;
     void adjustScrollPositionToBoundsIfNecessary() final;

Source/WebCore/platform/ScrollTypes.h

@@ -67,11 +67,9 @@ enum ScrollLogicalDirection : uint8_t {
     ScrollInlineDirectionForward
 };
 
-// FIXME: Add another status InNativeAnimation to indicate native scrolling is in progress.
-// See: https://bugs.webkit.org/show_bug.cgi?id=204936
-enum class ScrollBehaviorStatus : uint8_t {
-    NotInAnimation,
-    InNonNativeAnimation,
+enum class ScrollAnimationStatus : uint8_t {
+    NotAnimating,
+    Animating,
 };
 
 enum class ScrollIsAnimated : uint8_t {

Source/WebCore/platform/ScrollView.cpp

@@ -523,17 +523,15 @@ void ScrollView::setScrollPosition(const ScrollPosition& scrollPosition, const S
         return;
     }
 
-    if (currentScrollBehaviorStatus() == ScrollBehaviorStatus::InNonNativeAnimation)
+    if (scrollAnimationStatus() == ScrollAnimationStatus::Animating)
         scrollAnimator().cancelAnimations();
 
     ScrollPosition newScrollPosition = (!delegatesScrolling() && options.clamping == ScrollClamping::Clamped) ? adjustScrollPositionWithinRange(scrollPosition) : scrollPosition;
-    if ((!delegatesScrolling() || currentScrollType() == ScrollType::User) && currentScrollBehaviorStatus() == ScrollBehaviorStatus::NotInAnimation && newScrollPosition == this->scrollPosition())
+    if ((!delegatesScrolling() || currentScrollType() == ScrollType::User) && newScrollPosition == this->scrollPosition())
         return;
 
     if (!requestScrollPositionUpdate(newScrollPosition, currentScrollType(), options.clamping))
         updateScrollbars(newScrollPosition);
-
-    setScrollBehaviorStatus(ScrollBehaviorStatus::NotInAnimation);
 }
 
 bool ScrollView::scroll(ScrollDirection direction, ScrollGranularity granularity)

Source/WebCore/platform/ScrollableArea.cpp

@@ -143,7 +143,8 @@ void ScrollableArea::scrollToPositionWithoutAnimation(const FloatPoint& position
 void ScrollableArea::scrollToPositionWithAnimation(const FloatPoint& position, ScrollClamping)
 {
     LOG_WITH_STREAM(Scrolling, stream << "ScrollableArea " << this << " scrollToPositionWithAnimation " << position);
-    scrollAnimator().scrollToPositionWithAnimation(position);
+    if (scrollAnimator().scrollToPositionWithAnimation(position))
+        setScrollAnimationStatus(ScrollAnimationStatus::Animating);
 }
 
 void ScrollableArea::scrollToOffsetWithoutAnimation(const FloatPoint& offset, ScrollClamping clamping)
@@ -539,7 +540,7 @@ void ScrollableArea::resnapAfterLayout()
     if (correctedOffset != currentOffset) {
         LOG_WITH_STREAM(ScrollSnap, stream << " adjusting offset from " << currentOffset << " to " << correctedOffset);
         auto position = scrollPositionFromOffset(correctedOffset);
-        if (currentScrollBehaviorStatus() == ScrollBehaviorStatus::NotInAnimation)
+        if (scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating)
             scrollToOffsetWithoutAnimation(correctedOffset);
         else
             scrollAnimator->retargetRunningAnimation(position);

Source/WebCore/platform/ScrollableArea.h

@@ -68,9 +68,6 @@ class ScrollableArea : public CanMakeWeakPtr<ScrollableArea> {
     virtual bool isListBox() const { return false; }
     virtual bool isPDFPlugin() const { return false; }
 
-    ScrollBehaviorStatus currentScrollBehaviorStatus() { return m_currentScrollBehaviorStatus; }
-    void setScrollBehaviorStatus(ScrollBehaviorStatus status) { m_currentScrollBehaviorStatus = status; }
-
     WEBCORE_EXPORT bool scroll(ScrollDirection, ScrollGranularity, float multiplier = 1);
     WEBCORE_EXPORT void scrollToPositionWithAnimation(const FloatPoint&, ScrollClamping = ScrollClamping::Clamped);
     WEBCORE_EXPORT void scrollToPositionWithoutAnimation(const FloatPoint&, ScrollClamping = ScrollClamping::Clamped);
@@ -252,6 +249,10 @@ class ScrollableArea : public CanMakeWeakPtr<ScrollableArea> {
     ScrollType currentScrollType() const { return m_currentScrollType; }
     void setCurrentScrollType(ScrollType scrollType) { m_currentScrollType = scrollType; }
 
+    // This reflects animated scrolls triggered by CSS OM View "smooth" scrolls.
+    ScrollAnimationStatus scrollAnimationStatus() { return m_scrollAnimationStatus; }
+    void setScrollAnimationStatus(ScrollAnimationStatus status) { m_scrollAnimationStatus = status; }
+
     bool scrollShouldClearLatchedState() const { return m_scrollShouldClearLatchedState; }
     void setScrollShouldClearLatchedState(bool shouldClear) { m_scrollShouldClearLatchedState = shouldClear; }
 
@@ -403,7 +404,7 @@ class ScrollableArea : public CanMakeWeakPtr<ScrollableArea> {
     ScrollbarOverlayStyle m_scrollbarOverlayStyle { ScrollbarOverlayStyle::ScrollbarOverlayStyleDefault };
 
     ScrollType m_currentScrollType { ScrollType::User };
-    ScrollBehaviorStatus m_currentScrollBehaviorStatus { ScrollBehaviorStatus::NotInAnimation };
+    ScrollAnimationStatus m_scrollAnimationStatus { ScrollAnimationStatus::NotAnimating };
 
     bool m_inLiveResize { false };
     bool m_scrollOriginChanged { false };

Source/WebCore/platform/ScrollingEffectsController.cpp

@@ -456,7 +456,8 @@ void ScrollingEffectsController::scrollAnimationDidEnd(ScrollAnimation& animatio
         stopScrollSnapAnimation();
     }
 
-    m_client.setScrollBehaviorStatus(ScrollBehaviorStatus::NotInAnimation);
+    // FIXME: Need to track state better and only call this when the running animation is for CSS smooth scrolling. Calling should be harmless, though.
+    m_client.didStopAnimatedScroll();
     startOrStopAnimationCallbacks();
 }
 

Source/WebCore/platform/ScrollingEffectsController.h

@@ -82,10 +82,6 @@ class ScrollingEffectsControllerClient {
     virtual bool allowsHorizontalScrolling() const = 0;
     virtual bool allowsVerticalScrolling() const = 0;
 
-    // FIXME: Maybe ScrollBehaviorStatus should be stored on ScrollingEffectsController.
-    virtual void setScrollBehaviorStatus(ScrollBehaviorStatus) = 0;
-    virtual ScrollBehaviorStatus scrollBehaviorStatus() const = 0;
-
     virtual void immediateScrollBy(const FloatSize&, ScrollClamping = ScrollClamping::Clamped) = 0;
 
     // If the current scroll position is within the overhang area, this function will cause
@@ -113,8 +109,13 @@ class ScrollingEffectsControllerClient {
     virtual void removeWheelEventTestCompletionDeferralForReason(WheelEventTestMonitor::ScrollableAreaIdentifier, WheelEventTestMonitor::DeferReason) const { /* Do nothing */ }
 
     virtual FloatPoint scrollOffset() const = 0;
+
+    virtual void willStartAnimatedScroll() { }
+    virtual void didStopAnimatedScroll() { }
+
     virtual void willStartScrollSnapAnimation() { }
     virtual void didStopScrollSnapAnimation() { }
+
     virtual float pageScaleFactor() const = 0;
     virtual ScrollExtents scrollExtents() const = 0;
     virtual bool scrollAnimationEnabled() const { return true; }

Source/WebCore/rendering/RenderLayerScrollableArea.cpp

@@ -256,7 +256,7 @@ bool RenderLayerScrollableArea::requestScrollPositionUpdate(const ScrollPosition
 
 ScrollOffset RenderLayerScrollableArea::scrollToOffset(const ScrollOffset& scrollOffset, const ScrollPositionChangeOptions& options)
 {
-    if (currentScrollBehaviorStatus() == ScrollBehaviorStatus::InNonNativeAnimation)
+    if (scrollAnimationStatus() == ScrollAnimationStatus::Animating)
         scrollAnimator().cancelAnimations();
 
     ScrollOffset clampedScrollOffset = options.clamping == ScrollClamping::Clamped ? clampScrollOffset(scrollOffset) : scrollOffset;
@@ -270,11 +270,8 @@ ScrollOffset RenderLayerScrollableArea::scrollToOffset(const ScrollOffset& scrol
     auto snappedPosition = scrollPositionFromOffset(snappedOffset);
     if (options.animated == ScrollIsAnimated::Yes)
         ScrollableArea::scrollToPositionWithAnimation(snappedPosition);
-    else {
-        if (!requestScrollPositionUpdate(snappedPosition, options.type, options.clamping))
-            scrollToPositionWithoutAnimation(snappedPosition, options.clamping);
-        setScrollBehaviorStatus(ScrollBehaviorStatus::NotInAnimation);
-    }
+    else if (!requestScrollPositionUpdate(snappedPosition, options.type, options.clamping))
+        scrollToPositionWithoutAnimation(snappedPosition, options.clamping);
 
     setCurrentScrollType(previousScrollType);
     return snappedOffset;
@@ -312,7 +309,7 @@ void RenderLayerScrollableArea::scrollTo(const ScrollPosition& position)
 #endif
     }
 
-    if (m_scrollPosition == newPosition && currentScrollBehaviorStatus() == ScrollBehaviorStatus::NotInAnimation) {
+    if (m_scrollPosition == newPosition && scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating) {
         // FIXME: Nothing guarantees we get a scrollTo() with an unchanged position at the end of a user gesture.
         // The ScrollingCoordinator probably needs to message the main thread when a gesture ends.
         if (requiresScrollPositionReconciliation()) {
@@ -1759,7 +1756,7 @@ std::optional<LayoutRect> RenderLayerScrollableArea::updateScrollPosition(const
     ASSERT(box);
 
     ScrollOffset clampedScrollOffset = clampScrollOffset(scrollOffset() + toIntSize(roundedIntRect(revealRect).location()));
-    if (clampedScrollOffset != scrollOffset() || currentScrollBehaviorStatus() != ScrollBehaviorStatus::NotInAnimation) {
+    if (clampedScrollOffset != scrollOffset() || scrollAnimationStatus() != ScrollAnimationStatus::NotAnimating) {
         ScrollOffset oldScrollOffset = scrollOffset();
         ScrollOffset realScrollOffset = scrollToOffset(clampedScrollOffset, options);