Fix smooth scroll overshooting when mouse held down in scrollbar track.

Scrollbar::autoscrollPressedPart now checks if the mouse is under the thumb's eventual (target) position, rather than its current position. BUG=578554 Review URL: https://codereview.chromium.org/1601303003 Cr-Commit-Position: refs/heads/master@{#370285}
crosswalk-project · Jan 20, 2016 · ef53ba3 · ef53ba3
1 parent c705f32
commit ef53ba3
Show file tree

Hide file tree

Showing 10 changed files with 89 additions and 16 deletions.
diff --git a/third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/track-scroll-expected.txt b/third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/track-scroll-expected.txt
@@ -0,0 +1,11 @@
+This test scrolls by clicking in the scrollbar track.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS scrollY became pageStep
+PASS scrollY is pageStep
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/track-scroll.html b/third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/track-scroll.html
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<script src="../../../resources/js-test.js"></script>
+<style>
+body {
+    height: 1800px;
+}
+</style>
+<body>
+<script>
+window.jsTestIsAsync = true;
+
+description("This test scrolls by clicking in the scrollbar track.");
+
+// Compute ScrollableArea::pageStep.
+var pageStep = innerHeight * 0.875;
+if (navigator.userAgent.indexOf("Mac OS X") >= 0)
+    pageStep = Math.max(pageStep, innerHeight - 40);
+
+onload = function() {
+    if (!window.eventSender || !window.internals) {
+        finishJSTest();
+        return;
+    }
+
+    // Turn on smooth scrolling.
+    internals.settings.setScrollAnimatorEnabled(true);
+
+    // Click in the vertical scrollbar track, below the thumb.
+    eventSender.mouseMoveTo(790, 280);
+    eventSender.mouseDown();
+    eventSender.mouseUp();
+
+    // A second click should have no effect since we will be under the thumb
+    // by the time the animation completes.
+    eventSender.mouseDown();
+    eventSender.mouseUp();
+
+    shouldBecomeEqual("scrollY", "pageStep", function() {
+        requestAnimationFrame(function() {
+            // Make sure we stopped here.
+            shouldBe("scrollY", "pageStep");
+            finishJSTest();
+       });
+    });
+};
+</script>
+</body>
diff --git a/third_party/WebKit/Source/platform/scroll/ScrollAnimator.h b/third_party/WebKit/Source/platform/scroll/ScrollAnimator.h
@@ -53,6 +53,7 @@ class PLATFORM_EXPORT ScrollAnimator final : public ScrollAnimatorBase {
 
     ScrollResultOneDimensional userScroll(ScrollbarOrientation, ScrollGranularity, float step, float delta) override;
     void scrollToOffsetWithoutAnimation(const FloatPoint&) override;
+    FloatPoint desiredTargetPosition() const override;
 
     // ScrollAnimatorCompositorCoordinator implementation.
     void tickAnimation(double monotonicTime) override;
@@ -71,8 +72,6 @@ class PLATFORM_EXPORT ScrollAnimator final : public ScrollAnimatorBase {
     WTF::TimeFunction m_timeFunction;
 
 private:
-    FloatPoint desiredTargetPosition() const;
-
     // Returns true if the animation was scheduled successfully. If animation
     // could not be scheduled (e.g. because the frame is detached), scrolls
     // immediately to the target and returns false.

diff --git a/third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h b/third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h
@@ -33,6 +33,7 @@
 
 #include "platform/PlatformExport.h"
 #include "platform/PlatformWheelEvent.h"
+#include "platform/geometry/FloatPoint.h"
 #include "platform/geometry/FloatSize.h"
 #include "platform/heap/Handle.h"
 #include "platform/scroll/ScrollAnimatorCompositorCoordinator.h"
@@ -41,7 +42,6 @@
 
 namespace blink {
 
-class FloatPoint;
 class ScrollableArea;
 class Scrollbar;
 class WebCompositorAnimationTimeline;
@@ -71,6 +71,7 @@ class PLATFORM_EXPORT ScrollAnimatorBase : public ScrollAnimatorCompositorCoordi
 
     void setCurrentPosition(const FloatPoint&);
     FloatPoint currentPosition() const;
+    virtual FloatPoint desiredTargetPosition() const { return currentPosition(); }
 
     // Returns how much of pixelDelta will be used by the underlying scrollable
     // area.

diff --git a/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp b/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp
@@ -181,11 +181,11 @@ void Scrollbar::autoscrollTimerFired(Timer<Scrollbar>*)
     autoscrollPressedPart(theme().autoscrollTimerDelay());
 }
 
-static bool thumbUnderMouse(Scrollbar& scrollbar)
+bool Scrollbar::thumbWillBeUnderMouse() const
 {
-    int thumbPos = scrollbar.theme().trackPosition(scrollbar) + scrollbar.theme().thumbPosition(scrollbar);
-    int thumbLength = scrollbar.theme().thumbLength(scrollbar);
-    return scrollbar.pressedPos() >= thumbPos && scrollbar.pressedPos() < thumbPos + thumbLength;
+    int thumbPos = theme().trackPosition(*this) + theme().thumbPosition(*this, scrollableAreaTargetPos());
+    int thumbLength = theme().thumbLength(*this);
+    return pressedPos() >= thumbPos && pressedPos() < thumbPos + thumbLength;
 }
 
 void Scrollbar::autoscrollPressedPart(double delay)
@@ -195,7 +195,7 @@ void Scrollbar::autoscrollPressedPart(double delay)
         return;
 
     // Handle the track.
-    if ((m_pressedPart == BackTrackPart || m_pressedPart == ForwardTrackPart) && thumbUnderMouse(*this)) {
+    if ((m_pressedPart == BackTrackPart || m_pressedPart == ForwardTrackPart) && thumbWillBeUnderMouse()) {
         setHoveredPart(ThumbPart);
         return;
     }
@@ -213,7 +213,7 @@ void Scrollbar::startTimerIfNeeded(double delay)
 
     // Handle the track.  We halt track scrolling once the thumb is level
     // with us.
-    if ((m_pressedPart == BackTrackPart || m_pressedPart == ForwardTrackPart) && thumbUnderMouse(*this)) {
+    if ((m_pressedPart == BackTrackPart || m_pressedPart == ForwardTrackPart) && thumbWillBeUnderMouse()) {
         setHoveredPart(ThumbPart);
         return;
     }
@@ -562,6 +562,17 @@ float Scrollbar::scrollableAreaCurrentPos() const
     return m_scrollableArea->scrollPosition().y() - m_scrollableArea->minimumScrollPosition().y();
 }
 
+float Scrollbar::scrollableAreaTargetPos() const
+{
+    if (!m_scrollableArea)
+        return 0;
+
+    if (m_orientation == HorizontalScrollbar)
+        return m_scrollableArea->scrollAnimator().desiredTargetPosition().x() - m_scrollableArea->minimumScrollPosition().x();
+
+    return m_scrollableArea->scrollAnimator().desiredTargetPosition().y() - m_scrollableArea->minimumScrollPosition().y();
+}
+
 void Scrollbar::setNeedsPaintInvalidation(ScrollbarPart invalidParts)
 {
     if (m_theme.shouldRepaintAllPartsOnInvalidation())

diff --git a/third_party/WebKit/Source/platform/scroll/Scrollbar.h b/third_party/WebKit/Source/platform/scroll/Scrollbar.h
@@ -222,6 +222,8 @@ class PLATFORM_EXPORT Scrollbar : public Widget, public ScrollbarThemeClient {
     void invalidateRect(const IntRect&) override { setNeedsPaintInvalidation(AllParts); }
 
     float scrollableAreaCurrentPos() const;
+    float scrollableAreaTargetPos() const;
+    bool thumbWillBeUnderMouse() const;
 
     bool m_trackNeedsRepaint;
     bool m_thumbNeedsRepaint;

diff --git a/third_party/WebKit/Source/platform/scroll/ScrollbarTheme.cpp b/third_party/WebKit/Source/platform/scroll/ScrollbarTheme.cpp
@@ -207,14 +207,14 @@ bool ScrollbarTheme::shouldSnapBackToDragOrigin(const ScrollbarThemeClient& scro
     return Platform::current()->scrollbarBehavior()->shouldSnapBackToDragOrigin(mousePosition, trackRect(scrollbar), scrollbar.orientation() == HorizontalScrollbar);
 }
 
-int ScrollbarTheme::thumbPosition(const ScrollbarThemeClient& scrollbar)
+int ScrollbarTheme::thumbPosition(const ScrollbarThemeClient& scrollbar, float scrollPosition)
 {
     if (scrollbar.enabled()) {
         float size = scrollbar.totalSize() - scrollbar.visibleSize();
         // Avoid doing a floating point divide by zero and return 1 when usedTotalSize == visibleSize.
         if (!size)
             return 0;
-        float pos = std::max(0.0f, scrollbar.currentPos()) * (trackLength(scrollbar) - thumbLength(scrollbar)) / size;
+        float pos = std::max(0.0f, scrollPosition) * (trackLength(scrollbar) - thumbLength(scrollbar)) / size;
         return (pos < 1 && pos > 0) ? 1 : pos;
     }
     return 0;

diff --git a/third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h b/third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h
@@ -30,14 +30,14 @@
 #include "platform/geometry/IntRect.h"
 #include "platform/graphics/paint/DisplayItem.h"
 #include "platform/scroll/ScrollTypes.h"
+#include "platform/scroll/ScrollbarThemeClient.h"
 #include "public/platform/WebScrollbarButtonsPlacement.h"
 
 namespace blink {
 
 class CullRect;
 class GraphicsContext;
 class PlatformMouseEvent;
-class ScrollbarThemeClient;
 class ScrollbarThemePaintParams;
 
 class PLATFORM_EXPORT ScrollbarTheme {
@@ -85,7 +85,9 @@ class PLATFORM_EXPORT ScrollbarTheme {
     virtual bool shouldDragDocumentInsteadOfThumb(const ScrollbarThemeClient&, const PlatformMouseEvent&) { return false; }
 
     // The position of the thumb relative to the track.
-    virtual int thumbPosition(const ScrollbarThemeClient&);
+    int thumbPosition(const ScrollbarThemeClient& scrollbar) { return thumbPosition(scrollbar, scrollbar.currentPos()); }
+    // The position the thumb would have, relative to the track, at the specified scroll position.
+    virtual int thumbPosition(const ScrollbarThemeClient&, float scrollPosition);
     // The length of the thumb along the axis of the scrollbar.
     virtual int thumbLength(const ScrollbarThemeClient&);
     // The position of the track relative to the scrollbar.

diff --git a/third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp b/third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp
@@ -73,13 +73,13 @@ bool ScrollbarThemeOverlay::usesOverlayScrollbars() const
     return true;
 }
 
-int ScrollbarThemeOverlay::thumbPosition(const ScrollbarThemeClient& scrollbar)
+int ScrollbarThemeOverlay::thumbPosition(const ScrollbarThemeClient& scrollbar, float scrollPosition)
 {
     if (!scrollbar.totalSize())
         return 0;
 
     int trackLen = trackLength(scrollbar);
-    float proportion = static_cast<float>(scrollbar.currentPos()) / scrollbar.totalSize();
+    float proportion = static_cast<float>(scrollPosition) / scrollbar.totalSize();
     return round(proportion * trackLen);
 }
 

diff --git a/third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.h b/third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.h
@@ -45,7 +45,7 @@ class PLATFORM_EXPORT ScrollbarThemeOverlay : public ScrollbarTheme {
     int scrollbarMargin() const override;
     bool usesOverlayScrollbars() const override;
 
-    int thumbPosition(const ScrollbarThemeClient&) override;
+    int thumbPosition(const ScrollbarThemeClient&, float scrollPosition) override;
     int thumbLength(const ScrollbarThemeClient&) override;
 
     bool hasButtons(const ScrollbarThemeClient&) override { return false; }