REGRESSION (UI-side compositing): Scrolling web inspector timeline is…

… very jumpy

https://bugs.webkit.org/show_bug.cgi?id=258319
rdar://107685436

Reviewed by Simon Fraser.

When receiving a wheel event from dispatchEvent, the current design recieves this event
in the web process, and sends this request to the UI-process as a RequestedScrollData
object. This object contained a scrollPosition to animate to, but this was calculated
using the scroll position in the web process. Since this scroll position could be out
of sync with the UI-process while the user was scrolling, this could result in a scroll
to a scroll position behind the current scroll position, leading to a jumpy looking scroll.
To fix this, rather than sending across the scroll position to scroll to, simply send across
the requested scroll delta to the UI-process, and use the UI-process scroll position to
calculate the correct scroll position to animate to. Also, propagate down the original
scroll delta (if it exists), to be used in AsyncScrollingCoordinator::requestScrollToPosition,
so that we know whether to do a delta or scroll position update.

* Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::requestScrollToPosition):
* Source/WebCore/page/scrolling/ScrollingCoordinatorTypes.h:
* Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in:
* Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.mm:
(WebKit::RemoteScrollingTreeMac::startPendingScrollAnimations):

Canonical link: https://commits.webkit.org/266210@main

Source/WebCore/dom/Element.cpp

@@ -1172,17 +1172,18 @@ void Element::scrollIntoViewIfNotVisible(bool centerIfNotVisible)
 void Element::scrollBy(const ScrollToOptions& options)
 {
     ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options, 0, 0);
+    FloatSize originalDelta(scrollToOptions.left.value(), scrollToOptions.top.value());
     scrollToOptions.left.value() += scrollLeft();
     scrollToOptions.top.value() += scrollTop();
-    scrollTo(scrollToOptions, ScrollClamping::Clamped, ScrollSnapPointSelectionMethod::Directional);
+    scrollTo(scrollToOptions, ScrollClamping::Clamped, ScrollSnapPointSelectionMethod::Directional, originalDelta);
 }
 
 void Element::scrollBy(double x, double y)
 {
     scrollBy(ScrollToOptions(x, y));
 }
 
-void Element::scrollTo(const ScrollToOptions& options, ScrollClamping clamping, ScrollSnapPointSelectionMethod snapPointSelectionMethod)
+void Element::scrollTo(const ScrollToOptions& options, ScrollClamping clamping, ScrollSnapPointSelectionMethod snapPointSelectionMethod, std::optional<FloatSize> originalScrollDelta)
 {
     LOG_WITH_STREAM(Scrolling, stream << "Element " << *this << " scrollTo " << options.left << ", " << options.top);
 
@@ -1206,7 +1207,7 @@ void Element::scrollTo(const ScrollToOptions& options, ScrollClamping clamping,
         if (!window)
             return;
 
-        window->scrollTo(options, clamping, snapPointSelectionMethod);
+        window->scrollTo(options, clamping, snapPointSelectionMethod, originalScrollDelta);
         return;
     }
 
@@ -1226,7 +1227,7 @@ void Element::scrollTo(const ScrollToOptions& options, ScrollClamping clamping,
     );
 
     auto animated = useSmoothScrolling(scrollToOptions.behavior.value_or(ScrollBehavior::Auto), this) ? ScrollIsAnimated::Yes : ScrollIsAnimated::No;
-    auto scrollPositionChangeOptions = ScrollPositionChangeOptions::createProgrammaticWithOptions(clamping, animated, snapPointSelectionMethod);
+    auto scrollPositionChangeOptions = ScrollPositionChangeOptions::createProgrammaticWithOptions(clamping, animated, snapPointSelectionMethod, originalScrollDelta);
     renderer->setScrollPosition(scrollPosition, scrollPositionChangeOptions);
 }
 

Source/WebCore/dom/Element.h

@@ -205,7 +205,7 @@ class Element : public ContainerNode {
 
     void scrollBy(const ScrollToOptions&);
     void scrollBy(double x, double y);
-    virtual void scrollTo(const ScrollToOptions&, ScrollClamping = ScrollClamping::Clamped, ScrollSnapPointSelectionMethod = ScrollSnapPointSelectionMethod::Closest);
+    virtual void scrollTo(const ScrollToOptions&, ScrollClamping = ScrollClamping::Clamped, ScrollSnapPointSelectionMethod = ScrollSnapPointSelectionMethod::Closest, std::optional<FloatSize> originalScrollDelta = std::nullopt);
     void scrollTo(double x, double y);
 
     // These are only used by WebKitLegacy DOM API.

Source/WebCore/page/LocalDOMWindow.cpp

@@ -1774,17 +1774,18 @@ void LocalDOMWindow::scrollBy(const ScrollToOptions& options) const
         return;
 
     ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options, 0, 0);
