From 6d000ed531e2dfd6294d8ae4f41f0bec65368434 Mon Sep 17 00:00:00 2001 From: Steve Kobes Date: Thu, 21 Jan 2016 09:31:14 -0800 Subject: [PATCH] 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} (cherry picked from commit ef53ba38118cb84ed66bb9d9335e8b3a550865d6) Review URL: https://codereview.chromium.org/1617893002 . Cr-Commit-Position: refs/branch-heads/2623@{#42} Cr-Branched-From: 92d77538a86529ca35f9220bd3cd512cbea1f086-refs/heads/master@{#369907} --- .../smooth-scroll/track-scroll-expected.txt | 11 +++++ .../smooth-scroll/track-scroll.html | 47 +++++++++++++++++++ .../Source/platform/scroll/ScrollAnimator.h | 3 +- .../platform/scroll/ScrollAnimatorBase.h | 3 +- .../Source/platform/scroll/Scrollbar.cpp | 23 ++++++--- .../WebKit/Source/platform/scroll/Scrollbar.h | 2 + .../Source/platform/scroll/ScrollbarTheme.cpp | 4 +- .../Source/platform/scroll/ScrollbarTheme.h | 6 ++- .../platform/scroll/ScrollbarThemeOverlay.cpp | 4 +- .../platform/scroll/ScrollbarThemeOverlay.h | 2 +- 10 files changed, 89 insertions(+), 16 deletions(-) create mode 100644 third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/track-scroll-expected.txt create mode 100644 third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/track-scroll.html 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 new file mode 100644 index 0000000000000..3d1d3eae9fc96 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..1b90c01a02573 --- /dev/null +++ b/third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/track-scroll.html @@ -0,0 +1,47 @@ + + + + + + diff --git a/third_party/WebKit/Source/platform/scroll/ScrollAnimator.h b/third_party/WebKit/Source/platform/scroll/ScrollAnimator.h index 88465cad8c954..b225833840227 100644 --- 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 index bdbf1bbba9fb0..41f03875d70c4 100644 --- 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 index dd67570f155ea..8944a1934358d 100644 --- a/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp +++ b/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp @@ -179,11 +179,11 @@ void Scrollbar::autoscrollTimerFired(Timer*) 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) @@ -193,7 +193,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; } @@ -211,7 +211,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; } @@ -560,6 +560,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 index 295a0e8855eff..d61e2db8024bb 100644 --- a/third_party/WebKit/Source/platform/scroll/Scrollbar.h +++ b/third_party/WebKit/Source/platform/scroll/Scrollbar.h @@ -221,6 +221,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 index 4ba3e53da101f..e33d72715be56 100644 --- 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 index 8103c98e391c6..186a617469168 100644 --- a/third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h +++ b/third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h @@ -30,6 +30,7 @@ #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 { @@ -37,7 +38,6 @@ namespace blink { class CullRect; class GraphicsContext; class PlatformMouseEvent; -class ScrollbarThemeClient; class ScrollbarThemePaintParams; class PLATFORM_EXPORT ScrollbarTheme { @@ -79,7 +79,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 index a5a12bb6c5e85..09b47a04bfe38 100644 --- 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(scrollbar.currentPos()) / scrollbar.totalSize(); + float proportion = static_cast(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 index 0af2c0444fc2d..260a3b148e825 100644 --- 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; }