REGRESSION (r260276): Scrolling through shelves on music.apple.com is not smooth
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Jun 2020 23:58:30 +0000 (23:58 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Jun 2020 23:58:30 +0000 (23:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=213572
Source/WebCore:

Reviewed by Wenson Hsieh.

The scroll position for an overflow:scroll element with scroll-snap could jump around
during scrolling, if layout triggered a call to ScrollableArea::updateScrollSnapState().
The crux of the issue is that isScrollSnapInProgress() returned false for overflow:scrollers
which are scrolling asynchronously.

Fix by extending the existing ScrollingTree::isScrollSnapInProgress() to track all scrolling
nodes by storing a HashSet<ScrollingNodeID> rather than just a flag for the main frame.

RenderLayer::isScrollSnapInProgress() then consults the ScrollingCoordinator, which consults
the scrolling tree for this state.

Add the ability to test via internals.isScrollSnapInProgress().

Test: fast/scrolling/mac/scroll-snapping-in-progress.html

* dom/Document.h:
* page/FrameView.cpp:
(WebCore::FrameView::isScrollSnapInProgress const): The main thread can run snapping logic
even for async-scrollable frames, so consult the ScrollingCoordinator and FrameView's ScrollAnimator.
* page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::isScrollSnapInProgress const):
* page/scrolling/AsyncScrollingCoordinator.h:
* page/scrolling/ScrollingCoordinator.h:
(WebCore::ScrollingCoordinator::isScrollSnapInProgress const):
* page/scrolling/ScrollingTree.cpp:
(WebCore::ScrollingTree::isScrollSnapInProgressForNode):
(WebCore::ScrollingTree::setNodeScrollSnapInProgress):
(WebCore::ScrollingTree::isScrollSnapInProgress): Deleted.
(WebCore::ScrollingTree::setMainFrameIsScrollSnapping): Deleted.
* page/scrolling/ScrollingTree.h:
* page/scrolling/ScrollingTreeScrollingNode.cpp:
(WebCore::ScrollingTreeScrollingNode::setScrollSnapInProgress):
(WebCore::ScrollingTreeScrollingNode::isScrollSnapInProgress const):
* page/scrolling/ScrollingTreeScrollingNode.h:
* page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
(WebCore::ScrollingTreeFrameScrollingNodeMac::handleWheelEvent):
* page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:
(WebCore::ScrollingTreeScrollingNodeDelegateMac::willStartScrollSnapAnimation):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::didStopScrollSnapAnimation):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::requestScrollPositionUpdate):
(WebCore::RenderLayer::isScrollSnapInProgress const):
(WebCore::RenderLayer::setupFontSubpixelQuantization):
* testing/Internals.cpp:
(WebCore:: const):
(WebCore::Internals::scrollSnapOffsets):
(WebCore::Internals::isScrollSnapInProgress):
* testing/Internals.h:
* testing/Internals.idl:

Source/WebKit:

<rdar://problem/64306590>

Reviewed by Wenson Hsieh.

The scroll position for an overflow:scroll element with scroll-snap could jump around
during scrolling, if layout triggered a call to ScrollableArea::updateScrollSnapState().
The crux of the issue is that isScrollSnapInProgress() returned false for overflow:scrollers
which are scrolling asynchronously.

Fix by extending the existing ScrollingTree::isScrollSnapInProgress() to track all scrolling
nodes by storing a HashSet<ScrollingNodeID> rather than just a flag for the main frame.

RenderLayer::isScrollSnapInProgress() then consults the ScrollingCoordinator, which consults
the scrolling tree for this state.

Add the ability to test via internals.isScrollSnapInProgress().

* WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.h:
* WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:
(WebKit::RemoteScrollingCoordinator::isScrollSnapInProgress const):

LayoutTests:

Reviewed by Wenson Hsieh.

* fast/scrolling/mac/scroll-snapping-in-progress-expected.txt: Added.
* fast/scrolling/mac/scroll-snapping-in-progress.html: Added.

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

