Changeset 230171 in webkit

Timestamp:

Apr 2, 2018 12:06:44 PM (6 years ago)

Author:

Wenson Hsieh

Message:

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

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/API/Cocoa/WKWebView.mm (modified) (6 diffs)

trunk/Source/WebKit/ChangeLog

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

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

@@ -2121 +2121 @@
     UNUSED_PARAM(insideFixed);
 
-    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.
@@ -2154 +2144 @@
         CGFloat heightVisibleBelowFormAssistant = CGRectGetMaxY(visibleScrollViewBoundsInWebViewCoordinates) - CGRectGetMaxY(intersectionBetweenScrollViewAndFormAssistant);
 
-        if (heightVisibleAboveFormAssistant >= kMinimumHeightToShowContentAboveKeyboard || heightVisibleBelowFormAssistant < heightVisibleAboveFormAssistant)
+        if (heightVisibleAboveFormAssistant >= minimumHeightToShowContentAboveKeyboard || heightVisibleBelowFormAssistant < heightVisibleAboveFormAssistant)
             visibleSize.height = heightVisibleAboveFormAssistant;
         else {
@@ -2161 +2151 @@
         }
     }
+
+    // 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) {
+#if ENABLE(EXTRA_ZOOM_MODE)
+        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);
+#else
+        const double webViewStandardFontSize = 16;
+        scale = clampTo<double>(webViewStandardFontSize / fontSize, minimumScale, maximumScale);
+#endif
+    }
+
+    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();
@@ -2194 +2213 @@
         selectionRectInNewScale.scale(scale);
         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;
     }
 
@@ -2226 +2245 @@
     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)
         _page->willStartUserTriggeredZooming();
 
@@ -2233 +2252 @@
     // 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];
 }