Changeset 279564 in webkit

Timestamp:

Jul 5, 2021 2:21:05 AM (13 months ago)

Author:

Martin Robinson

Message:

[css-scroll-snap] Triggering a layout during scroll causes jittery scrolling on Mac when dragging the scrollbar
​https://bugs.webkit.org/show_bug.cgi?id=227478

Reviewed by Simon Fraser.

Source/WebCore:

When dragging the scrollbar thumb, wait to resnap after layout for a
given axis until the mouse button is up on the scrollbar. This prevents
the layout and the scrollbar from fighting to set the scroll position
which causes some pretty terrible jitter in this case.

Test: css3/scroll-snap/scroll-snap-drag-scrollbar-thumb-with-relayouts.html

• platform/ScrollAnimationSmooth.cpp:

(WebCore::ScrollAnimationSmooth::updatePerAxisData): Fix an issue where the new position
was calculated incorrectly. This did not cause the animation to land on the wrong place,
but did cause a very janky animation progression when retriggering animations to the
same location.

• platform/ScrollAnimator.cpp:

(WebCore::ScrollAnimator::retargetRunningAnimation): Added this helper, which retargets a
a running animation to allow for a smooth transition during relayouts.

• platform/ScrollAnimator.h: Added new method definition.
• platform/ScrollableArea.cpp:

(WebCore::ScrollableArea::resnapAfterLayout): Only snap after layout when the user is not
currently interacting with the scrollbar. Once the scrollbar isn't being interacted with,
snapping will occur already. When there is already some sort of animation in progress,
smoothly transition that animation to land on the new position instead of snapping there
immediately.

LayoutTests:

• css3/scroll-snap/scroll-snap-drag-scrollbar-thumb-with-relayouts-expected.txt: Added.
• css3/scroll-snap/scroll-snap-drag-scrollbar-thumb-with-relayouts.html: Added.
• platform/ios/TestExpectations: Skip this test on iOS since it uses a scrollbar thumb drag.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/css3/scroll-snap/scroll-snap-drag-scrollbar-thumb-with-relayouts-expected.txt (added)
• LayoutTests/css3/scroll-snap/scroll-snap-drag-scrollbar-thumb-with-relayouts.html (added)
• LayoutTests/platform/ios/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/ScrollAnimationSmooth.cpp (modified) (1 diff)
• Source/WebCore/platform/ScrollAnimator.cpp (modified) (1 diff)
• Source/WebCore/platform/ScrollAnimator.h (modified) (1 diff)
• Source/WebCore/platform/ScrollableArea.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-07-05  Martin Robinson  <mrobinson@webkit.org>
+
+        [css-scroll-snap] Triggering a layout during scroll causes jittery scrolling on Mac when dragging the scrollbar
+        https://bugs.webkit.org/show_bug.cgi?id=227478
+
+        Reviewed by Simon Fraser.
+
+        * css3/scroll-snap/scroll-snap-drag-scrollbar-thumb-with-relayouts-expected.txt: Added.
+        * css3/scroll-snap/scroll-snap-drag-scrollbar-thumb-with-relayouts.html: Added.
+        * platform/ios/TestExpectations: Skip this test on iOS since it uses a scrollbar thumb drag.
+
 2021-07-04  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/LayoutTests/platform/ios/TestExpectations

@@ -2288 +2288 @@
 webkit.org/b/157201 fast/scrolling/rtl-drag-vertical-scroller.html [ Failure ]
 webkit.org/b/157201 css3/scroll-snap/scroll-snap-drag-scrollbar-thumb.html [ Failure ]
+webkit.org/b/157201 css3/scroll-snap/scroll-snap-drag-scrollbar-thumb-with-relayouts.html [ Failure ]
 
 # DumpRenderTree doesn't support logging calls to runOpenPanel and iOS's WebKitTestRunner doesn't support eventSender.mouseDown.

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-07-05  Martin Robinson  <mrobinson@webkit.org>
+
+        [css-scroll-snap] Triggering a layout during scroll causes jittery scrolling on Mac when dragging the scrollbar
+        https://bugs.webkit.org/show_bug.cgi?id=227478
+
+        Reviewed by Simon Fraser.
+
+        When dragging the scrollbar thumb, wait to resnap after layout for a
+        given axis until the mouse button is up on the scrollbar. This prevents
+        the layout and the scrollbar from fighting to set the scroll position
+        which causes some pretty terrible jitter in this case.
+
+        Test: css3/scroll-snap/scroll-snap-drag-scrollbar-thumb-with-relayouts.html
+
+        * platform/ScrollAnimationSmooth.cpp:
+        (WebCore::ScrollAnimationSmooth::updatePerAxisData): Fix an issue where the new position
+        was calculated incorrectly. This did not cause the animation to land on the wrong place,
+        but did cause a very janky animation progression when retriggering animations to the
+        same location.
+        * platform/ScrollAnimator.cpp:
+        (WebCore::ScrollAnimator::retargetRunningAnimation): Added this helper, which retargets a
+        a running animation to allow for a smooth transition during relayouts.
+        * platform/ScrollAnimator.h: Added new method definition.
+        * platform/ScrollableArea.cpp:
+        (WebCore::ScrollableArea::resnapAfterLayout): Only snap after layout when the user is not
+        currently interacting with the scrollbar. Once the scrollbar isn't being interacted with,
+        snapping will occur already. When there is already some sort of animation in progress,
+        smoothly transition that animation to land on the new position instead of snapping there
+        immediately.
+
 2021-07-04  Alexey Shvayka  <shvaikalesh@gmail.com>
 

