[Extra zoom mode] Zoom level is sometimes excessive when zooming to focused form...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 2 Apr 2018 19:06:44 +0000 (19:06 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 2 Apr 2018 19:06:44 +0000 (19:06 +0000)

Reviewed by Timothy Hatcher.

Upon interactively focusing an element, we zoom and scroll to reveal that element. The heuristics introduced in
<https://trac.webkit.org/r168744> work by computing a target scale, and then a point to zoom to given that
scale. Currently, this scale is dependent on the computed font size of the form control, such that the form
control would be scaled to have an effective font size of 16.

However, in extra zoom mode, applying these same heuristics (ironically) results in excessive zoom levels, since
scaling the font up to 16 would cause most form controls to zoom so far in that we lose context of surrounding
elements such as labels and other form controls; the fact that the element is highlighted by the focused form
control overlay makes this even more confusing, since part of the focus overlay highlight rect often ends up
outside the viewport.

To fix this, we make a couple of tweaks to focus rect zooming in extra zoom mode. (1) Instead of computing
target zoom level based on font size, try to zoom such that the focused element rect fills up most of the
viewport (similar to double-tap zooming). This ensures that the focused form control overlay's highlight rect
makes sense in most cases, with few exceptions (e.g. the element frame is larger than the viewport). (2)
Introduce a minimum legible font size of 11, and compute the minimium scale needed such that the form control
font would appear to be at least this legible font size. Then, clamp the target scale chosen by (1) to this
minimum scale.

One additional consideration for (1) is that naively scaling to fit the element rect to the viewport (with some
fixed margins) would cause the viewport scale to always change when moving focus between form controls of
different dimensions, even if the current scale is more or less appropriate for all the focusable elements. To
address this, instead of computing a single target zoom scale for an element rect, compute a range of possible
target zoom scales (where the minimum and maximum values depend on the margin we add around the element rect).
If the current scale already falls within this target scale range, then we won't bother adjusting the scale at
all (unless the font size is too small — see (2)). If the current scale falls outside the target scale range, we
then make the minimal adjustment needed to ensure that the element rect fits well within the viewport without
being too small.

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

Move some logic around so that the target scale is computed after computing the visible size. Also renames some
constants local to this function (WKWebViewStandardFontSize, kMinimumHeightToShowContentAboveKeyboard,
UIWebFormAnimationDuration, CaretOffsetFromWindowEdge) such that they now share a consistent naming style.

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


index 1425aea..caa613a 100644 (file)
@@ -1,3 +1,47 @@
+2018-04-02  Wenson Hsieh  <wenson_hsieh@apple.com>
+        [Extra zoom mode] Zoom level is sometimes excessive when zooming to focused form controls
+        https://bugs.webkit.org/show_bug.cgi?id=184222
+        <rdar://problem/39063886>
+        Reviewed by Timothy Hatcher.
+        Upon interactively focusing an element, we zoom and scroll to reveal that element. The heuristics introduced in
+        <https://trac.webkit.org/r168744> work by computing a target scale, and then a point to zoom to given that
+        scale. Currently, this scale is dependent on the computed font size of the form control, such that the form
+        control would be scaled to have an effective font size of 16.
+        However, in extra zoom mode, applying these same heuristics (ironically) results in excessive zoom levels, since
+        scaling the font up to 16 would cause most form controls to zoom so far in that we lose context of surrounding
+        elements such as labels and other form controls; the fact that the element is highlighted by the focused form
+        control overlay makes this even more confusing, since part of the focus overlay highlight rect often ends up
+        outside the viewport.
+        To fix this, we make a couple of tweaks to focus rect zooming in extra zoom mode. (1) Instead of computing
+        target zoom level based on font size, try to zoom such that the focused element rect fills up most of the
+        viewport (similar to double-tap zooming). This ensures that the focused form control overlay's highlight rect
+        makes sense in most cases, with few exceptions (e.g. the element frame is larger than the viewport). (2)
+        Introduce a minimum legible font size of 11, and compute the minimium scale needed such that the form control
+        font would appear to be at least this legible font size. Then, clamp the target scale chosen by (1) to this
+        minimum scale.
+        One additional consideration for (1) is that naively scaling to fit the element rect to the viewport (with some
+        fixed margins) would cause the viewport scale to always change when moving focus between form controls of
+        different dimensions, even if the current scale is more or less appropriate for all the focusable elements. To
+        address this, instead of computing a single target zoom scale for an element rect, compute a range of possible
+        target zoom scales (where the minimum and maximum values depend on the margin we add around the element rect).
+        If the current scale already falls within this target scale range, then we won't bother adjusting the scale at
+        all (unless the font size is too small — see (2)). If the current scale falls outside the target scale range, we
+        then make the minimal adjustment needed to ensure that the element rect fits well within the viewport without
+        being too small.
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView _zoomToFocusRect:selectionRect:insideFixed:fontSize:minimumScale:maximumScale:allowScaling:forceScroll:]):
+        Move some logic around so that the target scale is computed after computing the visible size. Also renames some
+        constants local to this function (WKWebViewStandardFontSize, kMinimumHeightToShowContentAboveKeyboard,
+        UIWebFormAnimationDuration, CaretOffsetFromWindowEdge) such that they now share a consistent naming style.
 2018-04-02  Jer Noble  <jer.noble@apple.com>
         Enable Legacy EME for all WebKit & WebKitLegacy clients
