[iOS] Focusing a large editable element always scrolls to the top of the element
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Dec 2018 04:04:44 +0000 (04:04 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Dec 2018 04:04:44 +0000 (04:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192745
<rdar://problem/46758445>

Reviewed by Tim Horton.

Source/WebKit:

Currently, when focusing form controls or editable elements, we try to scroll such that the focused element rect
is centered within the visible area. In the case of very large focusable elements whose dimensions exceed the
width or height of the visible area, we instead scroll such that the top left point of the element is at the top
left corner of the visible area.

However, this results in unnecessary scrolling if the top of the element is already near the top of the visible
area. For WebKit2-based rich text editors that have an editable body element with a top content inset that
contains additional content, this means we will always scroll the additional content away when focusing the
editable body.

To avoid this behavior, adjust focused element zooming logic for editable elements that are too large to be
centered in the visible area, such that we only scroll the top left position of the focused element to the top
half or top right of the visible area, respectively. This reduces the amount of scrolling when focusing large
editable elements, while still making it clear which element is being focused.

* Platform/spi/ios/UIKitSPI.h:
* UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _zoomToFocusRect:selectionRect:insideFixed:fontSize:minimumScale:maximumScale:allowScaling:forceScroll:]):

Make some small adjustments to improve the readability of this method by using `clampTo` instead of clamping
values by comparing and setting values.

Also, fix an existing bug wherein focusable elements that are meant to be centered within the visible area are
currently offset by half the difference between the bottom inset amount and the top inset amount, in the case
where the `_obscuredInsets` SPI is used to specify content insets for the web view (i.e., MobileSafari).

* UIProcess/API/Cocoa/WKWebViewInternal.h:

Make a couple of arguments `const FloatRect&` instead of just `FloatRect`.

LayoutTests:

Add a new layout test to verify that we don't scroll unnecessarily when focusing a tall editable element, whose
top offset is already near the top of the viewport.

* editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt: Added.
* editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html [new file with mode: 0644]
Source/WebKit/ChangeLog
Source/WebKit/Platform/spi/ios/UIKitSPI.h
Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h

index 9a0a1e8..ad6427e 100644 (file)
@@ -1,3 +1,17 @@
+2018-12-17  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [iOS] Focusing a large editable element always scrolls to the top of the element
+        https://bugs.webkit.org/show_bug.cgi?id=192745
+        <rdar://problem/46758445>
+
+        Reviewed by Tim Horton.
+
+        Add a new layout test to verify that we don't scroll unnecessarily when focusing a tall editable element, whose
+        top offset is already near the top of the viewport.
+
+        * editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt: Added.
+        * editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html: Added.
+
 2018-12-17  Ryosuke Niwa  <rniwa@webkit.org>
 
         offsetLeft and offsetParent should adjust across shadow boundaries
