Lyrics jump around in Spotify

https://bugs.webkit.org/show_bug.cgi?id=263946 rdar://116843612 Reviewed by Tim Horton. The `RequestedScrollData` that's processed on the scrolling thread or in the UI process for a programmatic scroll can contain secondary data that says what to do before starting an animated scroll, in `requestedDataBeforeAnimatedScroll`. The three places that process this data were failing to take into account the `ScrollRequestType::CancelAnimatedScroll` request type, which resulted in using `positionOrDeltaBeforeAnimatedScroll` to do an initial non-animated scroll, which would always end up as 0,0 thus triggering the scroll to top behavior. * LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero-expected.txt: Added. * LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero-overflow-expected.txt: Added. * LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero-overflow.html: Added. * LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero.html: Added. * Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp: (WebCore::ScrollingTreeScrollingNode::handleScrollPositionRequest): * Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm: (WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTreeTransaction): * Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.mm: (WebKit::RemoteScrollingTreeMac::startPendingScrollAnimations): Canonical link: https://commits.webkit.org/270077@main
WebKit · Nov 1, 2023 · c231467 · c231467
1 parent 37c77f4
commit c231467
Show file tree

Hide file tree

Showing 7 changed files with 149 additions and 5 deletions.
diff --git a/LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero-expected.txt b/LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero-expected.txt
@@ -0,0 +1,9 @@
+When canceling an animated scroll, we should not trigger a scroll from zero.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/...s/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero-overflow-expected.txt b/...s/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero-overflow-expected.txt
@@ -0,0 +1,9 @@
+When canceling an animated scroll, we should not trigger a scroll from zero.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero-overflow.html b/LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero-overflow.html
@@ -0,0 +1,57 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+    <style>
+        .scroller {
+            width: 300px;
+            height: 300px;
+            overflow: scroll;
+            border: 1px solid black;
+        }
+
+        .content {
+            height: 2000px;
+            background-image: repeating-linear-gradient(transparent, silver 200px);
+        }
+    </style>
+    <script src="../../resources/ui-helper.js"></script>
+    <script src="../../resources/js-test-pre.js"></script>
+    <script>
+        jsTestIsAsync = true;
+
+        description('When canceling an animated scroll, we should not trigger a scroll from zero.');
+        window.addEventListener('load', async () => {
+            let scroller = document.getElementsByClassName('scroller')[0];
+
+            const startingScrollOffset = 400;
+            scroller.scrollTo(0, startingScrollOffset);
+
+            scroller.addEventListener('scroll', () => {
+                if (scroller.scrollTop < startingScrollOffset)
+                    testFailed(`We should never see a scroll offset less than ${startingScrollOffset} - saw ${scroller.scrollTop}`);
+            }, false);
+
+            setTimeout(() => {
+                scroller.scrollTo({ top: 500, behavior: 'smooth' });
+            }, 10);
+
+            await UIHelper.startMonitoringWheelEvents();
+
+            setTimeout(() => {
+                scroller.scrollTo({ top: 520, behavior: 'smooth' });
+            }, 60);
+
+            await UIHelper.waitForScrollCompletion();
+            finishJSTest();
+
+        }, false);
+    </script>
+</head>
+<body>
+    <div class="scroller">
+        <div class="content"></div>
+    </div>
+    <div id="console"></div>
+    <script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero.html b/LayoutTests/fast/scrolling/cancel-animated-scroll-should-not-scroll-from-zero.html
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        body {
+            height: 5000px;
+            background-image: repeating-linear-gradient(transparent, silver 500px);
+        }
+    </style>
+    <script src="../../resources/ui-helper.js"></script>
+    <script src="../../resources/js-test-pre.js"></script>
+    <script>
+        jsTestIsAsync = true;
+
+        description('When canceling an animated scroll, we should not trigger a scroll from zero.');
+        window.addEventListener('load', async () => {
+            const startingScrollOffset = 400;
+            window.scrollTo(0, startingScrollOffset);
+
+            window.addEventListener('scroll', () => {
+                if (window.scrollTop < startingScrollOffset)
+                    testFailed(`We should never see a scroll offset less than ${startingScrollOffset} - saw ${window.scrollTop}`);
+            }, false);
+
+            setTimeout(() => {
+                window.scrollTo({ top: 500, behavior: 'smooth' });
+            }, 10);
+
+            await UIHelper.startMonitoringWheelEvents();
+
+            setTimeout(() => {
+                window.scrollTo({ top: 520, behavior: 'smooth' });
+            }, 60);
+
+            await UIHelper.waitForScrollCompletion();
+            finishJSTest();
+
+        }, false);
+    </script>
+</head>
+<body>
+    <div class="scroller">
+        <div class="content"></div>
+    </div>
+    <div id="console"></div>
+    <script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp b/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
@@ -317,8 +317,18 @@ void ScrollingTreeScrollingNode::handleScrollPositionRequest(const RequestedScro
 
     if (requestedScrollData.requestedDataBeforeAnimatedScroll) {
         auto& [requestType, positionOrDeltaBeforeAnimatedScroll, scrollType, clamping] = *requestedScrollData.requestedDataBeforeAnimatedScroll;
-        auto intermediatePosition = RequestedScrollData::computeDestinationPosition(currentScrollPosition, requestType, positionOrDeltaBeforeAnimatedScroll);
-        scrollTo(intermediatePosition, scrollType, clamping);
+
+        switch (requestType) {
+        case ScrollRequestType::PositionUpdate:
+        case ScrollRequestType::DeltaUpdate: {
+            auto intermediatePosition = RequestedScrollData::computeDestinationPosition(currentScrollPosition, requestType, positionOrDeltaBeforeAnimatedScroll);
+            scrollTo(intermediatePosition, scrollType, clamping);
+            break;
+        }
+        case ScrollRequestType::CancelAnimatedScroll:
+            stopAnimatedScroll();
+            break;
+        }
     }
 
     auto destinationPosition = requestedScrollData.destinationPosition(currentScrollPosition);

diff --git a/Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm b/Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm
@@ -282,7 +282,8 @@
         auto currentScrollPosition = webPageProxy->scrollingCoordinatorProxy()->currentMainFrameScrollPosition();
         if (auto previousData = std::exchange(requestedScroll->requestedDataBeforeAnimatedScroll, std::nullopt)) {
             auto& [requestType, positionOrDeltaBeforeAnimatedScroll, scrollType, clamping] = *previousData;
-            currentScrollPosition = RequestedScrollData::computeDestinationPosition(currentScrollPosition, requestType, positionOrDeltaBeforeAnimatedScroll);
+            if (requestType != ScrollRequestType::CancelAnimatedScroll)
+                currentScrollPosition = RequestedScrollData::computeDestinationPosition(currentScrollPosition, requestType, positionOrDeltaBeforeAnimatedScroll);
         }
 
         webPageProxy->requestScroll(requestedScroll->destinationPosition(currentScrollPosition), layerTreeTransaction.scrollOrigin(), requestedScroll->animated);

diff --git a/Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.mm b/Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.mm
@@ -165,8 +165,18 @@
 
         if (auto previousData = std::exchange(data.requestedDataBeforeAnimatedScroll, std::nullopt)) {
             auto& [requestType, positionOrDeltaBeforeAnimatedScroll, scrollType, clamping] = *previousData;
-            auto intermediatePosition = RequestedScrollData::computeDestinationPosition(currentScrollPosition, requestType, positionOrDeltaBeforeAnimatedScroll);
-            targetNode->scrollTo(intermediatePosition, scrollType, clamping);
+
+            switch (requestType) {
+            case ScrollRequestType::PositionUpdate:
+            case ScrollRequestType::DeltaUpdate: {
+                auto intermediatePosition = RequestedScrollData::computeDestinationPosition(currentScrollPosition, requestType, positionOrDeltaBeforeAnimatedScroll);
+                targetNode->scrollTo(intermediatePosition, scrollType, clamping);
+                break;
+            }
+            case ScrollRequestType::CancelAnimatedScroll:
+                targetNode->stopAnimatedScroll();
+                break;
+            }
         }
 
         targetNode->startAnimatedScrollToPosition(data.destinationPosition(currentScrollPosition));