index 0d18e15..5fb0e68 100644 (file)
@@ -2120,22 +2120,12 @@ static WebCore::FloatPoint constrainContentOffset(WebCore::FloatPoint contentOff
     LOG_WITH_STREAM(VisibleRects, stream << "_zoomToFocusRect:" << focusedElementRectInDocumentCoordinates << " selectionRect:" << selectionRectInDocumentCoordinates);
-    const double WKWebViewStandardFontSize = 16;
-    const double kMinimumHeightToShowContentAboveKeyboard = 106;
-    const CFTimeInterval UIWebFormAnimationDuration = 0.25;
-    const double CaretOffsetFromWindowEdge = 20;
-    // Zoom around the element's bounding frame. We use a "standard" size to determine the proper frame.
-    double scale = allowScaling ? std::min(std::max(WKWebViewStandardFontSize / fontSize, minimumScale), maximumScale) : contentZoomScale(self);
-    CGFloat documentWidth = [_contentView bounds].size.width;
-    scale = CGRound(documentWidth * scale) / documentWidth;
+    const double minimumHeightToShowContentAboveKeyboard = 106;
+    const CFTimeInterval formControlZoomAnimationDuration = 0.25;
+    const double caretOffsetFromWindowEdge = 20;
     UIWindow *window = [_scrollView window];
-    WebCore::FloatRect focusedElementRectInNewScale = focusedElementRectInDocumentCoordinates;
-    focusedElementRectInNewScale.scale(scale);
-    focusedElementRectInNewScale.moveBy([_contentView frame].origin);
     // Find the portion of the view that is visible on the screen.
     UIViewController *topViewController = [[[_scrollView _viewControllerForAncestor] _rootAncestorViewController] _viewControllerForSupportedInterfaceOrientations];
     UIView *fullScreenView = topViewController.view;
@@ -2153,7 +2143,7 @@ static WebCore::FloatPoint constrainContentOffset(WebCore::FloatPoint contentOff
         CGFloat heightVisibleAboveFormAssistant = CGRectGetMinY(intersectionBetweenScrollViewAndFormAssistant) - CGRectGetMinY(visibleScrollViewBoundsInWebViewCoordinates);
         CGFloat heightVisibleBelowFormAssistant = CGRectGetMaxY(visibleScrollViewBoundsInWebViewCoordinates) - CGRectGetMaxY(intersectionBetweenScrollViewAndFormAssistant);
-        if (heightVisibleAboveFormAssistant >= kMinimumHeightToShowContentAboveKeyboard || heightVisibleBelowFormAssistant < heightVisibleAboveFormAssistant)
+        if (heightVisibleAboveFormAssistant >= minimumHeightToShowContentAboveKeyboard || heightVisibleBelowFormAssistant < heightVisibleAboveFormAssistant)
             visibleSize.height = heightVisibleAboveFormAssistant;
         else {
             visibleSize.height = heightVisibleBelowFormAssistant;
@@ -2161,6 +2151,35 @@ static WebCore::FloatPoint constrainContentOffset(WebCore::FloatPoint contentOff
+    // Zoom around the element's bounding frame. We use a "standard" size to determine the proper frame.
+    double currentScale = contentZoomScale(self);
+    double scale = currentScale;
+    if (allowScaling) {
+        const double minimumLegibleFontSize = 11;
+        const CGFloat minimumMarginForZoomingToEntireFocusRectInWebViewCoordinates = 10;
+        const CGFloat maximumMarginForZoomingToEntireFocusRectInWebViewCoordinates = 35;
+        CGRect minimumTargetRectInDocumentCoordinates = UIRectInsetEdges(focusedElementRectInDocumentCoordinates, UIRectEdgeAll, -minimumMarginForZoomingToEntireFocusRectInWebViewCoordinates / currentScale);
+        CGRect maximumTargetRectInDocumentCoordinates = UIRectInsetEdges(focusedElementRectInDocumentCoordinates, UIRectEdgeAll, -maximumMarginForZoomingToEntireFocusRectInWebViewCoordinates / currentScale);
+        double clampedMaximumTargetScale = clampTo<double>(std::min(visibleSize.width / CGRectGetWidth(minimumTargetRectInDocumentCoordinates), visibleSize.height / CGRectGetHeight(minimumTargetRectInDocumentCoordinates)), minimumScale, maximumScale);
+        double clampedMinimumTargetScale = clampTo<double>(std::min(visibleSize.width / CGRectGetWidth(maximumTargetRectInDocumentCoordinates), visibleSize.height / CGRectGetHeight(maximumTargetRectInDocumentCoordinates)), minimumScale, maximumScale);
+        double targetScaleIgnoringFontSizeConstraints = clampTo<double>(currentScale, clampedMinimumTargetScale, clampedMaximumTargetScale);
+        scale = std::max<double>(minimumLegibleFontSize / fontSize, targetScaleIgnoringFontSizeConstraints);
+        const double webViewStandardFontSize = 16;
+        scale = clampTo<double>(webViewStandardFontSize / fontSize, minimumScale, maximumScale);
+    }
+    CGFloat documentWidth = [_contentView bounds].size.width;
+    scale = CGRound(documentWidth * scale) / documentWidth;
+    WebCore::FloatRect focusedElementRectInNewScale = focusedElementRectInDocumentCoordinates;
+    focusedElementRectInNewScale.scale(scale);
+    focusedElementRectInNewScale.moveBy([_contentView frame].origin);
     BOOL selectionRectIsNotNull = !selectionRectInDocumentCoordinates.isZero();
     if (!forceScroll) {
         CGRect currentlyVisibleRegionInWebViewCoordinates;
@@ -2193,8 +2212,8 @@ static WebCore::FloatPoint constrainContentOffset(WebCore::FloatPoint contentOff
         WebCore::FloatRect selectionRectInNewScale = selectionRectInDocumentCoordinates;
         selectionRectInNewScale.moveBy([_contentView frame].origin);
-        minimumAllowableHorizontalOffsetInWebViewCoordinates = CGRectGetMaxX(selectionRectInNewScale) + CaretOffsetFromWindowEdge - visibleSize.width;
-        minimumAllowableVerticalOffsetInWebViewCoordinates = CGRectGetMaxY(selectionRectInNewScale) + CaretOffsetFromWindowEdge - visibleSize.height - visibleOffsetFromTop;
+        minimumAllowableHorizontalOffsetInWebViewCoordinates = CGRectGetMaxX(selectionRectInNewScale) + caretOffsetFromWindowEdge - visibleSize.width;
+        minimumAllowableVerticalOffsetInWebViewCoordinates = CGRectGetMaxY(selectionRectInNewScale) + caretOffsetFromWindowEdge - visibleSize.height - visibleOffsetFromTop;
     WebCore::FloatRect documentBoundsInNewScale = [_contentView bounds];
@@ -2225,17 +2244,14 @@ static WebCore::FloatPoint constrainContentOffset(WebCore::FloatPoint contentOff
     WebCore::FloatPoint newCenter = CGPointMake(topLeft.x + unobscuredScrollViewRectInWebViewCoordinates.size.width / 2.0, topLeft.y + unobscuredScrollViewRectInWebViewCoordinates.size.height / 2.0);
-    if (scale != contentZoomScale(self))
+    if (scale != currentScale)
     LOG_WITH_STREAM(VisibleRects, stream << "_zoomToFocusRect: zooming to " << newCenter << " scale:" << scale);
     // The newCenter has been computed in the new scale, but _zoomToCenter expected the center to be in the original scale.
     newCenter.scale(1 / scale);
-    [_scrollView _zoomToCenter:newCenter
-                        scale:scale
-                     duration:UIWebFormAnimationDuration
-                        force:YES];
+    [_scrollView _zoomToCenter:newCenter scale:scale duration:formControlZoomAnimationDuration force:YES];
 - (CGFloat)_targetContentZoomScaleForRect:(const WebCore::FloatRect&)targetRect currentScale:(double)currentScale fitEntireRect:(BOOL)fitEntireRect minimumScale:(double)minimumScale maximumScale:(double)maximumScale