diff --git a/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt b/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt
new file mode 100644 (file)
index 0000000..236c5ff
--- /dev/null
@@ -0,0 +1,6 @@
+    Here's to the crazy ones. The misfits. The rebels. The troublemakers. The round pegs in the square holes. The ones who see things differently. They're not fond of rules. And they have no respect for the status quo. You can quote them, disagree with them, glorify or vilify them. About the only thing you can't do is ignore them. Because they change things. They push the human race forward. And while some may see them as the crazy ones, we see genius. Because the people who are crazy enough to think they can change the world, are the ones who do.
+    
+This test verifies that we avoid unnecessary scrolling when focusing large editable areas. To reproduce manually, scroll to the top and focus the first line of editable text in the dashed box. The blue box should still be visible.
+
+The initial scroll offset is: 0,0
+The final scroll offset is: 0,0
diff --git a/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html b/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html
new file mode 100644 (file)
index 0000000..48861e1
--- /dev/null
@@ -0,0 +1,40 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+    <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1">
+    <script src="../../../resources/ui-helper.js"></script>
+</head>
+<body style="margin: 0">
+    <div style="width: 100%; height: 50px; background-color: lightblue; opacity: 0.25"></div>
+    <pre id="editor" contenteditable style="line-height: 1.5em; border: silver dashed 2px; height: 100vh; margin-top: 0">
+    Here's to the crazy ones. The misfits. The rebels. The troublemakers. The round pegs in the square holes. The ones who see things differently. They're not fond of rules. And they have no respect for the status quo. You can quote them, disagree with them, glorify or vilify them. About the only thing you can't do is ignore them. Because they change things. They push the human race forward. And while some may see them as the crazy ones, we see genius. Because the people who are crazy enough to think they can change the world, are the ones who do.
+    </pre>
+    <p id="description">
+    This test verifies that we avoid unnecessary scrolling when focusing large editable areas. To reproduce manually, scroll to the top and focus the first line of editable text in the dashed box. The blue box should still be visible.
+    </p>
+    <div>
+        <pre>The initial scroll offset is: <span id="initial"></span></pre>
+        <pre>The final scroll offset is: <span id="final"></span></pre>
+    </div>
+</body>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+addEventListener("load", async () => {
+    if (!window.testRunner)
+        return;
+
+    initial.textContent = [pageXOffset, pageYOffset].toString();
+
+    await UIHelper.activateAndWaitForInputSessionAt(100, 60);
+    editor.blur();
+    await UIHelper.waitForKeyboardToHide();
+
+    final.textContent = [pageXOffset, pageYOffset].toString();
+    testRunner.notifyDone();
+});
+</script>
+</html>
\ No newline at end of file
index 5e1cf3b..dbb9e51 100644 (file)
@@ -1,3 +1,41 @@
+2018-12-17  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [iOS] Focusing a large editable element always scrolls to the top of the element
+        https://bugs.webkit.org/show_bug.cgi?id=192745
+        <rdar://problem/46758445>
+
+        Reviewed by Tim Horton.
+
+        Currently, when focusing form controls or editable elements, we try to scroll such that the focused element rect
+        is centered within the visible area. In the case of very large focusable elements whose dimensions exceed the
+        width or height of the visible area, we instead scroll such that the top left point of the element is at the top
+        left corner of the visible area.
+
+        However, this results in unnecessary scrolling if the top of the element is already near the top of the visible
+        area. For WebKit2-based rich text editors that have an editable body element with a top content inset that
+        contains additional content, this means we will always scroll the additional content away when focusing the
+        editable body.
+
+        To avoid this behavior, adjust focused element zooming logic for editable elements that are too large to be
+        centered in the visible area, such that we only scroll the top left position of the focused element to the top
+        half or top right of the visible area, respectively. This reduces the amount of scrolling when focusing large
+        editable elements, while still making it clear which element is being focused.
+
+        * Platform/spi/ios/UIKitSPI.h:
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView _zoomToFocusRect:selectionRect:insideFixed:fontSize:minimumScale:maximumScale:allowScaling:forceScroll:]):
+
+        Make some small adjustments to improve the readability of this method by using `clampTo` instead of clamping
+        values by comparing and setting values.
+
+        Also, fix an existing bug wherein focusable elements that are meant to be centered within the visible area are
+        currently offset by half the difference between the bottom inset amount and the top inset amount, in the case
+        where the `_obscuredInsets` SPI is used to specify content insets for the web view (i.e., MobileSafari).
+
+        * UIProcess/API/Cocoa/WKWebViewInternal.h:
+
+        Make a couple of arguments `const FloatRect&` instead of just `FloatRect`.
+
 2018-12-17  Ryosuke Niwa  <rniwa@webkit.org>
 
         offsetLeft and offsetParent should adjust across shadow boundaries
