Momentum scrolling ends at the wrong place when a scrolling overflow element has...
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Nov 2018 22:03:19 +0000 (22:03 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Nov 2018 22:03:19 +0000 (22:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=191322

Reviewed by Dean Jackson.
Source/WebCore:

The scrolling momentum logic in ScrollController::handleWheelEvent() which computes whether the scroll is pinned
to the end makes use of ScrollableArea::visibleContentRect(). RenderLayer's implementation of this was incorrect when
the layer's element had borders, causing the momentum scroll to stop early.

Fix by using the correct size (visible size, not layer size).

Test: fast/scrolling/momentum-scroll-with-borders.html

* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::visibleContentRectInternal const):

LayoutTests:

* fast/scrolling/momentum-scroll-with-borders-expected.txt: Added.
* fast/scrolling/momentum-scroll-with-borders.html: Added.
* platform/ios/TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/accessibility/ios-simulator/scroll-in-overflow-div-expected.txt
LayoutTests/fast/scrolling/momentum-scroll-with-borders-expected.txt [new file with mode: 0644]
LayoutTests/fast/scrolling/momentum-scroll-with-borders.html [new file with mode: 0644]
LayoutTests/platform/ios/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayer.cpp

index 4b45706..c6935e5 100644 (file)
@@ -1,5 +1,16 @@
 2018-11-27  Simon Fraser  <simon.fraser@apple.com>
 
+        Momentum scrolling ends at the wrong place when a scrolling overflow element has a non-zero border
+        https://bugs.webkit.org/show_bug.cgi?id=191322
+
+        Reviewed by Dean Jackson.
+
+        * fast/scrolling/momentum-scroll-with-borders-expected.txt: Added.
+        * fast/scrolling/momentum-scroll-with-borders.html: Added.
+        * platform/ios/TestExpectations:
+
+2018-11-27  Simon Fraser  <simon.fraser@apple.com>
+
         Composited and tiled layers fail to update on scrolling in WebView
         https://bugs.webkit.org/show_bug.cgi?id=191821
         rdar://problem/46009272
index 7035599..1717676 100644 (file)
@@ -3,41 +3,41 @@ This makes sure that we can perform accessibility scroll actions inside overflow
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-AXScrollByPage received: data: AXScroll [position: 0.00 224.00]
+AXScrollByPage received: data: AXScroll [position: 0.00 220.00]
 scroll down 0 : success true
-AXScrollByPage received: data: AXScroll [position: 0.00 448.00]
+AXScrollByPage received: data: AXScroll [position: 0.00 440.00]
 scroll down 1 : success true
-AXScrollByPage received: data: AXScroll [position: 0.00 672.00]
+AXScrollByPage received: data: AXScroll [position: 0.00 660.00]
 scroll down 2 : success true
-AXScrollByPage received: data: AXScroll [position: 0.00 712.00]
+AXScrollByPage received: data: AXScroll [position: 0.00 716.00]
 scroll down 3 : success true
 scroll down 4 : success false
-AXScrollByPage received: data: AXScroll [position: 0.00 488.00]
+AXScrollByPage received: data: AXScroll [position: 0.00 496.00]
 scroll up 0 : success true
-AXScrollByPage received: data: AXScroll [position: 0.00 264.00]
+AXScrollByPage received: data: AXScroll [position: 0.00 276.00]
 scroll up 1 : success true
-AXScrollByPage received: data: AXScroll [position: 0.00 40.00]
+AXScrollByPage received: data: AXScroll [position: 0.00 56.00]
 scroll up 2 : success true
 AXScrollByPage received: data: AXScroll [position: 0.00 0.00]
 scroll up 3 : success true
 scroll up 4 : success false
-AXScrollByPage received: data: AXScroll [position: 424.00 0.00]
+AXScrollByPage received: data: AXScroll [position: 420.00 0.00]
 scroll left 0 : success true
-AXScrollByPage received: data: AXScroll [position: 848.00 0.00]
+AXScrollByPage received: data: AXScroll [position: 840.00 0.00]
 scroll left 1 : success true
-AXScrollByPage received: data: AXScroll [position: 1272.00 0.00]
+AXScrollByPage received: data: AXScroll [position: 1260.00 0.00]
 scroll left 2 : success true
-AXScrollByPage received: data: AXScroll [position: 1696.00 0.00]
+AXScrollByPage received: data: AXScroll [position: 1680.00 0.00]
 scroll left 3 : success true
-AXScrollByPage received: data: AXScroll [position: 2120.00 0.00]
+AXScrollByPage received: data: AXScroll [position: 2100.00 0.00]
 scroll left 4 : success true
-AXScrollByPage received: data: AXScroll [position: 1696.00 0.00]
+AXScrollByPage received: data: AXScroll [position: 1680.00 0.00]
 scroll right 0 : success true
-AXScrollByPage received: data: AXScroll [position: 1272.00 0.00]
+AXScrollByPage received: data: AXScroll [position: 1260.00 0.00]
 scroll right 1 : success true
-AXScrollByPage received: data: AXScroll [position: 848.00 0.00]
+AXScrollByPage received: data: AXScroll [position: 840.00 0.00]
 scroll right 2 : success true
-AXScrollByPage received: data: AXScroll [position: 424.00 0.00]
+AXScrollByPage received: data: AXScroll [position: 420.00 0.00]
 scroll right 3 : success true
 AXScrollByPage received: data: AXScroll [position: 0.00 0.00]
 scroll right 4 : success true
diff --git a/LayoutTests/fast/scrolling/momentum-scroll-with-borders-expected.txt b/LayoutTests/fast/scrolling/momentum-scroll-with-borders-expected.txt
new file mode 100644 (file)
index 0000000..9de4482
--- /dev/null
@@ -0,0 +1 @@
+PASS: scrollTop was 700
diff --git a/LayoutTests/fast/scrolling/momentum-scroll-with-borders.html b/LayoutTests/fast/scrolling/momentum-scroll-with-borders.html
new file mode 100644 (file)
index 0000000..06fff81
--- /dev/null
@@ -0,0 +1,77 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+        #scrolling {
+            height: 300px;
+            width: 300px;
+            border: 100px solid gray;
+            border-left-width: 0px;
+            border-right-width: 0px;
+            overflow-y: scroll;
+        }
+        
+        .content {
+            height: 1000px;
+            width: 100%;
+            background-image: repeating-linear-gradient(silver, white 200px);
+        }
+    </style>
+    <script>
+        function checkForScroll()
+        {
+            var scroller = document.getElementById("scrolling");
+            var expectedScrollTop = 700;
+            
+            if (scroller.scrollTop == expectedScrollTop)
+                document.getElementById('result').textContent = "PASS: scrollTop was " + expectedScrollTop;
+            else
+                document.getElementById('result').textContent = "FAIL: scrollTop was " + scroller.scrollTop;
+
+            testRunner.notifyDone();
+        }
+        
+        function scrollTest()
+        {
+            eventSender.mouseMoveTo(100, 120);
+
+            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'began', 'none');
+            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'changed', 'none');
+            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'changed', 'none');
+            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'ended', 'none');
+            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -3, 'none', 'begin');
+            
+            let remainingScrolls = 15;
+            let sendMomentumScroll = function() {
+                eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -5, 'none', 'continue');
+                if (!--remainingScrolls) {
+                    eventSender.callAfterScrollingCompletes(checkForScroll);
+                    return;
+                }
+                requestAnimationFrame(sendMomentumScroll);
+            }
+            requestAnimationFrame(sendMomentumScroll);
+        }
+
+        function startTest()
+        {
+            if (window.eventSender) {
+                testRunner.dumpAsText();
+                testRunner.waitUntilDone();
+
+                eventSender.monitorWheelEvents();
+                setTimeout(scrollTest, 0);
+            }
+        }
+        
+        window.addEventListener('load', startTest, false);
+    </script>
+</head>
+<body>
+<div id="scrolling">
+    <div class="content"></div>
+</div>
+<div id="result"></div>
+</body>
+</html>
index f46aece..a1d3e78 100644 (file)
@@ -665,6 +665,7 @@ fast/replaced/image-map-bug16782.html [ Skip ]
 fast/replaced/image-map.html [ Skip ]
 fast/scrolling/arrow-key-scroll-in-rtl-document.html [ Skip ]
 fast/scrolling/overflow-scroll-past-max.html [ Skip ]
