Attachment 357424 Details for Bug 192745 – Patch

[patch] Patch

bug-192745-20181216182129.patch (text/plain), 17.86 KB, created by Wenson Hsieh on 2018-12-16 18:21:30 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2018-12-16 18:21:30 PST

Size: 17.86 KB

patch

obsolete

>Subversion Revision: 239262
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 71e152e10e7435374bb5b5977d437e6db79a62a3..29db3faf77700406e3fbebe67e238c5e68b7dbaa 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,40 @@
>+2018-12-16  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 NOBODY (OOPS!).
>+
>+        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.
>+
>+        * 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-16  Chris Fleizach  <cfleizach@apple.com>
> 
>         AX: Support keyboard access preference for iOS in WebKit
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm b/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>index ba2048eabaab68cd733825c64f8b920fab96441e..22983fc5221a711a084b8ee022f63be64f026529 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>@@ -2225,7 +2225,7 @@ - (void)_zoomToInitialScaleWithOrigin:(WebCore::FloatPoint)origin animated:(BOOL
> }
> 
> // 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 @@ - (void)_zoomToFocusRect:(WebCore::FloatRect)focusedElementRectInDocumentCoordin
>             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();
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h b/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h
>index 1d15194cbfba9a9d6aa27479cc7ba5ae86b3b75a..86e71fb619ed235b98cedb95b30344ef70731e6d 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h
>@@ -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;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 28d445a7e29d4973bf15c1efc13310112a215c30..4739bca644ce928e9986ba37c3d207e68111bb19 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-12-16  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 NOBODY (OOPS!).
>+
>+        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-15  Youenn Fablet  <youenn@apple.com>
> 
>         Make RTCRtpSender.setParameters to activate specific encodings
>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
>index 0000000000000000000000000000000000000000..236c5fffda47d428d2d3297f5791ae963dcf9e10
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..48861e1e35b9b3139d456661af202021b3d7f514
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/no-scrolling-when-focusing-large-editable-area.html
>@@ -0,0 +1,40 @@
>+<!DOCTYPE html> 
>+<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

Actions: View | Formatted Diff | Diff

Attachments on bug 192745: 357424 | 357425