+    FloatSize originalDelta(scrollToOptions.left.value(), scrollToOptions.top.value());
     scrollToOptions.left.value() += view->mapFromLayoutToCSSUnits(view->contentsScrollPosition().x());
     scrollToOptions.top.value() += view->mapFromLayoutToCSSUnits(view->contentsScrollPosition().y());
-    scrollTo(scrollToOptions, ScrollClamping::Clamped, ScrollSnapPointSelectionMethod::Directional);
+    scrollTo(scrollToOptions, ScrollClamping::Clamped, ScrollSnapPointSelectionMethod::Directional, originalDelta);
 }
 
 void LocalDOMWindow::scrollTo(double x, double y, ScrollClamping clamping) const
 {
     scrollTo(ScrollToOptions(x, y), clamping);
 }
 
-void LocalDOMWindow::scrollTo(const ScrollToOptions& options, ScrollClamping clamping, ScrollSnapPointSelectionMethod snapPointSelectionMethod) const
+void LocalDOMWindow::scrollTo(const ScrollToOptions& options, ScrollClamping clamping, ScrollSnapPointSelectionMethod snapPointSelectionMethod, std::optional<FloatSize> originalScrollDelta) const
 {
     if (!isCurrentlyDisplayedInFrame())
         return;
@@ -1812,7 +1813,7 @@ void LocalDOMWindow::scrollTo(const ScrollToOptions& options, ScrollClamping cla
     // FIXME: Should we use document()->scrollingElement()?
     // See https://bugs.webkit.org/show_bug.cgi?id=205059
     auto animated = useSmoothScrolling(scrollToOptions.behavior.value_or(ScrollBehavior::Auto), document()->documentElement()) ? ScrollIsAnimated::Yes : ScrollIsAnimated::No;
-    auto scrollPositionChangeOptions = ScrollPositionChangeOptions::createProgrammaticWithOptions(clamping, animated, snapPointSelectionMethod);
+    auto scrollPositionChangeOptions = ScrollPositionChangeOptions::createProgrammaticWithOptions(clamping, animated, snapPointSelectionMethod, originalScrollDelta);
     view->setContentsScrollPosition(layoutPos, scrollPositionChangeOptions);
 }
 

Source/WebCore/page/LocalDOMWindow.h

@@ -278,7 +278,7 @@ class LocalDOMWindow final
 
     void scrollBy(const ScrollToOptions&) const;
     void scrollBy(double x, double y) const;
-    void scrollTo(const ScrollToOptions&, ScrollClamping = ScrollClamping::Clamped, ScrollSnapPointSelectionMethod = ScrollSnapPointSelectionMethod::Closest) const;
+    void scrollTo(const ScrollToOptions&, ScrollClamping = ScrollClamping::Clamped, ScrollSnapPointSelectionMethod = ScrollSnapPointSelectionMethod::Closest, std::optional<FloatSize> originalScrollDelta = std::nullopt) const;
     void scrollTo(double x, double y, ScrollClamping = ScrollClamping::Clamped) const;
 
     void moveBy(float x, float y) const;

Source/WebCore/page/LocalFrameView.cpp

@@ -2464,7 +2464,7 @@ void LocalFrameView::setScrollPosition(const ScrollPosition& scrollPosition, con
     auto snappedPosition = scrollPositionFromOffset(snappedOffset);
 
     if (options.animated == ScrollIsAnimated::Yes)
-        scrollToPositionWithAnimation(snappedPosition, currentScrollType(), options.clamping);
+        scrollToPositionWithAnimation(snappedPosition, options);
     else
         ScrollView::setScrollPosition(snappedPosition, options);
 
@@ -3085,12 +3085,12 @@ bool LocalFrameView::requestStopKeyboardScrollAnimation(bool immediate)
     return false;
 }
 
-bool LocalFrameView::requestScrollToPosition(const ScrollPosition& position, ScrollType scrollType, ScrollClamping clamping, ScrollIsAnimated animated)
+bool LocalFrameView::requestScrollToPosition(const ScrollPosition& position, const ScrollPositionChangeOptions& options)
 {
-    LOG_WITH_STREAM(Scrolling, stream << "LocalFrameView::requestScrollToPosition " << position << " animated  " << (animated == ScrollIsAnimated::Yes));
+    LOG_WITH_STREAM(Scrolling, stream << "LocalFrameView::requestScrollToPosition " << position << " options  " << options);
 
 #if ENABLE(ASYNC_SCROLLING)
-    if (auto* tiledBacking = this->tiledBacking(); tiledBacking && animated == ScrollIsAnimated::No) {
+    if (auto* tiledBacking = this->tiledBacking(); tiledBacking && options.animated == ScrollIsAnimated::No) {
 #if PLATFORM(IOS_FAMILY)
         auto contentSize = exposedContentRect().size();
 #else
@@ -3102,12 +3102,10 @@ bool LocalFrameView::requestScrollToPosition(const ScrollPosition& position, Scr
 
 #if ENABLE(ASYNC_SCROLLING) || USE(COORDINATED_GRAPHICS)
     if (auto scrollingCoordinator = this->scrollingCoordinator())
-        return scrollingCoordinator->requestScrollToPosition(*this, position, scrollType, clamping, animated);
+        return scrollingCoordinator->requestScrollToPosition(*this, position, options);
 #else
     UNUSED_PARAM(position);
-    UNUSED_PARAM(scrollType);
-    UNUSED_PARAM(clamping);
-    UNUSED_PARAM(animated);
+    UNUSED_PARAM(options);
 #endif
 
     return false;
@@ -4269,17 +4267,17 @@ void LocalFrameView::scrollTo(const ScrollPosition& newPosition)
     didChangeScrollOffset();
 }
 
-void LocalFrameView::scrollToPositionWithAnimation(const ScrollPosition& position, ScrollType scrollType, ScrollClamping)
+void LocalFrameView::scrollToPositionWithAnimation(const ScrollPosition& position, const ScrollPositionChangeOptions& options)
 {
     // FIXME: Why isn't all this in ScrollableArea?
     auto previousScrollType = currentScrollType();
-    setCurrentScrollType(scrollType);
+    setCurrentScrollType(options.type);
 
     if (scrollAnimationStatus() == ScrollAnimationStatus::Animating)
         scrollAnimator().cancelAnimations();
 
     if (position != scrollPosition())
-        ScrollableArea::scrollToPositionWithAnimation(position);
+        ScrollableArea::scrollToPositionWithAnimation(position, options);
 
     setCurrentScrollType(previousScrollType);
 }

Source/WebCore/page/LocalFrameView.h

@@ -286,7 +286,7 @@ class LocalFrameView final : public FrameView {
     bool requestStartKeyboardScrollAnimation(const KeyboardScroll&) final;
     bool requestStopKeyboardScrollAnimation(bool immediate) final;
 
-    bool requestScrollToPosition(const ScrollPosition&, ScrollType = ScrollType::Programmatic, ScrollClamping = ScrollClamping::Clamped, ScrollIsAnimated = ScrollIsAnimated::No) final;
+    bool requestScrollToPosition(const ScrollPosition&, const ScrollPositionChangeOptions& options = ScrollPositionChangeOptions::createProgrammatic()) final;
     void stopAsyncAnimatedScroll() final;
 
     bool isUserScrollInProgress() const final;
@@ -718,7 +718,7 @@ class LocalFrameView final : public FrameView {
 
     void renderLayerDidScroll(const RenderLayer&);
 
-    void scrollToPositionWithAnimation(const ScrollPosition&, ScrollType = ScrollType::Programmatic, ScrollClamping = ScrollClamping::Clamped);
+    void scrollToPositionWithAnimation(const ScrollPosition&, const ScrollPositionChangeOptions& options = ScrollPositionChangeOptions::createProgrammatic());
 
     bool inUpdateEmbeddedObjects() const { return m_inUpdateEmbeddedObjects; }
 

Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp

@@ -303,7 +303,7 @@ bool AsyncScrollingCoordinator::requestStopKeyboardScrollAnimation(ScrollableAre
     return true;
 }
 
-bool AsyncScrollingCoordinator::requestScrollToPosition(ScrollableArea& scrollableArea, const ScrollPosition& scrollPosition, ScrollType scrollType, ScrollClamping clamping, ScrollIsAnimated animated)
+bool AsyncScrollingCoordinator::requestScrollToPosition(ScrollableArea& scrollableArea, const ScrollPosition& scrollPosition, const ScrollPositionChangeOptions& options)
 {
     ASSERT(isMainThread());
     ASSERT(m_page);
@@ -325,12 +325,12 @@ bool AsyncScrollingCoordinator::requestScrollToPosition(ScrollableArea& scrollab
     bool isSnapshotting = m_page->isTakingSnapshotsForApplicationSuspension();
     bool inProgrammaticScroll = scrollableArea.currentScrollType() == ScrollType::Programmatic;
 
-    if ((inProgrammaticScroll && animated == ScrollIsAnimated::No) || inBackForwardCache) {
+    if ((inProgrammaticScroll && options.animated == ScrollIsAnimated::No) || inBackForwardCache) {
         auto scrollUpdate = ScrollUpdate { scrollingNodeID, scrollPosition, { }, ScrollUpdateType::PositionUpdate, ScrollingLayerPositionAction::Set };
         applyScrollUpdate(WTFMove(scrollUpdate), ScrollType::Programmatic);
     }
 
-    ASSERT(inProgrammaticScroll == (scrollType == ScrollType::Programmatic));
+    ASSERT(inProgrammaticScroll == (options.type == ScrollType::Programmatic));
 
     // If this frame view's document is being put into the back/forward cache, we don't want to update our
     // main frame scroll position. Just let the FrameView think that we did.
@@ -341,7 +341,13 @@ bool AsyncScrollingCoordinator::requestScrollToPosition(ScrollableArea& scrollab
     if (!stateNode)
         return false;
 
-    stateNode->setRequestedScrollData({ ScrollRequestType::PositionUpdate, scrollPosition, scrollType, clamping, animated });
+    if (options.originalScrollDelta)
+        stateNode->setRequestedScrollData({ ScrollRequestType::DeltaUpdate, *options.originalScrollDelta, options.type, options.clamping, options.animated });
+    else
+        stateNode->setRequestedScrollData({ ScrollRequestType::PositionUpdate, scrollPosition, options.type, options.clamping, options.animated });
+
+    LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::requestScrollToPosition: " << (options.animated == ScrollIsAnimated::Yes ? "isAnimated" : "isntAnimated") << " currentScrollPosition: " << scrollableArea.scrollPosition() << " scrollTo:" << (options.animated == ScrollIsAnimated::Yes ? IntPoint(scrollPosition - scrollableArea.scrollPosition()) : scrollPosition) << " requestedScrollData: " << stateNode->requestedScrollData() << "options" << options);
+
     // FIXME: This should schedule a rendering update
     commitTreeStateIfNeeded();
     return true;

Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h

@@ -112,7 +112,7 @@ class AsyncScrollingCoordinator : public ScrollingCoordinator {
     WEBCORE_EXPORT bool requestStartKeyboardScrollAnimation(ScrollableArea&, const KeyboardScroll&) final;
     WEBCORE_EXPORT bool requestStopKeyboardScrollAnimation(ScrollableArea&, bool) final;
 
-    WEBCORE_EXPORT bool requestScrollToPosition(ScrollableArea&, const ScrollPosition&, ScrollType, ScrollClamping, ScrollIsAnimated) final;
+    WEBCORE_EXPORT bool requestScrollToPosition(ScrollableArea&, const ScrollPosition&, const ScrollPositionChangeOptions& options) final;
     WEBCORE_EXPORT void stopAnimatedScroll(ScrollableArea&) final;
 
     WEBCORE_EXPORT void applyScrollingTreeLayerPositions() override;

Source/WebCore/page/scrolling/ScrollingCoordinator.h

@@ -126,7 +126,7 @@ class ScrollingCoordinator : public ThreadSafeRefCountedAndCanMakeThreadSafeWeak
     virtual bool requestStartKeyboardScrollAnimation(ScrollableArea&, const KeyboardScroll&) { return false; }
     virtual bool requestStopKeyboardScrollAnimation(ScrollableArea&, bool) { return false; }
 
-    virtual bool requestScrollToPosition(ScrollableArea&, const ScrollPosition&, ScrollType = ScrollType::Programmatic, ScrollClamping = ScrollClamping::Clamped, ScrollIsAnimated = ScrollIsAnimated::No) { return false; }
+    virtual bool requestScrollToPosition(ScrollableArea&, const ScrollPosition&, const ScrollPositionChangeOptions&) { return false; }
     virtual void stopAnimatedScroll(ScrollableArea&) { }
 
     virtual WheelEventHandlingResult handleWheelEventForScrolling(const PlatformWheelEvent&, ScrollingNodeID, std::optional<WheelScrollGestureState>) { return WheelEventHandlingResult::unhandled(WheelEventProcessingSteps::SynchronousScrolling); }

Source/WebCore/page/scrolling/ScrollingCoordinatorTypes.cpp

@@ -33,11 +33,11 @@ void RequestedScrollData::merge(RequestedScrollData&& other)
     if (other.requestType == ScrollRequestType::CancelAnimatedScroll && animated == ScrollIsAnimated::No) {
         // Carry over the previously requested scroll position so that we can set `requestedDataBeforeAnimatedScroll`
         // below in the case where the cancelled animated scroll is immediately followed by another animated scroll.
-        other.scrollPosition = scrollPosition;
-    } else if (other.requestType == ScrollRequestType::PositionUpdate && other.animated == ScrollIsAnimated::Yes) {
+        other.scrollPositionOrDelta = scrollPositionOrDelta;
+    } else if ((other.requestType == ScrollRequestType::PositionUpdate || other.requestType == ScrollRequestType::DeltaUpdate) && other.animated == ScrollIsAnimated::Yes) {
         switch (animated) {
         case ScrollIsAnimated::No:
-            other.requestedDataBeforeAnimatedScroll = { scrollPosition, scrollType, clamping };
+            other.requestedDataBeforeAnimatedScroll = { requestType, scrollPositionOrDelta, scrollType, clamping };
             break;
         case ScrollIsAnimated::Yes:
             other.requestedDataBeforeAnimatedScroll = requestedDataBeforeAnimatedScroll;
@@ -47,6 +47,11 @@ void RequestedScrollData::merge(RequestedScrollData&& other)
     *this = WTFMove(other);
 }
 
+FloatPoint RequestedScrollData::destinationPosition(const FloatPoint& currentScrollPosition) const
+{
+    return requestType == ScrollRequestType::DeltaUpdate ? (currentScrollPosition + std::get<FloatSize>(scrollPositionOrDelta)) : std::get<FloatPoint>(scrollPositionOrDelta);
+}
+
 TextStream& operator<<(TextStream& ts, SynchronousScrollingReason reason)
 {
     switch (reason) {
@@ -172,4 +177,41 @@ TextStream& operator<<(TextStream& ts, ScrollUpdateType type)
     return ts;
 }
 
+TextStream& operator<<(WTF::TextStream& ts, ScrollRequestType type)
+{
+    switch (type) {
+    case ScrollRequestType::CancelAnimatedScroll: ts << "cancel animated scroll"; break;
+    case ScrollRequestType::PositionUpdate: ts << "position update"; break;
+    case ScrollRequestType::DeltaUpdate: ts << "delta update"; break;
+    }
+    return ts;
+}
+
+TextStream& operator<<(TextStream& ts, RequestedScrollData requestedScrollData)
+{
+    ts.dumpProperty("requested-type", requestedScrollData.requestType);
+
+    if (requestedScrollData.requestType != ScrollRequestType::CancelAnimatedScroll) {
+        if (requestedScrollData.requestType == ScrollRequestType::DeltaUpdate)
+            ts.dumpProperty("requested-scroll-delta", std::get<FloatSize>(requestedScrollData.scrollPositionOrDelta));
+        else
+            ts.dumpProperty("requested-scroll-position", std::get<FloatPoint>(requestedScrollData.scrollPositionOrDelta));
+        ts.dumpProperty("requested-scroll-position-is-programatic", requestedScrollData.scrollType);
+        ts.dumpProperty("requested-scroll-position-clamping", requestedScrollData.clamping);
+        ts.dumpProperty("requested-scroll-position-animated", requestedScrollData.animated == ScrollIsAnimated::Yes);
+        if (requestedScrollData.requestedDataBeforeAnimatedScroll) {
+            auto oldType = std::get<0>(*requestedScrollData.requestedDataBeforeAnimatedScroll);
+            ts.dumpProperty("requested-scroll-position-old-data-type", oldType);
+
+            if (oldType == ScrollRequestType::DeltaUpdate)
+                ts.dumpProperty("requested-scroll-position-old-delta", std::get<FloatSize>(std::get<1>(*requestedScrollData.requestedDataBeforeAnimatedScroll)));
+            else
+                ts.dumpProperty("requested-scroll-position-old-position", std::get<FloatPoint>(std::get<1>(*requestedScrollData.requestedDataBeforeAnimatedScroll)));
+            ts.dumpProperty("requested-scroll-position-old-data-is-programatic", std::get<2>(*requestedScrollData.requestedDataBeforeAnimatedScroll));
+            ts.dumpProperty("requested-scroll-position-old-data-animated", std::get<3>(*requestedScrollData.requestedDataBeforeAnimatedScroll));
+        }
+    }
+    return ts;
+}
+
 } // namespace WebCore