22 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/scrolling/mac/scroll-snapping-in-progress-expected.txt [new file with mode: 0644]
LayoutTests/fast/scrolling/mac/scroll-snapping-in-progress.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.h
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
Source/WebCore/page/scrolling/ScrollingCoordinator.h
Source/WebCore/page/scrolling/ScrollingTree.cpp
Source/WebCore/page/scrolling/ScrollingTree.h
Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm
Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.h
Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm

index d2898fa..3fc645d 100644 (file)
@@ -1,3 +1,13 @@
+2020-06-24  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r260276): Scrolling through shelves on music.apple.com is not smooth
+        https://bugs.webkit.org/show_bug.cgi?id=213572
+
+        Reviewed by Wenson Hsieh.
+
+        * fast/scrolling/mac/scroll-snapping-in-progress-expected.txt: Added.
+        * fast/scrolling/mac/scroll-snapping-in-progress.html: Added.
+
 2020-06-24  Jason Lawrence  <lawrence.j@apple.com>
 
         Unreviewed, reverting r263466.
diff --git a/LayoutTests/fast/scrolling/mac/scroll-snapping-in-progress-expected.txt b/LayoutTests/fast/scrolling/mac/scroll-snapping-in-progress-expected.txt
new file mode 100644 (file)
index 0000000..40c4dff
--- /dev/null
@@ -0,0 +1,11 @@
+Scroll-snap offsets for 'container': vertical = { 0, 30, 150, 270, 390, 510, 615 }
+Initial state:
+PASS window.internals.isScrollSnapInProgress(container) is false
+
+Sending wheel events
+PASS container.scrollTop > 0 is true
+PASS window.internals.isScrollSnapInProgress(container) is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/scrolling/mac/scroll-snapping-in-progress.html b/LayoutTests/fast/scrolling/mac/scroll-snapping-in-progress.html
new file mode 100644 (file)
index 0000000..7913cad
--- /dev/null
@@ -0,0 +1,74 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+    <style>
+        .container {
+            width: 150px;
+            height: 400px;
+            border: 1px solid black;
+            padding: 10px;
+            margin: 20px;
+            overflow-x: scroll;
+            scroll-snap-type: y mandatory;
+        }
+
+        .box {
+            margin: 20px;
+            width: 100px;
+            height: 100px;
+            scroll-snap-align: start;
+            background-color: blue;
+        }
+    </style>
+    <script src="../../../resources/js-test.js"></script>
+    <script src="../../../resources/ui-helper.js"></script>
+    <script>
+        jsTestIsAsync = true;
+        
+        let container = undefined;
+        async function runTest()
+        {
+            let firstScroll = true;
+            container = document.getElementById('container');
+            container.addEventListener('scroll', () => {
+                if (firstScroll) {
+                    shouldBeTrue('container.scrollTop > 0');
+                    shouldBeTrue('window.internals.isScrollSnapInProgress(container)');
+                }
+                firstScroll = false;
+            });
+            
+            debug('Scroll-snap offsets for \'container\': ' + window.internals.scrollSnapOffsets(container));
+
+            debug('Initial state:');
+            shouldBeFalse('window.internals.isScrollSnapInProgress(container)');
+
+            debug('');
+            debug('Sending wheel events');
+            await UIHelper.mouseWheelScrollAt(100, 100);
+
+            finishJSTest();
+        }
+
+        window.addEventListener('load', () => {
+            if (window.testRunner)
+                setTimeout(runTest, 0);
+        }, false);
+    </script>
+</head>
+<body>
+    <div id="container" class="container">
+        <div class="contents">
+            <div class="box"></div>
+            <div class="box"></div>
+            <div class="box"></div>
+            <div class="box"></div>
+            <div class="box"></div>
+            <div class="box"></div>
+            <div class="box"></div>
+            <div class="box"></div>
+        </div>
+    </div>
+    <div id="console"></div>
+</body>
+</html>
index e47675f..1fc664e 100644 (file)
@@ -1,3 +1,60 @@
+2020-06-24  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r260276): Scrolling through shelves on music.apple.com is not smooth
+        https://bugs.webkit.org/show_bug.cgi?id=213572
+
+        Reviewed by Wenson Hsieh.
+
+        The scroll position for an overflow:scroll element with scroll-snap could jump around
+        during scrolling, if layout triggered a call to ScrollableArea::updateScrollSnapState().
+        The crux of the issue is that isScrollSnapInProgress() returned false for overflow:scrollers
+        which are scrolling asynchronously.
+
+        Fix by extending the existing ScrollingTree::isScrollSnapInProgress() to track all scrolling
+        nodes by storing a HashSet<ScrollingNodeID> rather than just a flag for the main frame.
+
+        RenderLayer::isScrollSnapInProgress() then consults the ScrollingCoordinator, which consults
+        the scrolling tree for this state.
+
+        Add the ability to test via internals.isScrollSnapInProgress().
+
+        Test: fast/scrolling/mac/scroll-snapping-in-progress.html
+
+        * dom/Document.h:
+        * page/FrameView.cpp:
+        (WebCore::FrameView::isScrollSnapInProgress const): The main thread can run snapping logic
+        even for async-scrollable frames, so consult the ScrollingCoordinator and FrameView's ScrollAnimator.
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::isScrollSnapInProgress const):
+        * page/scrolling/AsyncScrollingCoordinator.h:
+        * page/scrolling/ScrollingCoordinator.h:
+        (WebCore::ScrollingCoordinator::isScrollSnapInProgress const):
+        * page/scrolling/ScrollingTree.cpp:
+        (WebCore::ScrollingTree::isScrollSnapInProgressForNode):
+        (WebCore::ScrollingTree::setNodeScrollSnapInProgress):
+        (WebCore::ScrollingTree::isScrollSnapInProgress): Deleted.
+        (WebCore::ScrollingTree::setMainFrameIsScrollSnapping): Deleted.
+        * page/scrolling/ScrollingTree.h:
+        * page/scrolling/ScrollingTreeScrollingNode.cpp:
+        (WebCore::ScrollingTreeScrollingNode::setScrollSnapInProgress):
+        (WebCore::ScrollingTreeScrollingNode::isScrollSnapInProgress const):
+        * page/scrolling/ScrollingTreeScrollingNode.h:
+        * page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::handleWheelEvent):
+        * page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:
+        (WebCore::ScrollingTreeScrollingNodeDelegateMac::willStartScrollSnapAnimation):
+        (WebCore::ScrollingTreeScrollingNodeDelegateMac::didStopScrollSnapAnimation):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::requestScrollPositionUpdate):
+        (WebCore::RenderLayer::isScrollSnapInProgress const):
+        (WebCore::RenderLayer::setupFontSubpixelQuantization):
+        * testing/Internals.cpp:
+        (WebCore:: const):
+        (WebCore::Internals::scrollSnapOffsets):
+        (WebCore::Internals::isScrollSnapInProgress):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2020-06-24  Jason Lawrence  <lawrence.j@apple.com>
 
         Unreviewed, reverting r263466.
