REGRESSION(r255957): Element with scroll-behavior:smooth isn't draggable after r255957
authorcathiechen@igalia.com <cathiechen@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Mar 2020 05:30:54 +0000 (05:30 +0000)
committercathiechen@igalia.com <cathiechen@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Mar 2020 05:30:54 +0000 (05:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=208566

Reviewed by Simon Fraser and Frédéric Wang.

Source/WebCore:

To perform smooth scroll, RenderLayer::scrollRectToVisible checks the value of scroll-behavior.
It starts an animated scrolling if scroll-behavior is smooth.
On the other hand, the drag action would start an autoscroll if the element is scrollable.
The autoscroll uses m_autoscrollTimer which is a repeating timer, when the timer fired it calls
scrollRectToVisible with different positions.
So if performing autoscroll on scroll-bahavior: smooth element, there are two nested animations.
When timer fired, scrollRectToVisible is called, because of scroll-behavior:smooth, it starts
animated scrolling not instant scrolling. Then there's the next timer fired, the previous
animated scrolling would be canceled. Eventually, the element becomes un-draggable.
To fix this, while performing autoscroll, scrollRectToVisible shouldn't trigger animated scrolling
no matter what the value of scroll-behavior is.

Test: fast/events/drag-smooth-scroll-element.html

* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::scrollRectToVisible):
(WebCore::RenderLayer::autoscroll):
* rendering/RenderLayer.h:

LayoutTests:

The test uses eventSender to generate drag action.

* fast/events/drag-smooth-scroll-element-expected.txt: Added.
* fast/events/drag-smooth-scroll-element.html: Added.
* platform/ios/TestExpectations: IOS doesn't support mouse events, so skip the test.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/drag-smooth-scroll-element-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/drag-smooth-scroll-element.html [new file with mode: 0644]
LayoutTests/platform/ios/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayer.h

index 5b50272..e7f3df3 100644 (file)
@@ -1,3 +1,16 @@
+2020-03-12  Cathie Chen  <cathiechen@igalia.com>
+
+        REGRESSION(r255957): Element with scroll-behavior:smooth isn't draggable after r255957
+        https://bugs.webkit.org/show_bug.cgi?id=208566
+
+        Reviewed by Simon Fraser and Frédéric Wang.
+
+        The test uses eventSender to generate drag action.
+
+        * fast/events/drag-smooth-scroll-element-expected.txt: Added.
+        * fast/events/drag-smooth-scroll-element.html: Added.
+        * platform/ios/TestExpectations: IOS doesn't support mouse events, so skip the test.
+
 2020-03-12  Zalan Bujtas  <zalan@apple.com>
 
         RenderTreeNeedsLayoutChecker asserts with imported/w3c/web-platform-tests/css/css-position/position-absolute-crash-chrome-005.html