+fast/scrolling/momentum-scroll-with-borders.html [ Skip ]
 fast/scrolling/scroll-animator-basic-events.html [ Skip ]
 fast/scrolling/scroll-animator-overlay-scrollbars-clicked.html [ Skip ]
 fast/scrolling/scroll-animator-overlay-scrollbars-hovered.html [ Skip ]
index 473e1e2..839d809 100644 (file)
@@ -1,5 +1,23 @@
 2018-11-27  Simon Fraser  <simon.fraser@apple.com>
 
+        Momentum scrolling ends at the wrong place when a scrolling overflow element has a non-zero border
+        https://bugs.webkit.org/show_bug.cgi?id=191322
+
+        Reviewed by Dean Jackson.
+        
+        The scrolling momentum logic in ScrollController::handleWheelEvent() which computes whether the scroll is pinned
+        to the end makes use of ScrollableArea::visibleContentRect(). RenderLayer's implementation of this was incorrect when
+        the layer's element had borders, causing the momentum scroll to stop early.
+        
+        Fix by using the correct size (visible size, not layer size).
+
+        Test: fast/scrolling/momentum-scroll-with-borders.html
+
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::visibleContentRectInternal const):
+
+2018-11-27  Simon Fraser  <simon.fraser@apple.com>
+
         Composited and tiled layers fail to update on scrolling in WebView
         https://bugs.webkit.org/show_bug.cgi?id=191821
         rdar://problem/46009272
index 7354be5..0ade5d2 100644 (file)
@@ -2804,8 +2804,8 @@ IntRect RenderLayer::visibleContentRectInternal(VisibleContentRectIncludesScroll
     if (showsOverflowControls() && scrollbarInclusion == IncludeScrollbars)
         scrollbarSpace = scrollbarIntrusion();
     
-    // FIXME: This seems wrong: m_layerSize includes borders. Can we just use the ScrollableArea implementation?
-    return IntRect(scrollPosition(), IntSize(std::max(0, m_layerSize.width() - scrollbarSpace.width()), std::max(0, m_layerSize.height() - scrollbarSpace.height())));
+    auto visibleSize = this->visibleSize();
+    return { scrollPosition(), { std::max(0, visibleSize.width() - scrollbarSpace.width()), std::max(0, visibleSize.height() - scrollbarSpace.height()) } };
 }
 
 IntSize RenderLayer::overhangAmount() const