index 7e6e990..ec4df9e 100644 (file)
@@ -461,7 +461,7 @@ public:
     RefPtr<Range> caretRangeFromPoint(const LayoutPoint& clientPoint);
 
     WEBCORE_EXPORT Element* scrollingElementForAPI();
-    Element* scrollingElement();
+    WEBCORE_EXPORT Element* scrollingElement();
 
     enum ReadyState { Loading, Interactive,  Complete };
     ReadyState readyState() const { return m_readyState; }
index 7b52c86..7ae7299 100644 (file)
@@ -933,19 +933,19 @@ bool FrameView::isScrollSnapInProgress() const
 {
     if (scrollbarsSuppressed())
         return false;
-    
+
     // If the scrolling thread updates the scroll position for this FrameView, then we should return
     // ScrollingCoordinator::isScrollSnapInProgress().
     if (auto scrollingCoordinator = this->scrollingCoordinator()) {
-        if (!scrollingCoordinator->shouldUpdateScrollLayerPositionSynchronously(*this))
-            return scrollingCoordinator->isScrollSnapInProgress();
+        if (scrollingCoordinator->isScrollSnapInProgress(scrollingNodeID()))
+            return true;
     }
-    
+
     // If the main thread updates the scroll position for this FrameView, we should return
     // ScrollAnimator::isScrollSnapInProgress().
     if (ScrollAnimator* scrollAnimator = existingScrollAnimator())
         return scrollAnimator->isScrollSnapInProgress();
-    
+
     return false;
 }
 
