Some cleanup in ScrollAnimator
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Jan 2016 21:58:48 +0000 (21:58 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Jan 2016 21:58:48 +0000 (21:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=152649

Reviewed by Zalan Bujtas.
Source/WebCore:

Change ScrollAnimatorMac::adjustScrollPositionIfNecessary() and similar code in
ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary() to
constrain between minimumScrollPosition() and maximumScrollPosition(), rather than
rolling their own code.

This revealed several issues. First, RenderLayer::maximumScrollPosition() is
wrong when the layer has borders, because RenderLayer::visibleContentRectInternal()
seems to have incorrect logic. However, we can just remove it, and use the ScrollableArea
implementation.

Second, ScrollAnimatorMac::scrollToOffsetWithoutAnimation() was failing to do a
position/offset conversion, so do one. We're converting too much, and should probably
just change ScrollAnimator to do everything in terms of positions.

Third, ScrollAnimator::scroll() was clamping a scroll position as an offset
(detected by scrollbars/scroll-rtl-or-bt-layer.html), so fix that.

Remove ScrollController::absoluteScrollPosition() and overrides, since this was
confusingly named, and could just be removed.

Remove ScrollController::m_origOrigin which was assigned to, but never read.

Test: fast/scrolling/arrow-key-scroll-in-rtl-document.html: new
      fast/dom/horizontal-scrollbar-in-rtl.html: progressed with these changes.

* page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:
* page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
(WebCore::ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary):
(WebCore::ScrollingTreeFrameScrollingNodeMac::absoluteScrollPosition): Deleted.
* platform/ScrollAnimator.cpp:
(WebCore::ScrollAnimator::scroll):
(WebCore::ScrollAnimator::notifyPositionChanged):
* platform/ScrollableArea.h:
(WebCore::ScrollableArea::constrainScrollPosition):
* platform/cocoa/ScrollController.h:
* platform/cocoa/ScrollController.mm:
(WebCore::ScrollController::snapRubberBandTimerFired): Deleted.
(WebCore::ScrollController::snapRubberBand): Deleted.
* platform/mac/ScrollAnimatorMac.h:
* platform/mac/ScrollAnimatorMac.mm:
(-[WebScrollAnimationHelperDelegate _immediateScrollToPoint:]):
(WebCore::ScrollAnimatorMac::scroll):
(WebCore::ScrollAnimatorMac::scrollToOffsetWithoutAnimation):
(WebCore::ScrollAnimatorMac::adjustScrollPositionIfNecessary):
(WebCore::ScrollAnimatorMac::adjustScrollPositionToBoundsIfNecessary):
(WebCore::ScrollAnimatorMac::immediateScrollToPosition):
(WebCore::ScrollAnimatorMac::immediateScrollToPositionForScrollAnimation):
(WebCore::ScrollAnimatorMac::immediateScrollTo): Deleted.
(WebCore::ScrollAnimatorMac::immediateScrollToPointForScrollAnimation): Deleted.
(WebCore::ScrollAnimatorMac::absoluteScrollPosition): Deleted.
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::visibleContentRectInternal):
(WebCore::RenderLayer::overhangAmount):
(WebCore::RenderLayer::maximumScrollPosition): Deleted.
* rendering/RenderLayer.h:
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::minimumScrollPosition):
(WebCore::RenderListBox::maximumScrollPosition): RenderListBox scrolls by lines,
so it needs a custom implementation of this.
* rendering/RenderListBox.h:

LayoutTests:

Added fast/scrolling/arrow-key-scroll-in-rtl-document.html to test for arrow
key scrolling in an RTL document, which an earlier version of the patch
regressed without detection.

* fast/dom/horizontal-scrollbar-in-rtl-expected.txt:
* fast/scrolling/arrow-key-scroll-in-rtl-document-expected.txt: Added.
* fast/scrolling/arrow-key-scroll-in-rtl-document.html: Added.

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

17 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/horizontal-scrollbar-in-rtl-expected.txt
LayoutTests/fast/scrolling/arrow-key-scroll-in-rtl-document-expected.txt [new file with mode: 0644]
LayoutTests/fast/scrolling/arrow-key-scroll-in-rtl-document.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h
Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm
Source/WebCore/platform/ScrollAnimator.cpp
Source/WebCore/platform/ScrollableArea.h
Source/WebCore/platform/cocoa/ScrollController.h
Source/WebCore/platform/cocoa/ScrollController.mm
Source/WebCore/platform/mac/ScrollAnimatorMac.h
Source/WebCore/platform/mac/ScrollAnimatorMac.mm
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayer.h
Source/WebCore/rendering/RenderListBox.cpp
Source/WebCore/rendering/RenderListBox.h

