- Revision
- 239314
- Author
- wenson_hs...@apple.com
- Date
- 2018-12-17 20:04:44 -0800 (Mon, 17 Dec 2018)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (239313 => 239314)
--- trunk/LayoutTests/ChangeLog 2018-12-18 03:52:53 UTC (rev 239313)
+++ trunk/LayoutTests/ChangeLog 2018-12-18 04:04:44 UTC (rev 239314)
@@ -1,3 +1,17 @@
+2018-12-17 Wenson Hsieh <wenson_hs...@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 <rn...@webkit.org>
offsetLeft and offsetParent should adjust across shadow boundaries
Added: trunk/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt (0 => 239314)
--- trunk/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt 2018-12-18 04:04:44 UTC (rev 239314)
@@ -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
Added: trunk/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html (0 => 239314)
--- trunk/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html 2018-12-18 04:04:44 UTC (rev 239314)
@@ -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=""
+</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
Modified: trunk/Source/WebKit/ChangeLog (239313 => 239314)
--- trunk/Source/WebKit/ChangeLog 2018-12-18 03:52:53 UTC (rev 239313)
+++ trunk/Source/WebKit/ChangeLog 2018-12-18 04:04:44 UTC (rev 239314)
@@ -1,3 +1,41 @@
+2018-12-17 Wenson Hsieh <wenson_hs...@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 <rn...@webkit.org>
offsetLeft and offsetParent should adjust across shadow boundaries
Modified: trunk/Source/WebKit/Platform/spi/ios/UIKitSPI.h (239313 => 239314)
--- trunk/Source/WebKit/Platform/spi/ios/UIKitSPI.h 2018-12-18 03:52:53 UTC (rev 239313)
+++ trunk/Source/WebKit/Platform/spi/ios/UIKitSPI.h 2018-12-18 04:04:44 UTC (rev 239314)
@@ -348,6 +348,7 @@
@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)
Modified: trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm (239313 => 239314)
--- trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm 2018-12-18 03:52:53 UTC (rev 239313)
+++ trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm 2018-12-18 04:04:44 UTC (rev 239314)
@@ -2225,7 +2225,7 @@
}
// 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 @@
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;
- CGPoint topLeft;
- topLeft.x = focusedElementRectInNewScale.x() - horizontalSpaceInWebViewCoordinates;
- topLeft.y = focusedElementRectInNewScale.y() - verticalSpaceInWebViewCoordinates - visibleOffsetFromTop;
+ 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;
+ 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();
Modified: trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h (239313 => 239314)
--- trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h 2018-12-18 03:52:53 UTC (rev 239313)
+++ trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h 2018-12-18 04:04:44 UTC (rev 239314)
@@ -101,7 +101,7 @@
- (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;