index 7bb1327..53d64da 100644 (file)
@@ -904,9 +904,9 @@ void AsyncScrollingCoordinator::setActiveScrollSnapIndices(ScrollingNodeID scrol
 #endif
 
 #if ENABLE(CSS_SCROLL_SNAP)
-bool AsyncScrollingCoordinator::isScrollSnapInProgress() const
+bool AsyncScrollingCoordinator::isScrollSnapInProgress(ScrollingNodeID nodeID) const
 {
-    return scrollingTree()->isScrollSnapInProgress();
+    return scrollingTree()->isScrollSnapInProgressForNode(nodeID);
 }
 
 void AsyncScrollingCoordinator::updateScrollSnapPropertiesWithFrameView(const FrameView& frameView)
index 2726688..b63665e 100644 (file)
@@ -132,7 +132,7 @@ private:
     void setScrollPinningBehavior(ScrollPinningBehavior) override;
 
 #if ENABLE(CSS_SCROLL_SNAP)
-    bool isScrollSnapInProgress() const override;
+    bool isScrollSnapInProgress(ScrollingNodeID) const override;
 #endif
 
     WEBCORE_EXPORT void reconcileViewportConstrainedLayerPositions(ScrollingNodeID, const LayoutRect& viewportRect, ScrollingLayerPositionAction) override;
index b1b7701..4fbde6f 100644 (file)
@@ -171,7 +171,7 @@ public:
     virtual String scrollingStateTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal) const;
     virtual String scrollingTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal) const;
     virtual bool isRubberBandInProgress() const { return false; }
-    virtual bool isScrollSnapInProgress() const { return false; }
+    virtual bool isScrollSnapInProgress(ScrollingNodeID) const { return false; }
     virtual void updateScrollSnapPropertiesWithFrameView(const FrameView&) { }
     virtual void setScrollPinningBehavior(ScrollPinningBehavior) { }
 
index 726a355..7b59a13 100644 (file)
@@ -467,16 +467,23 @@ void ScrollingTree::setMainFrameIsRubberBanding(bool isRubberBanding)
 }
 
 // Can be called from the main thread.
-bool ScrollingTree::isScrollSnapInProgress()
+bool ScrollingTree::isScrollSnapInProgressForNode(ScrollingNodeID nodeID)
 {
+    if (!nodeID)
+        return false;
+
     LockHolder lock(m_treeStateMutex);
-    return m_treeState.mainFrameIsScrollSnapping;
+    return m_treeState.nodesWithActiveScrollSnap.contains(nodeID);
 }
     
-void ScrollingTree::setMainFrameIsScrollSnapping(bool isScrollSnapping)
+void ScrollingTree::setNodeScrollSnapInProgress(ScrollingNodeID nodeID, bool isScrollSnapping)
 {
+    ASSERT(nodeID);
     LockHolder locker(m_treeStateMutex);
-    m_treeState.mainFrameIsScrollSnapping = isScrollSnapping;
+    if (isScrollSnapping)
+        m_treeState.nodesWithActiveScrollSnap.add(nodeID);
+    else
+        m_treeState.nodesWithActiveScrollSnap.remove(nodeID);
 }
 
 void ScrollingTree::setMainFramePinnedState(RectEdges<bool> edgePinningState)
index 4bdfe54..a2c43d9 100644 (file)
@@ -94,8 +94,9 @@ public:
 
     void setMainFrameIsRubberBanding(bool);
     bool isRubberBandInProgress();