index 2dbc56b..07bf1ca 100644 (file)
@@ -1,3 +1,18 @@
+2016-01-02  Simon Fraser  <simon.fraser@apple.com>
+
+        Some cleanup in ScrollAnimator
+        https://bugs.webkit.org/show_bug.cgi?id=152649
+
+        Reviewed by Zalan Bujtas.
+        
+        Added fast/scrolling/arrow-key-scroll-in-rtl-document.html to test for arrow
+        key scrolling in an RTL document, which an earlier version of the patch
+        regressed without detection.
+
+        * fast/dom/horizontal-scrollbar-in-rtl-expected.txt:
+        * fast/scrolling/arrow-key-scroll-in-rtl-document-expected.txt: Added.
+        * fast/scrolling/arrow-key-scroll-in-rtl-document.html: Added.
+
 2016-01-02  Zalan Bujtas  <zalan@apple.com>
 
         Simple line layout:: Add text-decoration support.
index c456d5d..b025557 100644 (file)
@@ -3,6 +3,6 @@ horizontal scroll: : Success
 continuously call window.scrollX : Success
 zoom in and out preserve scroll position: Success
 resize preserves scroll position: Success
-KeyDown HOME move x-scroll position to right for RTL page: -1000
-KeyDown END move x-scroll position to right for RTL page: -1000
+KeyDown HOME move x-scroll position to right for RTL page: 0
+KeyDown END move x-scroll position to right for RTL page: 0
 selectAll selects all document: Success