trunk/Source/WebCore/platform/ScrollAnimationSmooth.cpp

@@ -294 +294 @@
         data.startTime = { };
     }
-    float newPosition = data.desiredPosition + delta;
-
+
+    float newPosition = data.currentPosition + delta;
     newPosition = std::max(std::min(newPosition, maxScrollPosition), minScrollPosition);
-
     if (newPosition == data.desiredPosition)
         return false;
+
 
     Seconds animationTime, repeatMinimumSustainTime, attackTime, releaseTime, maximumCoastTime;

trunk/Source/WebCore/platform/ScrollAnimator.cpp

@@ -147 +147 @@
 }
 
+void ScrollAnimator::retargetRunningAnimation(const FloatPoint& newPosition)
+{
+    ASSERT(scrollableArea().currentScrollBehaviorStatus() == ScrollBehaviorStatus::InNonNativeAnimation);
+    ASSERT(m_scrollAnimation->isActive());
+    m_scrollAnimation->scroll(newPosition);
+}
+
 FloatPoint ScrollAnimator::offsetFromPosition(const FloatPoint& position)
 {

trunk/Source/WebCore/platform/ScrollAnimator.h

@@ -73 +73 @@
     bool scrollToOffsetWithoutAnimation(const FloatPoint&, ScrollClamping = ScrollClamping::Clamped);
     virtual bool scrollToPositionWithoutAnimation(const FloatPoint& position, ScrollClamping = ScrollClamping::Clamped);
+    virtual void retargetRunningAnimation(const FloatPoint&);
 
     bool scrollToOffsetWithAnimation(const FloatPoint&);

trunk/Source/WebCore/platform/ScrollableArea.cpp

@@ -530 +530 @@
     auto currentOffset = scrollOffset();
     auto correctedOffset = currentOffset;
-    const auto& horizontal = info->horizontalSnapOffsets;
-    auto activeHorizontalIndex = currentHorizontalSnapPointIndex();
-    if (activeHorizontalIndex)
-        correctedOffset.setX(horizontal[*activeHorizontalIndex].offset.toInt());
-
-    const auto& vertical = info->verticalSnapOffsets;
-    auto activeVerticalIndex = currentVerticalSnapPointIndex();
-    if (activeVerticalIndex)
-        correctedOffset.setY(vertical[*activeVerticalIndex].offset.toInt());
+
+    if (!horizontalScrollbar() || horizontalScrollbar()->pressedPart() == ScrollbarPart::NoPart) {
+        const auto& horizontal = info->horizontalSnapOffsets;
+        auto activeHorizontalIndex = currentHorizontalSnapPointIndex();
+        if (activeHorizontalIndex)
+            correctedOffset.setX(horizontal[*activeHorizontalIndex].offset.toInt());
+    }
+
+    if (!verticalScrollbar() || verticalScrollbar()->pressedPart() == ScrollbarPart::NoPart) {
+        const auto& vertical = info->verticalSnapOffsets;
+        auto activeVerticalIndex = currentVerticalSnapPointIndex();
+        if (activeVerticalIndex)
+            correctedOffset.setY(vertical[*activeVerticalIndex].offset.toInt());
+    }
 
     if (correctedOffset != currentOffset) {
         LOG_WITH_STREAM(ScrollSnap, stream << " adjusting offset from " << currentOffset << " to " << correctedOffset);
-        scrollToOffsetWithoutAnimation(correctedOffset);
+        auto position = scrollPositionFromOffset(correctedOffset);
+        if (currentScrollBehaviorStatus() == ScrollBehaviorStatus::NotInAnimation)
+            scrollToOffsetWithoutAnimation(correctedOffset);
+        else
+            scrollAnimator->retargetRunningAnimation(position);
     }
 }