diff --git a/LayoutTests/fast/events/drag-smooth-scroll-element-expected.txt b/LayoutTests/fast/events/drag-smooth-scroll-element-expected.txt
new file mode 100644 (file)
index 0000000..6448022
--- /dev/null
@@ -0,0 +1 @@
+PASS: Test drag scroll-behavior:smooth element
diff --git a/LayoutTests/fast/events/drag-smooth-scroll-element.html b/LayoutTests/fast/events/drag-smooth-scroll-element.html
new file mode 100644 (file)
index 0000000..bdf5339
--- /dev/null
@@ -0,0 +1,74 @@
+<!DOCTYPE HTML><!-- webkit-test-runner [ experimental:CSSOMViewSmoothScrollingEnabled=true ] -->
+<style>
+#scroller {
+    width: 200px;
+    height: 200px;
+    border: 1px solid black;
+    overflow: scroll;
+    scroll-behavior: smooth;
+}
+
+#innerDiv {
+    width: 400px;
+    height: 400px;
+}
+</style>
+
+<body style="margin:0" onload="startTest()">
+<div id="scroller">
+    <div id="innerDiv">
+
+    </div>
+</div>
+<div id="log"></div>
+</body>
+
+<script>
+var initialScrollTop;
+var startYPosition;
+var endYPosition;
+var scroller = document.getElementById('scroller');
+
+function log(msg)
+{
+    document.getElementById('log').appendChild(document.createTextNode(msg + '\n'));
+}
+function testDragScrollResult()
+{
+    if (window.eventSender) {
+        // Try to prevent momentum scroll
+        eventSender.mouseMoveTo(50, endYPosition - 50);
+        eventSender.mouseUp();
+    }
+    if (scroller.scrollTop > initialScrollTop)
+        log("PASS: Test drag scroll-behavior:smooth element");
+    else
+        log("FAIL: Test drag scroll-behavior:smooth element");
+
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+
+function startTest() {
+    if (window.testRunner) {
+        testRunner.waitUntilDone();
+        testRunner.dumpAsText();
+    }
+
+    startYPosition = scroller.offsetHeight - 50;
+    endYPosition = scroller.offsetHeight + 100;
+    initialScrollTop = scroller.scrollTop;
+
+    if (window.eventSender) {
+        eventSender.dragMode = false;
+        eventSender.mouseMoveTo(50, startYPosition);
+        eventSender.mouseDown();
+        eventSender.mouseMoveTo(50, startYPosition + 20);
+        eventSender.mouseMoveTo(50, endYPosition);
+        setTimeout(testDragScrollResult, 100);
+    } else {
+        log("Test can't run without eventSender");
+    }
+}
+
+</script>
index 327e851..9f97ffa 100644 (file)
@@ -444,6 +444,7 @@ fast/events/attribute-listener-deletion-crash.html [ Skip ]
 fast/events/autoscroll-in-overflow-hidden-html.html [ Skip ]
 fast/events/autoscroll-in-textarea.html [ Skip ]
 fast/events/autoscroll-in-textfield.html [ Skip ]
+fast/events/drag-smooth-scroll-element.html [ Skip ]
 fast/events/autoscroll-main-document.html [ Skip ]
 fast/events/autoscroll-nonscrollable-iframe-in-scrollable-div.html [ Skip ]
 fast/events/autoscroll-should-not-stop-on-keypress.html [ Skip ]
index 768c2d3..3f41f40 100644 (file)
@@ -1,3 +1,29 @@
+2020-03-12  Cathie Chen  <cathiechen@igalia.com>
+
+        REGRESSION(r255957): Element with scroll-behavior:smooth isn't draggable after r255957
+        https://bugs.webkit.org/show_bug.cgi?id=208566
+
+        Reviewed by Simon Fraser and Frédéric Wang.
+
+        To perform smooth scroll, RenderLayer::scrollRectToVisible checks the value of scroll-behavior.
+        It starts an animated scrolling if scroll-behavior is smooth.
+        On the other hand, the drag action would start an autoscroll if the element is scrollable.
+        The autoscroll uses m_autoscrollTimer which is a repeating timer, when the timer fired it calls
+        scrollRectToVisible with different positions.
+        So if performing autoscroll on scroll-bahavior: smooth element, there are two nested animations.
+        When timer fired, scrollRectToVisible is called, because of scroll-behavior:smooth, it starts
+        animated scrolling not instant scrolling. Then there's the next timer fired, the previous
+        animated scrolling would be canceled. Eventually, the element becomes un-draggable.
+        To fix this, while performing autoscroll, scrollRectToVisible shouldn't trigger animated scrolling
+        no matter what the value of scroll-behavior is.
+
+        Test: fast/events/drag-smooth-scroll-element.html
+
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::scrollRectToVisible):
+        (WebCore::RenderLayer::autoscroll):
+        * rendering/RenderLayer.h:
+
 2020-03-12  Zalan Bujtas  <zalan@apple.com>
 
         RenderTreeNeedsLayoutChecker asserts with imported/w3c/web-platform-tests/css/css-position/position-absolute-crash-chrome-005.html
index 688289f..fbf3609 100644 (file)
@@ -2815,7 +2815,7 @@ bool RenderLayer::allowsCurrentScroll() const
     return box->hasHorizontalOverflow() || box->hasVerticalOverflow();
 }
 