diff --git a/LayoutTests/fast/scrolling/arrow-key-scroll-in-rtl-document-expected.txt b/LayoutTests/fast/scrolling/arrow-key-scroll-in-rtl-document-expected.txt
new file mode 100644 (file)
index 0000000..6cfd419
--- /dev/null
@@ -0,0 +1 @@
+PASS: scrollLeft is -120
diff --git a/LayoutTests/fast/scrolling/arrow-key-scroll-in-rtl-document.html b/LayoutTests/fast/scrolling/arrow-key-scroll-in-rtl-document.html
new file mode 100644 (file)
index 0000000..3089180
--- /dev/null
@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+
+<html dir="rtl">
+<head>
+    <style>
+        .wide {
+            width: 2000px;
+            height: 10px;
+            background-color: silver;
+        }
+        .origin {
+            position: absolute;
+            top: 0;
+            left: 0;
+            width: 10px;
+            height: 10px;
+            background-color: blue;
+        }
+    </style>
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function checkForScroll()
+        {
+            var expectedScrollLeft = -120;
+            var scrollLeft = document.scrollingElement.scrollLeft;
+            if (scrollLeft != expectedScrollLeft)
+                document.getElementById('result').textContent = "FAIL: scrollLeft is " + scrollLeft +  ", expected " + expectedScrollLeft;
+            else
+                document.getElementById('result').textContent = "PASS: scrollLeft is " + scrollLeft;
+
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+
+        function doTest()
+        {
+            if (window.eventSender) {
+                eventSender.monitorWheelEvents();
+                eventSender.keyDown("leftArrow");
+                eventSender.keyDown("leftArrow");
+                eventSender.keyDown("leftArrow");
+                eventSender.callAfterScrollingCompletes(checkForScroll);
+            }
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    <div class="wide"></div>
+    <div class="origin"></div>
+    <div id="result"></div>
+</body>
+</html>
index f2fbe16..fe75c86 100644 (file)
@@ -1,3 +1,71 @@
+2016-01-02  Simon Fraser  <simon.fraser@apple.com>
+
+        Some cleanup in ScrollAnimator
+        https://bugs.webkit.org/show_bug.cgi?id=152649
+
+        Reviewed by Zalan Bujtas.
+
+        Change ScrollAnimatorMac::adjustScrollPositionIfNecessary() and similar code in
+        ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary() to
+        constrain between minimumScrollPosition() and maximumScrollPosition(), rather than
+        rolling their own code.
+        
+        This revealed several issues. First, RenderLayer::maximumScrollPosition() is
+        wrong when the layer has borders, because RenderLayer::visibleContentRectInternal()
+        seems to have incorrect logic. However, we can just remove it, and use the ScrollableArea
+        implementation.
+        
+        Second, ScrollAnimatorMac::scrollToOffsetWithoutAnimation() was failing to do a
+        position/offset conversion, so do one. We're converting too much, and should probably
+        just change ScrollAnimator to do everything in terms of positions.
+        
+        Third, ScrollAnimator::scroll() was clamping a scroll position as an offset
+        (detected by scrollbars/scroll-rtl-or-bt-layer.html), so fix that.
+        
+        Remove ScrollController::absoluteScrollPosition() and overrides, since this was
+        confusingly named, and could just be removed.
+        
+        Remove ScrollController::m_origOrigin which was assigned to, but never read.
+
+        Test: fast/scrolling/arrow-key-scroll-in-rtl-document.html: new
+              fast/dom/horizontal-scrollbar-in-rtl.html: progressed with these changes.
+
+        * page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:
+        * page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary):
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::absoluteScrollPosition): Deleted.
+        * platform/ScrollAnimator.cpp:
+        (WebCore::ScrollAnimator::scroll):
+        (WebCore::ScrollAnimator::notifyPositionChanged):
+        * platform/ScrollableArea.h:
+        (WebCore::ScrollableArea::constrainScrollPosition):
+        * platform/cocoa/ScrollController.h:
+        * platform/cocoa/ScrollController.mm:
+        (WebCore::ScrollController::snapRubberBandTimerFired): Deleted.
+        (WebCore::ScrollController::snapRubberBand): Deleted.
+        * platform/mac/ScrollAnimatorMac.h:
+        * platform/mac/ScrollAnimatorMac.mm:
+        (-[WebScrollAnimationHelperDelegate _immediateScrollToPoint:]):
+        (WebCore::ScrollAnimatorMac::scroll):
+        (WebCore::ScrollAnimatorMac::scrollToOffsetWithoutAnimation):
+        (WebCore::ScrollAnimatorMac::adjustScrollPositionIfNecessary):
+        (WebCore::ScrollAnimatorMac::adjustScrollPositionToBoundsIfNecessary):
+        (WebCore::ScrollAnimatorMac::immediateScrollToPosition):
+        (WebCore::ScrollAnimatorMac::immediateScrollToPositionForScrollAnimation):
+        (WebCore::ScrollAnimatorMac::immediateScrollTo): Deleted.
+        (WebCore::ScrollAnimatorMac::immediateScrollToPointForScrollAnimation): Deleted.
+        (WebCore::ScrollAnimatorMac::absoluteScrollPosition): Deleted.
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::visibleContentRectInternal):
+        (WebCore::RenderLayer::overhangAmount):
+        (WebCore::RenderLayer::maximumScrollPosition): Deleted.
+        * rendering/RenderLayer.h:
+        * rendering/RenderListBox.cpp:
+        (WebCore::RenderListBox::minimumScrollPosition):
+        (WebCore::RenderListBox::maximumScrollPosition): RenderListBox scrolls by lines,
+        so it needs a custom implementation of this.
+        * rendering/RenderListBox.h:
+
 2016-01-02  Zalan Bujtas  <zalan@apple.com>
 
         Simple line layout:: Add text-decoration support.
index 9b8db4a..67a2f96 100644 (file)
@@ -58,7 +58,6 @@ private:
     bool canScrollHorizontally() override;
     bool canScrollVertically() override;
     bool shouldRubberBandInDirection(ScrollDirection) override;
-    IntPoint absoluteScrollPosition() override;
     void immediateScrollBy(const FloatSize&) override;
     void immediateScrollByWithoutContentEdgeConstraints(const FloatSize&) override;
     void stopSnapRubberbandTimer() override;
index 681f324..5db695e 100644 (file)
@@ -325,11 +325,6 @@ bool ScrollingTreeFrameScrollingNodeMac::shouldRubberBandInDirection(ScrollDirec
     return true;
 }
 
