iOS: Autoscrolling is too fast and way too aggressive
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Feb 2020 23:23:04 +0000 (23:23 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Feb 2020 23:23:04 +0000 (23:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=207717
<rdar://problem/59208122>

Reviewed by Simon Fraser.

* page/EventHandler.h:
* page/ios/EventHandlerIOS.mm:
(WebCore::EventHandler::startSelectionAutoscroll):
(WebCore::EventHandler::cancelSelectionAutoscroll):
(WebCore::adjustAutoscrollDestinationForInsetEdges):
(WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll const):
(WebCore::autoscrollAdjustmentFactorForScreenBoundaries): Deleted.
Make a few small changes to autoscrolling on iOS to make it feel better:

- Store the autoscrolling position in "unscrolled" coordinates, and do
all work in this space, converting back when it's time to actually scroll.
This fixes the problem where you have to wiggle your finger to autoscroll,
because now when the timer fires, the point actually moves (before, it was
all stored in "content" coordinates, so wouldn't actually change until
the client pushed a new point).

- Reintroduce the macOS-style linear scaling of scrolling velocity
in (and beyond) the inset region. We scale the fractional distance into
the inset region to a 20pt/50ms scroll velocity; when you exit the inset
it continues scaling up linearly from there.

- Only apply insets in the direction that the autoscroll drag is occurring
in. This avoids a problem where e.g. horizontally selecting text on the
first visible line of a page would cause us to scroll up, as it sat
within the top inset. Instead, we only apply an inset in the direction of
the drag, and do not allow its magnitude to exceed the currently dragged
distance.

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

Source/WebCore/ChangeLog
Source/WebCore/page/EventHandler.h
Source/WebCore/page/ios/EventHandlerIOS.mm

index 3086466..8a8e27f 100644 (file)
@@ -1,3 +1,39 @@
+2020-02-13  Tim Horton  <timothy_horton@apple.com>
+
+        iOS: Autoscrolling is too fast and way too aggressive
+        https://bugs.webkit.org/show_bug.cgi?id=207717
+        <rdar://problem/59208122>
+
+        Reviewed by Simon Fraser.
+
+        * page/EventHandler.h:
+        * page/ios/EventHandlerIOS.mm:
+        (WebCore::EventHandler::startSelectionAutoscroll):
+        (WebCore::EventHandler::cancelSelectionAutoscroll):
+        (WebCore::adjustAutoscrollDestinationForInsetEdges):
+        (WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll const):
+        (WebCore::autoscrollAdjustmentFactorForScreenBoundaries): Deleted.
+        Make a few small changes to autoscrolling on iOS to make it feel better:
+
+        - Store the autoscrolling position in "unscrolled" coordinates, and do
+        all work in this space, converting back when it's time to actually scroll.
+        This fixes the problem where you have to wiggle your finger to autoscroll,
+        because now when the timer fires, the point actually moves (before, it was
+        all stored in "content" coordinates, so wouldn't actually change until
+        the client pushed a new point).
+
+        - Reintroduce the macOS-style linear scaling of scrolling velocity
+        in (and beyond) the inset region. We scale the fractional distance into
+        the inset region to a 20pt/50ms scroll velocity; when you exit the inset
+        it continues scaling up linearly from there.
+
+        - Only apply insets in the direction that the autoscroll drag is occurring
+        in. This avoids a problem where e.g. horizontally selecting text on the
+        first visible line of a page would cause us to scroll up, as it sat
+        within the top inset. Instead, we only apply an inset in the direction of
+        the drag, and do not allow its magnitude to exceed the currently dragged
+        distance.
+
 2020-02-13  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r254557.
index ea2472d..b7c8120 100644 (file)
@@ -626,8 +626,9 @@ private:
 
 #if PLATFORM(IOS_FAMILY)
     bool m_shouldAllowMouseDownToStartDrag { false };
-    IntPoint m_targetAutoscrollPositionInWindow;
     bool m_isAutoscrolling { false };
+    IntPoint m_targetAutoscrollPositionInUnscrolledRootViewCoordinates;
+    Optional<IntPoint> m_initialTargetAutoscrollPositionInUnscrolledRootViewCoordinates;
 #endif
 
 #if ENABLE(CURSOR_VISIBILITY)
index 6ed72cc..4f17921 100644 (file)
@@ -585,8 +585,11 @@ void EventHandler::startSelectionAutoscroll(RenderObject* renderer, const FloatP
 {
     Ref<Frame> protectedFrame(m_frame);
 
-    m_targetAutoscrollPositionInWindow = protectedFrame->view()->contentsToView(roundedIntPoint(positionInWindow));
-    
+    m_targetAutoscrollPositionInUnscrolledRootViewCoordinates = protectedFrame->view()->contentsToRootView(roundedIntPoint(positionInWindow)) - toIntSize(protectedFrame->view()->documentScrollPositionRelativeToViewOrigin());
+
+    if (!m_isAutoscrolling)
+        m_initialTargetAutoscrollPositionInUnscrolledRootViewCoordinates = m_targetAutoscrollPositionInUnscrolledRootViewCoordinates;
+
     m_isAutoscrolling = true;
     m_autoscrollController->startAutoscrollForSelection(renderer);
 }
@@ -594,65 +597,83 @@ void EventHandler::startSelectionAutoscroll(RenderObject* renderer, const FloatP
 void EventHandler::cancelSelectionAutoscroll()
 {
     m_isAutoscrolling = false;
+    m_initialTargetAutoscrollPositionInUnscrolledRootViewCoordinates = WTF::nullopt;
     m_autoscrollController->stopAutoscrollTimer();
 }
 
-static IntSize autoscrollAdjustmentFactorForScreenBoundaries(const IntPoint& contentPosition, const FloatRect& unobscuredContentRect, float zoomFactor)
+static IntPoint adjustAutoscrollDestinationForInsetEdges(IntPoint autoscrollPoint, Optional<IntPoint> initialAutoscrollPoint, FloatRect unobscuredRootViewRect)
 {
-    // If the window is at the edge of the screen, and the touch position is also at that edge of the screen,
-    // we need to adjust the autoscroll amount in order for the user to be able to autoscroll in that direction.
-    // We can pretend that the touch position is slightly beyond the edge of the screen, and then autoscrolling
-    // will occur as expected. This function figures out just how much to adjust the autoscroll amount by
-    // in order to get autoscrolling to feel natural in this situation.
-    
-    IntSize adjustmentFactor;
-    
-#define EDGE_DISTANCE_THRESHOLD 100
+    IntPoint resultPoint = autoscrollPoint;
 
-    CGSize edgeDistanceThreshold = CGSizeMake(EDGE_DISTANCE_THRESHOLD / zoomFactor, EDGE_DISTANCE_THRESHOLD / zoomFactor);
-    
-    float screenLeftEdge = unobscuredContentRect.x();
-    float insetScreenLeftEdge = screenLeftEdge + edgeDistanceThreshold.width;
-    float screenRightEdge = unobscuredContentRect.maxX();
-    float insetScreenRightEdge = screenRightEdge - edgeDistanceThreshold.width;
-    if (contentPosition.x() >= screenLeftEdge && contentPosition.x() < insetScreenLeftEdge) {
-        float distanceFromEdge = contentPosition.x() - screenLeftEdge - edgeDistanceThreshold.width;
+    const float edgeInset = 75;
+    const float maximumScrollingSpeed = 20;
+    const float insetDistanceThreshold = edgeInset / 2;
+
+    // FIXME: Ideally we would only inset on edges that touch the edge of the screen,
+    // like macOS, but we don't have enough information in WebCore to do that currently.
+    FloatRect insetUnobscuredRootViewRect = unobscuredRootViewRect;
+
+    if (initialAutoscrollPoint) {
+        IntSize autoscrollDelta = autoscrollPoint - *initialAutoscrollPoint;
+
+        // Inset edges in the direction of the autoscroll.
+        // Do not apply insets until you drag in the direction of the edge at least `insetDistanceThreshold`,
+        // to make it possible to select text that abuts the edge of `unobscuredRootViewRect` without causing
+        // unwanted autoscrolling.
+        if (autoscrollDelta.width() < insetDistanceThreshold)
+            insetUnobscuredRootViewRect.shiftXEdgeTo(insetUnobscuredRootViewRect.x() + std::min<float>(edgeInset, -autoscrollDelta.width() - insetDistanceThreshold));
+        else if (autoscrollDelta.width() > insetDistanceThreshold)
+            insetUnobscuredRootViewRect.shiftMaxXEdgeTo(insetUnobscuredRootViewRect.maxX() - std::min<float>(edgeInset, autoscrollDelta.width() - insetDistanceThreshold));
+
+        if (autoscrollDelta.height() < insetDistanceThreshold)
+            insetUnobscuredRootViewRect.shiftYEdgeTo(insetUnobscuredRootViewRect.y() + std::min<float>(edgeInset, -autoscrollDelta.height() - insetDistanceThreshold));
+        else if (autoscrollDelta.height() > insetDistanceThreshold)
+            insetUnobscuredRootViewRect.shiftMaxYEdgeTo(insetUnobscuredRootViewRect.maxY() - std::min<float>(edgeInset, autoscrollDelta.height() - insetDistanceThreshold));
+    }
+
+    // If the current autoscroll point is beyond the edge of the view (respecting insets), shift it outside
+    // of the view, so that autoscrolling will occur. The distance we move outside of the view is scaled from
+    // `edgeInset` to `maximumScrollingSpeed` so that the inset's contribution to the speed is independent of its size.
+    if (autoscrollPoint.x() < insetUnobscuredRootViewRect.x()) {
+        float distanceFromEdge = autoscrollPoint.x() - insetUnobscuredRootViewRect.x();
         if (distanceFromEdge < 0)
-            adjustmentFactor.setWidth(-edgeDistanceThreshold.width);
-    } else if (contentPosition.x() >= insetScreenRightEdge && contentPosition.x() < screenRightEdge) {
-        float distanceFromEdge = edgeDistanceThreshold.width - (screenRightEdge - contentPosition.x());
+            resultPoint.setX(unobscuredRootViewRect.x() + ((distanceFromEdge / edgeInset) * maximumScrollingSpeed));
+    } else if (autoscrollPoint.x() >= insetUnobscuredRootViewRect.maxX()) {
+        float distanceFromEdge = autoscrollPoint.x() - insetUnobscuredRootViewRect.maxX();
         if (distanceFromEdge > 0)
-            adjustmentFactor.setWidth(edgeDistanceThreshold.width);
+            resultPoint.setX(unobscuredRootViewRect.maxX() + ((distanceFromEdge / edgeInset) * maximumScrollingSpeed));
     }
-    
-    float screenTopEdge = unobscuredContentRect.y();
-    float insetScreenTopEdge = screenTopEdge + edgeDistanceThreshold.height;
-    float screenBottomEdge = unobscuredContentRect.maxY();
-    float insetScreenBottomEdge = screenBottomEdge - edgeDistanceThreshold.height;
-    
-    if (contentPosition.y() >= screenTopEdge && contentPosition.y() < insetScreenTopEdge) {
-        float distanceFromEdge = contentPosition.y() - screenTopEdge - edgeDistanceThreshold.height;
+
+    if (autoscrollPoint.y() < insetUnobscuredRootViewRect.y()) {
+        float distanceFromEdge = autoscrollPoint.y() - insetUnobscuredRootViewRect.y();
         if (distanceFromEdge < 0)
-            adjustmentFactor.setHeight(-edgeDistanceThreshold.height);
-    } else if (contentPosition.y() >= insetScreenBottomEdge && contentPosition.y() < screenBottomEdge) {
-        float distanceFromEdge = edgeDistanceThreshold.height - (screenBottomEdge - contentPosition.y());
+            resultPoint.setY(unobscuredRootViewRect.y() + ((distanceFromEdge / edgeInset) * maximumScrollingSpeed));
+    } else if (autoscrollPoint.y() >= insetUnobscuredRootViewRect.maxY()) {
+        float distanceFromEdge = autoscrollPoint.y() - insetUnobscuredRootViewRect.maxY();
         if (distanceFromEdge > 0)
-            adjustmentFactor.setHeight(edgeDistanceThreshold.height);
+            resultPoint.setY(unobscuredRootViewRect.maxY() + ((distanceFromEdge / edgeInset) * maximumScrollingSpeed));
     }
-    
-    return adjustmentFactor;
+
+    return resultPoint;
 }
     
 IntPoint EventHandler::targetPositionInWindowForSelectionAutoscroll() const
 {
     Ref<Frame> protectedFrame(m_frame);
+
+    if (!m_frame.view())
+        return { };
+    auto& frameView = *m_frame.view();
+
+    // All work is done in "unscrolled" root view coordinates (as if delegatesScrolling were off),
+    // so that when the autoscrolling timer fires, it uses the new scroll position, so that it
+    // can keep scrolling without the client pushing a new contents-space target position via startSelectionAutoscroll.
+    auto scrollPosition = toIntSize(frameView.documentScrollPositionRelativeToViewOrigin());
     
-    FloatRect unobscuredContentRect = protectedFrame->view()->unobscuredContentRect();
-    
-    // Manually need to convert viewToContents, as it will be skipped because delegatedScrolling is on iOS
-    IntPoint contentPosition = protectedFrame->view()->viewToContents(protectedFrame->view()->convertFromContainingWindow(m_targetAutoscrollPositionInWindow));
-    IntSize adjustPosition = autoscrollAdjustmentFactorForScreenBoundaries(contentPosition, unobscuredContentRect, protectedFrame->page()->pageScaleFactor());
-    return contentPosition + adjustPosition;
+    FloatRect unobscuredContentRectInUnscrolledRootViewCoordinates = frameView.contentsToRootView(frameView.unobscuredContentRect());
+    unobscuredContentRectInUnscrolledRootViewCoordinates.move(-scrollPosition);
+
+    return adjustAutoscrollDestinationForInsetEdges(m_targetAutoscrollPositionInUnscrolledRootViewCoordinates, m_initialTargetAutoscrollPositionInUnscrolledRootViewCoordinates, unobscuredContentRectInUnscrolledRootViewCoordinates) + scrollPosition;
 }
     
 bool EventHandler::shouldUpdateAutoscroll()