-void RenderLayer::scrollRectToVisible(const LayoutRect& absoluteRect, bool insideFixed, const ScrollRectToVisibleOptions& options)
+void RenderLayer::scrollRectToVisible(const LayoutRect& absoluteRect, bool insideFixed, const ScrollRectToVisibleOptions& options, AutoscrollStatus autoscrollStatus)
 {
     LOG_WITH_STREAM(Scrolling, stream << "Layer " << this << " scrollRectToVisible " << absoluteRect);
 
@@ -2846,7 +2846,7 @@ void RenderLayer::scrollRectToVisible(const LayoutRect& absoluteRect, bool insid
         ScrollOffset clampedScrollOffset = clampScrollOffset(scrollOffset() + toIntSize(roundedIntRect(revealRect).location()));
         if (clampedScrollOffset != scrollOffset() || currentScrollBehaviorStatus() != ScrollBehaviorStatus::NotInAnimation) {
             ScrollOffset oldScrollOffset = scrollOffset();
-            bool animated = useSmoothScrolling(options.behavior, box->element());
+            bool animated = autoscrollStatus == AutoscrollStatus::NotInProgress && useSmoothScrolling(options.behavior, box->element());
             scrollToPosition(scrollPositionFromOffset(clampedScrollOffset), ScrollType::Programmatic, ScrollClamping::Clamped, animated);
             IntSize scrollOffsetDifference = clampedScrollOffset - oldScrollOffset;
             localExposeRect.move(-scrollOffsetDifference);
@@ -2873,7 +2873,7 @@ void RenderLayer::scrollRectToVisible(const LayoutRect& absoluteRect, bool insid
                 scrollPosition = scrollPosition.constrainedBetween(IntPoint(), IntPoint(frameView.contentsSize()));
                 // FIXME: Should we use contentDocument()->scrollingElement()?
                 // See https://bugs.webkit.org/show_bug.cgi?id=205059
-                bool animated = ownerElement->contentDocument() && useSmoothScrolling(options.behavior, ownerElement->contentDocument()->documentElement());
+                bool animated = autoscrollStatus == AutoscrollStatus::NotInProgress && ownerElement->contentDocument() && useSmoothScrolling(options.behavior, ownerElement->contentDocument()->documentElement());
                 frameView.setScrollPosition(scrollPosition, ScrollClamping::Clamped, animated);
 
                 if (options.shouldAllowCrossOriginScrolling == ShouldAllowCrossOriginScrolling::Yes || frameView.safeToPropagateScrollToParent()) {
@@ -2915,7 +2915,7 @@ void RenderLayer::scrollRectToVisible(const LayoutRect& absoluteRect, bool insid
                 ScrollOffset clampedScrollPosition = roundedIntPoint(revealRect.location()).constrainedBetween(minScrollPosition, maxScrollPosition);
                 // FIXME: Should we use document()->scrollingElement()?
                 // See https://bugs.webkit.org/show_bug.cgi?id=205059
-                bool animated = useSmoothScrolling(options.behavior, renderer().document().documentElement());
+                bool animated = autoscrollStatus == AutoscrollStatus::NotInProgress && useSmoothScrolling(options.behavior, renderer().document().documentElement());
                 frameView.setScrollPosition(clampedScrollPosition, ScrollClamping::Clamped, animated);
             }
 
@@ -3055,7 +3055,7 @@ LayoutRect RenderLayer::getRectToExpose(const LayoutRect& visibleRect, const Lay
 void RenderLayer::autoscroll(const IntPoint& positionInWindow)
 {
     IntPoint currentDocumentPosition = renderer().view().frameView().windowToContents(positionInWindow);
-    scrollRectToVisible(LayoutRect(currentDocumentPosition, LayoutSize(1, 1)), false, { SelectionRevealMode::Reveal, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignToEdgeIfNeeded, ShouldAllowCrossOriginScrolling::Yes });
+    scrollRectToVisible(LayoutRect(currentDocumentPosition, LayoutSize(1, 1)), false, { SelectionRevealMode::Reveal, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignToEdgeIfNeeded, ShouldAllowCrossOriginScrolling::Yes }, AutoscrollStatus::InProgress);
 }
 
 bool RenderLayer::canResize() const
index 1a5d0f0..96d39ef 100644 (file)
@@ -462,8 +462,9 @@ public:
 
     void availableContentSizeChanged(AvailableSizeChangeReason) override;
 
+    enum AutoscrollStatus { NotInProgress, InProgress };
     // "absoluteRect" is in scaled document coordinates.
-    void scrollRectToVisible(const LayoutRect& absoluteRect, bool insideFixed, const ScrollRectToVisibleOptions&);
+    void scrollRectToVisible(const LayoutRect& absoluteRect, bool insideFixed, const ScrollRectToVisibleOptions&, AutoscrollStatus = AutoscrollStatus::NotInProgress);
 
     bool scrollsOverflow() const;
     bool hasScrollableHorizontalOverflow() const;