-IntPoint ScrollingTreeFrameScrollingNodeMac::absoluteScrollPosition()
-{
-    return roundedIntPoint(scrollPosition());
-}
-
 void ScrollingTreeFrameScrollingNodeMac::immediateScrollBy(const FloatSize& delta)
 {
     scrollBy(delta);
@@ -350,15 +345,9 @@ void ScrollingTreeFrameScrollingNodeMac::stopSnapRubberbandTimer()
 
 void ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary()
 {
-    FloatPoint currentScrollPosition = absoluteScrollPosition();
-    FloatPoint minPosition = minimumScrollPosition();
-    FloatPoint maxPosition = maximumScrollPosition();
-
-    float nearestXWithinBounds = std::max(std::min(currentScrollPosition.x(), maxPosition.x()), minPosition.x());
-    float nearestYWithinBounds = std::max(std::min(currentScrollPosition.y(), maxPosition.y()), minPosition.y());
-
-    FloatPoint nearestPointWithinBounds(nearestXWithinBounds, nearestYWithinBounds);
-    immediateScrollBy(nearestPointWithinBounds - currentScrollPosition);
+    FloatPoint currentScrollPosition = scrollPosition();
+    FloatPoint constainedPosition = currentScrollPosition.constrainedBetween(minimumScrollPosition(), maximumScrollPosition());
+    immediateScrollBy(constainedPosition - currentScrollPosition);
 }
 
 FloatPoint ScrollingTreeFrameScrollingNodeMac::scrollPosition() const
index f35bef9..085411a 100644 (file)
@@ -63,15 +63,22 @@ ScrollAnimator::~ScrollAnimator()
 
 bool ScrollAnimator::scroll(ScrollbarOrientation orientation, ScrollGranularity, float step, float multiplier)
 {
-    float* currentPos = (orientation == HorizontalScrollbar) ? &m_currentPosX : &m_currentPosY;
-    float newPos = std::max(std::min(*currentPos + (step * multiplier), static_cast<float>(m_scrollableArea.scrollSize(orientation))), 0.0f);
-    float delta = *currentPos - newPos;
-    if (*currentPos == newPos)
+    FloatPoint currentPosition(m_currentPosX, m_currentPosY);
+    FloatSize delta;
+    if (orientation == HorizontalScrollbar)
+        delta.setWidth(step * multiplier);
+    else
+        delta.setHeight(step * multiplier);
+
+    FloatPoint newPosition = FloatPoint(currentPosition + delta).constrainedBetween(m_scrollableArea.minimumScrollPosition(), m_scrollableArea.maximumScrollPosition());
+    if (currentPosition == newPosition)
         return false;
-    *currentPos = newPos;
 
-    notifyPositionChanged(orientation == HorizontalScrollbar ? FloatSize(delta, 0) : FloatSize(0, delta));
+    // FIXME: m_currentPosX/Y should just be a FloatPoint.
+    m_currentPosX = newPosition.x();
+    m_currentPosY = newPosition.y();
 
+    notifyPositionChanged(newPosition - currentPosition);
     return true;
 }
 
@@ -194,7 +201,8 @@ void ScrollAnimator::updateActiveScrollSnapIndexForOffset()
 void ScrollAnimator::notifyPositionChanged(const FloatSize& delta)
 {
     UNUSED_PARAM(delta);
-    m_scrollableArea.setScrollOffsetFromAnimation(IntPoint(m_currentPosX, m_currentPosY));
+    // FIXME: need to not map back and forth all the time.
+    m_scrollableArea.setScrollOffsetFromAnimation(m_scrollableArea.scrollOffsetFromPosition(IntPoint(m_currentPosX, m_currentPosY)));
 }
 
 #if ENABLE(CSS_SCROLL_SNAP)
index 02bffbd..4295d84 100644 (file)
@@ -188,6 +188,11 @@ public:
     virtual ScrollPosition minimumScrollPosition() const;
     virtual ScrollPosition maximumScrollPosition() const;
 
+    ScrollPosition constrainScrollPosition(const ScrollPosition& position) const
+    {
+        return position.constrainedBetween(minimumScrollPosition(), maximumScrollPosition());
+    }
+
     ScrollOffset maximumScrollOffset() const;
 
     WEBCORE_EXPORT ScrollPosition scrollPositionFromOffset(ScrollOffset) const;
index 432992a..1b64812 100644 (file)
@@ -60,9 +60,6 @@ public:
     virtual bool canScrollVertically() = 0;
     virtual bool shouldRubberBandInDirection(ScrollDirection) = 0;
 
-    // Return the absolute scroll position, not relative to the scroll origin.
-    virtual WebCore::IntPoint absoluteScrollPosition() = 0;
-
     virtual void immediateScrollBy(const FloatSize&) = 0;
     virtual void immediateScrollByWithoutContentEdgeConstraints(const FloatSize&) = 0;
     virtual void startSnapRubberbandTimer()
@@ -179,7 +176,6 @@ private:
     // Rubber band state.
     CFTimeInterval m_startTime { 0 };
     FloatSize m_startStretch;
-    FloatPoint m_origOrigin;
     FloatSize m_origVelocity;
     RunLoop::Timer<ScrollController> m_snapRubberbandTimer;
 #endif
index 885201a..cd6fdbe 100644 (file)
@@ -349,12 +349,10 @@ void ScrollController::snapRubberBandTimerFired()
                 m_stretchScrollForce = FloatSize();
                 m_startTime = 0;
                 m_startStretch = FloatSize();
-                m_origOrigin = FloatPoint();
                 m_origVelocity = FloatSize();
                 return;
             }
 
