Changeset 239314 in webkit

Timestamp:

Dec 17, 2018 8:04:44 PM (5 years ago)

Author:

Wenson Hsieh

Message:

[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.

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt (added)
• LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html (added)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/Platform/spi/ios/UIKitSPI.h (modified) (1 diff)
• Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm (modified) (2 diffs)
• Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/Platform/spi/ios/UIKitSPI.h

@@ -349 +349 @@
 @property (nonatomic, getter=_indicatorInsetAdjustmentBehavior, setter=_setIndicatorInsetAdjustmentBehavior:) UIScrollViewIndicatorInsetAdjustmentBehavior indicatorInsetAdjustmentBehavior;
 @property (nonatomic, readonly) UIEdgeInsets _systemContentInset;
+@property (nonatomic, readonly) UIEdgeInsets _effectiveContentInset;
 @end
 

trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm

@@ -2226 +2226 @@
 
 // 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
 {
@@ -2306 +2306 @@
     }
 
-    // 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);
-
-    CGPoint topLeft;
-    topLeft.x = focusedElementRectInNewScale.x() - horizontalSpaceInWebViewCoordinates;
-    topLeft.y = focusedElementRectInNewScale.y() - verticalSpaceInWebViewCoordinates - visibleOffsetFromTop;
+    // 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);
+    }
+
+    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;
-    }
-
-    WebCore::FloatRect documentBoundsInNewScale = [_contentView bounds];
-    documentBoundsInNewScale.scale(scale);
-    documentBoundsInNewScale.moveBy([_contentView frame].origin);
+        maximumAllowableHorizontalOffsetInWebViewCoordinates = std::min(maximumAllowableHorizontalOffsetInWebViewCoordinates, CGRectGetMinX(selectionRectInNewScale) - caretOffsetFromWindowEdge);
+        maximumAllowableVerticalOffsetInWebViewCoordinates = std::min(maximumAllowableVerticalOffsetInWebViewCoordinates, CGRectGetMinY(selectionRectInNewScale) - caretOffsetFromWindowEdge - visibleOffsetFromTop);
+    }
 
     // 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;
-    }
-
-    WebCore::FloatPoint newCenter = CGPointMake(topLeft.x + unobscuredScrollViewRectInWebViewCoordinates.size.width / 2.0, topLeft.y + unobscuredScrollViewRectInWebViewCoordinates.size.height / 2.0);
+    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 + CGRectGetWidth(self.bounds) / 2, topLeft.y + CGRectGetHeight(self.bounds) / 2);
 
     if (scale != currentScale)

trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h

@@ -102 +102 @@
 - (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;