-    void setMainFrameIsScrollSnapping(bool);
-    bool isScrollSnapInProgress();
+
+    void setNodeScrollSnapInProgress(ScrollingNodeID, bool);
+    bool isScrollSnapInProgressForNode(ScrollingNodeID);
 
     virtual void invalidate() { }
     WEBCORE_EXPORT virtual void commitTreeState(std::unique_ptr<ScrollingStateTree>&&);
@@ -244,8 +245,8 @@ private:
         FloatPoint mainFrameScrollPosition;
         PlatformDisplayID displayID { 0 };
         Optional<unsigned> nominalFramesPerSecond;
+        HashSet<ScrollingNodeID> nodesWithActiveScrollSnap;
         bool mainFrameIsRubberBanding { false };
-        bool mainFrameIsScrollSnapping { false };
     };
     
     Lock m_treeStateMutex;
index d07eaee..8853fc9 100644 (file)
@@ -219,6 +219,16 @@ bool ScrollingTreeScrollingNode::isRubberBanding() const
         || scrollPosition.y() > maxScrollPosition.y();
 }
 
+void ScrollingTreeScrollingNode::setScrollSnapInProgress(bool isSnapping)
+{
+    scrollingTree().setNodeScrollSnapInProgress(scrollingNodeID(), isSnapping);
+}
+
+bool ScrollingTreeScrollingNode::isScrollSnapInProgress() const
+{
+    return scrollingTree().isScrollSnapInProgressForNode(scrollingNodeID());
+}
+
 FloatPoint ScrollingTreeScrollingNode::adjustedScrollPosition(const FloatPoint& scrollPosition, ScrollClamping clamping) const
 {
     if (clamping == ScrollClamping::Clamped)
index 458eb43..898fea4 100644 (file)
@@ -68,6 +68,9 @@ public:
     RectEdges<bool> edgePinnedState() const;
     bool isRubberBanding() const;
 
+    void setScrollSnapInProgress(bool);
+    bool isScrollSnapInProgress() const;
+
     // These are imperative; they adjust the scrolling layers.
     void scrollTo(const FloatPoint&, ScrollType = ScrollType::User, ScrollClamping = ScrollClamping::Clamped);
     void scrollBy(const FloatSize&, ScrollClamping = ScrollClamping::Clamped);
index ac63a8d..a91d7f3 100644 (file)
@@ -124,8 +124,7 @@ WheelEventHandlingResult ScrollingTreeFrameScrollingNodeMac::handleWheelEvent(co
     bool handled = m_delegate.handleWheelEvent(wheelEvent);
 
 #if ENABLE(CSS_SCROLL_SNAP)
-    if (isRootNode())
-        scrollingTree().setMainFrameIsScrollSnapping(m_delegate.isScrollSnapInProgress());
+    setScrollSnapInProgress(m_delegate.isScrollSnapInProgress());
 
     if (m_delegate.activeScrollSnapIndexDidChange())
         scrollingTree().setActiveScrollSnapIndices(scrollingNodeID(), m_delegate.activeScrollSnapIndexForAxis(ScrollEventAxis::Horizontal), m_delegate.activeScrollSnapIndexForAxis(ScrollEventAxis::Vertical));
index a0bc97a..7c7264a 100644 (file)
@@ -351,12 +351,12 @@ float ScrollingTreeScrollingNodeDelegateMac::pageScaleFactor() const
 
 void ScrollingTreeScrollingNodeDelegateMac::willStartScrollSnapAnimation()
 {
-    scrollingTree().setMainFrameIsScrollSnapping(true);
+    scrollingNode().setScrollSnapInProgress(true);
 }
 
 void ScrollingTreeScrollingNodeDelegateMac::didStopScrollSnapAnimation()
 {
-    scrollingTree().setMainFrameIsScrollSnapping(false);
+    scrollingNode().setScrollSnapInProgress(false);
 }
     
 LayoutSize ScrollingTreeScrollingNodeDelegateMac::scrollExtent() const
index fe17882..6b19bc3 100644 (file)
@@ -2590,7 +2590,7 @@ bool RenderLayer::requestScrollPositionUpdate(const ScrollPosition& position, Sc
 #if ENABLE(ASYNC_SCROLLING)
     LOG_WITH_STREAM(Scrolling, stream << *this << " requestScrollPositionUpdate " << position);
 
-    if (ScrollingCoordinator* scrollingCoordinator = page().scrollingCoordinator())
+    if (auto* scrollingCoordinator = page().scrollingCoordinator())
         return scrollingCoordinator->requestScrollPositionUpdate(*this, position, scrollType, clamping);
 #endif
     return false;
@@ -3570,12 +3570,18 @@ bool RenderLayer::isScrollSnapInProgress() const
 {
     if (!scrollsOverflow())
         return false;
-    
-    if (ScrollAnimator* scrollAnimator = existingScrollAnimator())
+
+    if (auto* scrollingCoordinator = page().scrollingCoordinator()) {
+        if (scrollingCoordinator->isScrollSnapInProgress(scrollingNodeID()))
+            return true;
+    }
+
+    if (auto* scrollAnimator = existingScrollAnimator())
         return scrollAnimator->isScrollSnapInProgress();
-    
+
     return false;
 }
+
 #endif
 
 bool RenderLayer::usesMockScrollAnimator() const
@@ -4375,7 +4381,7 @@ bool RenderLayer::setupFontSubpixelQuantization(GraphicsContext& context, bool&
 
     bool scrollingOnMainThread = true;
 #if ENABLE(ASYNC_SCROLLING)
-    if (ScrollingCoordinator* scrollingCoordinator = page().scrollingCoordinator())
+    if (auto* scrollingCoordinator = page().scrollingCoordinator())
         scrollingOnMainThread = scrollingCoordinator->shouldUpdateScrollLayerPositionSynchronously(renderer().view().frameView());
 #endif
 
index d52d38f..ab095a0 100644 (file)
@@ -2841,12 +2841,21 @@ ExceptionOr<ScrollableArea*> Internals::scrollableAreaForNode(Node* node) const
             return Exception { InvalidAccessError };
 
         scrollableArea = frameView;
+    } else if (node == nodeRef->document().scrollingElement()) {
+        auto* frameView = nodeRef->document().view();
+        if (!frameView)
+            return Exception { InvalidAccessError };
+
+        scrollableArea = frameView;
     } else if (is<Element>(nodeRef)) {
         auto& element = downcast<Element>(nodeRef.get());
         if (!element.renderBox())
             return Exception { InvalidAccessError };
 
         auto& renderBox = *element.renderBox();
+        if (!renderBox.canBeScrolledAndHasScrollableArea())
+            return Exception { InvalidAccessError };
+
         if (is<RenderListBox>(renderBox))
             scrollableArea = &downcast<RenderListBox>(renderBox);
         else
@@ -4643,31 +4652,15 @@ void Internals::setPlatformMomentumScrollingPredictionEnabled(bool enabled)
 
 ExceptionOr<String> Internals::scrollSnapOffsets(Element& element)
 {
-    element.document().updateLayoutIgnorePendingStylesheets();
-
-    if (!element.renderBox())
-        return String();
-
-    RenderBox& box = *element.renderBox();
-    ScrollableArea* scrollableArea;
-
-    if (box.isBody()) {
-        FrameView* frameView = box.frame().mainFrame().view();
-        if (!frameView || !frameView->isScrollable())
-            return Exception { InvalidAccessError };
-        scrollableArea = frameView;
-
-    } else {
-        if (!box.canBeScrolledAndHasScrollableArea())
-            return Exception { InvalidAccessError };
-        scrollableArea = box.layer();
-    }
+    auto areaOrException = scrollableAreaForNode(&element);
+    if (areaOrException.hasException())
+        return areaOrException.releaseException();
 
+    auto* scrollableArea = areaOrException.releaseReturnValue();
     if (!scrollableArea)
-        return String();
+        return Exception { InvalidAccessError };
 
     StringBuilder result;
-
     if (auto* offsets = scrollableArea->horizontalSnapOffsets()) {
         if (offsets->size()) {
             result.appendLiteral("horizontal = ");
@@ -4688,6 +4681,18 @@ ExceptionOr<String> Internals::scrollSnapOffsets(Element& element)
     return result.toString();
 }
 
+ExceptionOr<bool> Internals::isScrollSnapInProgress(Element& element)
+{
+    auto areaOrException = scrollableAreaForNode(&element);
+    if (areaOrException.hasException())
+        return areaOrException.releaseException();
+
+    auto* scrollableArea = areaOrException.releaseReturnValue();
+    if (!scrollableArea)
+        return Exception { InvalidAccessError };
+
+    return scrollableArea->isScrollSnapInProgress();
+}
 #endif
 
 bool Internals::testPreloaderSettingViewport()
index daa94c8..153b373 100644 (file)
@@ -705,6 +705,7 @@ public:
 
 #if ENABLE(CSS_SCROLL_SNAP)
     ExceptionOr<String> scrollSnapOffsets(Element&);
+    ExceptionOr<bool> isScrollSnapInProgress(Element&);
     void setPlatformMomentumScrollingPredictionEnabled(bool);
 #endif
 
index a561b97..594aa5a 100644 (file)
@@ -716,6 +716,7 @@ enum CompositingPolicy {
     [Conditional=CONTENT_FILTERING] readonly attribute MockContentFilterSettings mockContentFilterSettings;
 
     [Conditional=CSS_SCROLL_SNAP, MayThrowException] DOMString scrollSnapOffsets(Element element);
+    [Conditional=CSS_SCROLL_SNAP, MayThrowException] boolean isScrollSnapInProgress(Element element);
     [Conditional=CSS_SCROLL_SNAP] void setPlatformMomentumScrollingPredictionEnabled(boolean enabled);
 
     [MayThrowException] DOMString pathStringWithShrinkWrappedRects(sequence<double> rectComponents, double radius);
index a961471..d3ce90e 100644 (file)
@@ -1,3 +1,28 @@
+2020-06-24  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r260276): Scrolling through shelves on music.apple.com is not smooth
+        https://bugs.webkit.org/show_bug.cgi?id=213572
+        <rdar://problem/64306590>
+
+        Reviewed by Wenson Hsieh.
+
+        The scroll position for an overflow:scroll element with scroll-snap could jump around
+        during scrolling, if layout triggered a call to ScrollableArea::updateScrollSnapState().
+        The crux of the issue is that isScrollSnapInProgress() returned false for overflow:scrollers
+        which are scrolling asynchronously.
+
+        Fix by extending the existing ScrollingTree::isScrollSnapInProgress() to track all scrolling
+        nodes by storing a HashSet<ScrollingNodeID> rather than just a flag for the main frame.
+
+        RenderLayer::isScrollSnapInProgress() then consults the ScrollingCoordinator, which consults
+        the scrolling tree for this state.
+
+        Add the ability to test via internals.isScrollSnapInProgress().
+
+        * WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.h:
+        * WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:
+        (WebKit::RemoteScrollingCoordinator::isScrollSnapInProgress const):
+
 2020-06-24  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, reverting r263295.
index 8e64935..24b4a73 100644 (file)
@@ -64,7 +64,7 @@ private:
 
     bool isRubberBandInProgress() const override;
     void setScrollPinningBehavior(WebCore::ScrollPinningBehavior) override;
-    bool isScrollSnapInProgress() const override;
+    bool isScrollSnapInProgress(WebCore::ScrollingNodeID) const override;
 
     // IPC::MessageReceiver
     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
index cbfa31b..588d240 100644 (file)
@@ -76,7 +76,7 @@ bool RemoteScrollingCoordinator::isRubberBandInProgress() const
     return false;
 }
 
-bool RemoteScrollingCoordinator::isScrollSnapInProgress() const
+bool RemoteScrollingCoordinator::isScrollSnapInProgress(ScrollingNodeID) const
 {
     // FIXME: need to maintain state in the web process?
     return false;