-            m_origOrigin = m_client.absoluteScrollPosition() - m_startStretch;
             m_origVelocity = m_momentumVelocity;
 
             // Just like normal scrolling, prefer vertical rubberbanding
@@ -387,7 +385,6 @@ void ScrollController::snapRubberBandTimerFired()
             m_stretchScrollForce = FloatSize();
             m_startTime = 0;
             m_startStretch = FloatSize();
-            m_origOrigin = FloatPoint();
             m_origVelocity = FloatSize();
         }
     } else {
@@ -451,7 +448,6 @@ void ScrollController::snapRubberBand()
 
     m_startTime = [NSDate timeIntervalSinceReferenceDate];
     m_startStretch = FloatSize();
-    m_origOrigin = FloatPoint();
     m_origVelocity = FloatSize();
 
     startSnapRubberbandTimer();
index b50d259..5e3eac8 100644 (file)
@@ -52,7 +52,7 @@ public:
     ScrollAnimatorMac(ScrollableArea&);
     virtual ~ScrollAnimatorMac();
 
-    void immediateScrollToPointForScrollAnimation(const FloatPoint& newPosition);
+    void immediateScrollToPositionForScrollAnimation(const FloatPoint& newPosition);
     bool haveScrolledSincePageLoad() const { return m_haveScrolledSincePageLoad; }
 
     void updateScrollerStyle();
@@ -134,7 +134,7 @@ private:
 
     FloatPoint adjustScrollPositionIfNecessary(const FloatPoint&) const;
 
-    void immediateScrollTo(const FloatPoint&);
+    void immediateScrollToPosition(const FloatPoint&);
 
     bool isRubberBandInProgress() const override;
     bool isScrollSnapInProgress() const override;
@@ -148,7 +148,6 @@ private:
     virtual bool canScrollHorizontally() override;
     virtual bool canScrollVertically() override;
     virtual bool shouldRubberBandInDirection(ScrollDirection) override;
-    virtual WebCore::IntPoint absoluteScrollPosition() override;
     virtual void immediateScrollByWithoutContentEdgeConstraints(const FloatSize&) override;
     virtual void immediateScrollBy(const FloatSize&) override;
     virtual void adjustScrollPositionToBoundsIfNecessary() override;
index 4b359ad..0354774 100644 (file)
 #include "BlockExceptions.h"
 #include "FloatPoint.h"
 #include "GraphicsLayer.h"
+#include "Logging.h"
 #include "NSScrollerImpDetails.h"
 #include "PlatformWheelEvent.h"
 #include "ScrollView.h"
 #include "ScrollableArea.h"
 #include "ScrollbarTheme.h"
 #include "ScrollbarThemeMac.h"
+#include "TextStream.h"
 #include "WebCoreSystemInterface.h"
 
 using namespace WebCore;
@@ -130,7 +132,7 @@ static NSSize abs(NSSize size)
 {
     if (!_animator)
         return;
-    _animator->immediateScrollToPointForScrollAnimation(newPosition);
+    _animator->immediateScrollToPositionForScrollAnimation(newPosition);
 }
 
 - (NSPoint)_pixelAlignProposedScrollPosition:(NSPoint)newOrigin
@@ -700,26 +702,35 @@ bool ScrollAnimatorMac::scroll(ScrollbarOrientation orientation, ScrollGranulari
     if (granularity == ScrollByPixel)
         return ScrollAnimator::scroll(orientation, granularity, step, multiplier);
 
-    float currentPos = orientation == HorizontalScrollbar ? m_currentPosX : m_currentPosY;
-    float newPos = std::max<float>(std::min<float>(currentPos + (step * multiplier), static_cast<float>(m_scrollableArea.scrollSize(orientation))), 0);
-    if (currentPos == newPos)
+    FloatPoint currentPosition(m_currentPosX, m_currentPosY);
+    FloatSize delta;
+    if (orientation == HorizontalScrollbar)
+        delta.setWidth(step * multiplier);
+    else
+        delta.setHeight(step * multiplier);
+
+    FloatPoint newPosition = FloatPoint(currentPosition + delta).constrainedBetween(m_scrollableArea.minimumScrollPosition(), m_scrollableArea.maximumScrollPosition());
+    if (currentPosition == newPosition)
         return false;
 
-    NSPoint newPoint;
     if ([m_scrollAnimationHelper _isAnimating]) {
         NSPoint targetOrigin = [m_scrollAnimationHelper targetOrigin];
-        newPoint = orientation == HorizontalScrollbar ? NSMakePoint(newPos, targetOrigin.y) : NSMakePoint(targetOrigin.x, newPos);
-    } else
-        newPoint = orientation == HorizontalScrollbar ? NSMakePoint(newPos, m_currentPosY) : NSMakePoint(m_currentPosX, newPos);
+        if (orientation == HorizontalScrollbar)
+            newPosition.setY(targetOrigin.y);
+        else
+            newPosition.setX(targetOrigin.x);
+    }
 
-    [m_scrollAnimationHelper scrollToPoint:newPoint];
+    LOG_WITH_STREAM(Scrolling, stream << "ScrollAnimatorMac::scroll " << " from " << FloatPoint(m_currentPosX, m_currentPosY) << " to " << newPosition);
+    [m_scrollAnimationHelper scrollToPoint:newPosition];
     return true;
 }
 