index 741b8ba..1f63a32 100644 (file)
@@ -348,6 +348,7 @@ typedef NS_ENUM(NSInteger, UIScrollViewIndicatorInsetAdjustmentBehavior) {
 @property (nonatomic, getter=_contentScrollInset, setter=_setContentScrollInset:) UIEdgeInsets contentScrollInset;
 @property (nonatomic, getter=_indicatorInsetAdjustmentBehavior, setter=_setIndicatorInsetAdjustmentBehavior:) UIScrollViewIndicatorInsetAdjustmentBehavior indicatorInsetAdjustmentBehavior;
 @property (nonatomic, readonly) UIEdgeInsets _systemContentInset;
+@property (nonatomic, readonly) UIEdgeInsets _effectiveContentInset;
 @end
 
 @interface NSString (UIKitDetails)
index 8dc30f1..b9bed1d 100644 (file)
@@ -2225,7 +2225,7 @@ static WebCore::FloatPoint constrainContentOffset(WebCore::FloatPoint contentOff
 }
 
 // focusedElementRect and selectionRect are both in document coordinates.
-- (void)_zoomToFocusRect:(WebCore::FloatRect)focusedElementRectInDocumentCoordinates selectionRect:(WebCore::FloatRect)selectionRectInDocumentCoordinates insideFixed:(BOOL)insideFixed
+- (void)_zoomToFocusRect:(const WebCore::FloatRect&)focusedElementRectInDocumentCoordinates selectionRect:(const WebCore::FloatRect&)selectionRectInDocumentCoordinates insideFixed:(BOOL)insideFixed
     fontSize:(float)fontSize minimumScale:(double)minimumScale maximumScale:(double)maximumScale allowScaling:(BOOL)allowScaling forceScroll:(BOOL)forceScroll
 {
     LOG_WITH_STREAM(VisibleRects, stream << "_zoomToFocusRect:" << focusedElementRectInDocumentCoordinates << " selectionRect:" << selectionRectInDocumentCoordinates);
@@ -2305,53 +2305,85 @@ static WebCore::FloatPoint constrainContentOffset(WebCore::FloatPoint contentOff
             return;
     }
 
-    // We want to zoom to the left/top corner of the DOM node, with as much spacing on all sides as we
-    // can get based on the visible area after zooming (workingFrame).  The spacing in either dimension is half the
+    // We want to center the focused element within the viewport, with as much spacing on all sides as
+    // we can get based on the visible area after zooming. The spacing in either dimension is half the
     // difference between the size of the DOM node and the size of the visible frame.
-    CGFloat horizontalSpaceInWebViewCoordinates = std::max((visibleSize.width - focusedElementRectInNewScale.width()) / 2.0, 0.0);
-    CGFloat verticalSpaceInWebViewCoordinates = std::max((visibleSize.height - focusedElementRectInNewScale.height()) / 2.0, 0.0);
+    // If the element is too wide to be horizontally centered or too tall to be vertically centered, we
+    // instead scroll such that the left edge or top edge of the element is within the left half or top
+    // half of the viewport, respectively.
+    CGFloat horizontalSpaceInWebViewCoordinates = (visibleSize.width - focusedElementRectInNewScale.width()) / 2.0;
+    CGFloat verticalSpaceInWebViewCoordinates = (visibleSize.height - focusedElementRectInNewScale.height()) / 2.0;
+
+    auto topLeft = CGPointZero;
+    auto scrollViewInsets = [_scrollView _effectiveContentInset];
+    auto currentTopLeft = [_scrollView contentOffset];
+
+    if (_haveSetObscuredInsets) {
+        currentTopLeft.x += _obscuredInsets.left;
+        currentTopLeft.y += _obscuredInsets.top;
+    }
+
+    if (horizontalSpaceInWebViewCoordinates > 0)
+        topLeft.x = focusedElementRectInNewScale.x() - horizontalSpaceInWebViewCoordinates;
+    else {
+        auto minimumOffsetToRevealLeftEdge = std::max(-scrollViewInsets.left, focusedElementRectInNewScale.x() - visibleSize.width / 2);
+        auto maximumOffsetToRevealLeftEdge = focusedElementRectInNewScale.x();
+        topLeft.x = clampTo<double>(currentTopLeft.x, minimumOffsetToRevealLeftEdge, maximumOffsetToRevealLeftEdge);
+    }
+
+    if (verticalSpaceInWebViewCoordinates > 0)
+        topLeft.y = focusedElementRectInNewScale.y() - verticalSpaceInWebViewCoordinates;
+    else {
+        auto minimumOffsetToRevealTopEdge = std::max(-scrollViewInsets.top, focusedElementRectInNewScale.y() - visibleSize.height / 2);
+        auto maximumOffsetToRevealTopEdge = focusedElementRectInNewScale.y();
+        topLeft.y = clampTo<double>(currentTopLeft.y, minimumOffsetToRevealTopEdge, maximumOffsetToRevealTopEdge);
+    }
 
-    CGPoint topLeft;
-    topLeft.x = focusedElementRectInNewScale.x() - horizontalSpaceInWebViewCoordinates;
-    topLeft.y = focusedElementRectInNewScale.y() - verticalSpaceInWebViewCoordinates - visibleOffsetFromTop;
+    topLeft.y -= visibleOffsetFromTop;
+
+    WebCore::FloatRect documentBoundsInNewScale = [_contentView bounds];
+    documentBoundsInNewScale.scale(scale);
+    documentBoundsInNewScale.moveBy([_contentView frame].origin);
 
     CGFloat minimumAllowableHorizontalOffsetInWebViewCoordinates = -INFINITY;
     CGFloat minimumAllowableVerticalOffsetInWebViewCoordinates = -INFINITY;
+    CGFloat maximumAllowableHorizontalOffsetInWebViewCoordinates = CGRectGetMaxX(documentBoundsInNewScale) - visibleSize.width;
+    CGFloat maximumAllowableVerticalOffsetInWebViewCoordinates = CGRectGetMaxY(documentBoundsInNewScale) - visibleSize.height;
+
     if (selectionRectIsNotNull) {
         WebCore::FloatRect selectionRectInNewScale = selectionRectInDocumentCoordinates;
         selectionRectInNewScale.scale(scale);
         selectionRectInNewScale.moveBy([_contentView frame].origin);
+        // Adjust the min and max allowable scroll offsets, such that the selection rect remains visible.
         minimumAllowableHorizontalOffsetInWebViewCoordinates = CGRectGetMaxX(selectionRectInNewScale) + caretOffsetFromWindowEdge - visibleSize.width;
         minimumAllowableVerticalOffsetInWebViewCoordinates = CGRectGetMaxY(selectionRectInNewScale) + caretOffsetFromWindowEdge - visibleSize.height - visibleOffsetFromTop;
+        maximumAllowableHorizontalOffsetInWebViewCoordinates = std::min(maximumAllowableHorizontalOffsetInWebViewCoordinates, CGRectGetMinX(selectionRectInNewScale) - caretOffsetFromWindowEdge);
+        maximumAllowableVerticalOffsetInWebViewCoordinates = std::min(maximumAllowableVerticalOffsetInWebViewCoordinates, CGRectGetMinY(selectionRectInNewScale) - caretOffsetFromWindowEdge - visibleOffsetFromTop);
     }
 
-    WebCore::FloatRect documentBoundsInNewScale = [_contentView bounds];
-    documentBoundsInNewScale.scale(scale);
-    documentBoundsInNewScale.moveBy([_contentView frame].origin);
-
     // Constrain the left edge in document coordinates so that:
     //  - it isn't so small that the scrollVisibleRect isn't visible on the screen
     //  - it isn't so great that the document's right edge is less than the right edge of the screen
-    if (selectionRectIsNotNull && topLeft.x < minimumAllowableHorizontalOffsetInWebViewCoordinates)
-        topLeft.x = minimumAllowableHorizontalOffsetInWebViewCoordinates;
-    else {
-        CGFloat maximumAllowableHorizontalOffset = CGRectGetMaxX(documentBoundsInNewScale) - visibleSize.width;
-        if (topLeft.x > maximumAllowableHorizontalOffset)
-            topLeft.x = maximumAllowableHorizontalOffset;
-    }
+    topLeft.x = clampTo<CGFloat>(topLeft.x, minimumAllowableHorizontalOffsetInWebViewCoordinates, maximumAllowableHorizontalOffsetInWebViewCoordinates);
 
     // Constrain the top edge in document coordinates so that:
     //  - it isn't so small that the scrollVisibleRect isn't visible on the screen
     //  - it isn't so great that the document's bottom edge is higher than the top of the form assistant
-    if (selectionRectIsNotNull && topLeft.y < minimumAllowableVerticalOffsetInWebViewCoordinates)
-        topLeft.y = minimumAllowableVerticalOffsetInWebViewCoordinates;
-    else {
-        CGFloat maximumAllowableVerticalOffset = CGRectGetMaxY(documentBoundsInNewScale) - visibleSize.height;
-        if (topLeft.y > maximumAllowableVerticalOffset)
-            topLeft.y = maximumAllowableVerticalOffset;
+    topLeft.y = clampTo<CGFloat>(topLeft.y, minimumAllowableVerticalOffsetInWebViewCoordinates, maximumAllowableVerticalOffsetInWebViewCoordinates);
+
+    if (_haveSetObscuredInsets) {
+        // This looks unintuitive, but is necessary in order to precisely center the focused element in the visible area.
+        // The top left position already accounts for top and left obscured insets - i.e., a topLeft of (0, 0) corresponds
+        // to the top- and left-most point below (and to the right of) the top inset area and left inset areas, respectively.
+        // However, when telling WKScrollView to scroll to a given center position, this center position is computed relative
+        // to the coordinate space of the scroll view. Thus, to compute our center position from the top left position, we
+        // need to first move the top left position up and to the left, and then add half the width and height of the content
+        // area (including obscured insets).
+        topLeft.x -= _obscuredInsets.left;
+        topLeft.y -= _obscuredInsets.top;
     }
 
-    WebCore::FloatPoint newCenter = CGPointMake(topLeft.x + unobscuredScrollViewRectInWebViewCoordinates.size.width / 2.0, topLeft.y + unobscuredScrollViewRectInWebViewCoordinates.size.height / 2.0);
+    WebCore::FloatPoint newCenter = CGPointMake(topLeft.x + CGRectGetWidth(self.bounds) / 2, topLeft.y + CGRectGetHeight(self.bounds) / 2);
 
     if (scale != currentScale)
         _page->willStartUserTriggeredZooming();
index 1d15194..86e71fb 100644 (file)
@@ -101,7 +101,7 @@ struct PrintInfo;
 
 - (void)_scrollToContentScrollPosition:(WebCore::FloatPoint)scrollPosition scrollOrigin:(WebCore::IntPoint)scrollOrigin;
 - (BOOL)_scrollToRect:(WebCore::FloatRect)targetRect origin:(WebCore::FloatPoint)origin minimumScrollDistance:(float)minimumScrollDistance;
-- (void)_zoomToFocusRect:(WebCore::FloatRect)focusedElementRect selectionRect:(WebCore::FloatRect)selectionRectInDocumentCoordinates insideFixed:(BOOL)insideFixed fontSize:(float)fontSize minimumScale:(double)minimumScale maximumScale:(double)maximumScale allowScaling:(BOOL)allowScaling forceScroll:(BOOL)forceScroll;
+- (void)_zoomToFocusRect:(const WebCore::FloatRect&)focusedElementRect selectionRect:(const WebCore::FloatRect&)selectionRectInDocumentCoordinates insideFixed:(BOOL)insideFixed fontSize:(float)fontSize minimumScale:(double)minimumScale maximumScale:(double)maximumScale allowScaling:(BOOL)allowScaling forceScroll:(BOOL)forceScroll;
 - (BOOL)_zoomToRect:(WebCore::FloatRect)targetRect withOrigin:(WebCore::FloatPoint)origin fitEntireRect:(BOOL)fitEntireRect minimumScale:(double)minimumScale maximumScale:(double)maximumScale minimumScrollDistance:(float)minimumScrollDistance;
 - (void)_zoomOutWithOrigin:(WebCore::FloatPoint)origin animated:(BOOL)animated;
 - (void)_zoomToInitialScaleWithOrigin:(WebCore::FloatPoint)origin animated:(BOOL)animated;