+// FIXME: Maybe this should take a position.
 void ScrollAnimatorMac::scrollToOffsetWithoutAnimation(const FloatPoint& offset)
 {
     [m_scrollAnimationHelper _stopRun];
-    immediateScrollTo(offset);
+    immediateScrollToPosition(ScrollableArea::scrollPositionFromOffset(offset, toFloatSize(m_scrollableArea.scrollOrigin())));
 }
 
 FloatPoint ScrollAnimatorMac::adjustScrollPositionIfNecessary(const FloatPoint& position) const
@@ -727,10 +738,7 @@ FloatPoint ScrollAnimatorMac::adjustScrollPositionIfNecessary(const FloatPoint&
     if (!m_scrollableArea.constrainsScrollingToContentEdge())
         return position;
 
-    float newX = std::max<float>(std::min<float>(position.x(), m_scrollableArea.totalContentsSize().width() - m_scrollableArea.visibleWidth()), 0);
-    float newY = std::max<float>(std::min<float>(position.y(), m_scrollableArea.totalContentsSize().height() - m_scrollableArea.visibleHeight()), 0);
-
-    return FloatPoint(newX, newY);
+    return m_scrollableArea.constrainScrollPosition(ScrollPosition(position));
 }
 
 void ScrollAnimatorMac::adjustScrollPositionToBoundsIfNecessary()
@@ -738,14 +746,14 @@ void ScrollAnimatorMac::adjustScrollPositionToBoundsIfNecessary()
     bool currentlyConstrainsToContentEdge = m_scrollableArea.constrainsScrollingToContentEdge();
     m_scrollableArea.setConstrainsScrollingToContentEdge(true);
 
-    IntPoint currentScrollPosition = absoluteScrollPosition();
-    FloatPoint nearestPointWithinBounds = adjustScrollPositionIfNecessary(absoluteScrollPosition());
-    immediateScrollBy(nearestPointWithinBounds - currentScrollPosition);
+    ScrollPosition currentScrollPosition = m_scrollableArea.scrollPosition();
+    ScrollPosition constainedPosition = m_scrollableArea.constrainScrollPosition(currentScrollPosition);
+    immediateScrollBy(constainedPosition - currentScrollPosition);
 
     m_scrollableArea.setConstrainsScrollingToContentEdge(currentlyConstrainsToContentEdge);
 }
 
-void ScrollAnimatorMac::immediateScrollTo(const FloatPoint& newPosition)
+void ScrollAnimatorMac::immediateScrollToPosition(const FloatPoint& newPosition)
 {
     FloatPoint adjustedPosition = adjustScrollPositionIfNecessary(newPosition);
  
@@ -779,10 +787,10 @@ bool ScrollAnimatorMac::isScrollSnapInProgress() const
 #endif
 }
 
-void ScrollAnimatorMac::immediateScrollToPointForScrollAnimation(const FloatPoint& newPosition)
+void ScrollAnimatorMac::immediateScrollToPositionForScrollAnimation(const FloatPoint& newPosition)
 {
     ASSERT(m_scrollAnimationHelper);
-    immediateScrollTo(newPosition);
+    immediateScrollToPosition(newPosition);
 }
 
 void ScrollAnimatorMac::notifyPositionChanged(const FloatSize& delta)
@@ -1282,12 +1290,6 @@ bool ScrollAnimatorMac::shouldRubberBandInDirection(ScrollDirection)
     return false;
 }
 
-IntPoint ScrollAnimatorMac::absoluteScrollPosition()
-{
-    // FIXME: can this use m_scrollableArea.scrollPosition()?
-    return m_scrollableArea.scrollOffsetFromPosition(m_scrollableArea.visibleContentRect().location());
-}
-
 void ScrollAnimatorMac::immediateScrollByWithoutContentEdgeConstraints(const FloatSize& delta)
 {
     m_scrollableArea.setConstrainsScrollingToContentEdge(false);
index 7782939..a049678 100644 (file)
@@ -2732,19 +2732,13 @@ int RenderLayer::scrollPosition(Scrollbar* scrollbar) const
     return 0;
 }
 
-ScrollPosition RenderLayer::maximumScrollPosition() const
-{
-    // FIXME: m_scrollSize may not be up-to-date if m_scrollDimensionsDirty is true.
-    // FIXME: should be able to use the ScrollableArea implementation.
-    return scrollPositionFromOffset(roundedIntPoint(m_scrollSize) - visibleContentRectIncludingScrollbars(ContentsVisibleRect).size());
-}
-
 IntRect RenderLayer::visibleContentRectInternal(VisibleContentRectIncludesScrollbars scrollbarInclusion, VisibleContentRectBehavior) const
 {
     IntSize scrollbarSpace;
     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())));
 }
 
@@ -2756,7 +2750,7 @@ IntSize RenderLayer::overhangAmount() const
 
     IntSize stretch;
 
-    // FIXME: use maximumScrollOffset()
+    // FIXME: use maximumScrollOffset(), or just move this to ScrollableArea.
     ScrollOffset scrollOffset = scrollOffsetFromPosition(scrollPosition());
     if (scrollOffset.y() < 0)
         stretch.setHeight(scrollOffset.y());
index cc45692..e1cba39 100644 (file)
@@ -866,7 +866,6 @@ private:
     virtual void setScrollOffset(const ScrollOffset&) override;
 
     virtual ScrollPosition scrollPosition() const override { return m_scrollPosition; }
-    virtual ScrollPosition maximumScrollPosition() const override;
 
     virtual IntRect visibleContentRectInternal(VisibleContentRectIncludesScrollbars, VisibleContentRectBehavior) const override;
     virtual IntSize visibleSize() const override;
index f459f93..764539b 100644 (file)
@@ -608,6 +608,16 @@ int RenderListBox::scrollPosition(Scrollbar*) const
     return m_indexOffset;
 }
 
+ScrollPosition RenderListBox::minimumScrollPosition() const
+{
+    return { 0, 0 };
+}
+
+ScrollPosition RenderListBox::maximumScrollPosition() const
+{
+    return { 0, numItems() - numVisibleItems() };
+}
+
 void RenderListBox::setScrollOffset(const ScrollOffset& offset)
 {
     scrollTo(offset.y());
index cce33be..2f8697a 100644 (file)
@@ -112,6 +112,8 @@ private:
     virtual int scrollSize(ScrollbarOrientation) const override;
     virtual int scrollPosition(Scrollbar*) const override;
     virtual void setScrollOffset(const ScrollOffset&) override;
+    virtual ScrollPosition minimumScrollPosition() const override;
+    virtual ScrollPosition maximumScrollPosition() const override;
     virtual void invalidateScrollbarRect(Scrollbar*, const IntRect&) override;
     virtual bool isActive() const override;
     virtual bool isScrollCornerVisible() const override { return false; } // We don't support resize on list boxes yet. If we did these would have to change.