REGRESSION (r244897): Caret may appear wider than normal after zooming to focus an...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 4 May 2019 02:40:29 +0000 (02:40 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 4 May 2019 02:40:29 +0000 (02:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197579

Reviewed by Tim Horton.

Source/WebKit:

Fixes a couple of flaky tests (CaretSelectionRectAfterRestoringFirstResponderWithRetainActiveFocusedState and
CaretSelectionRectAfterRestoringFirstResponder) that began failing after r244897. These tests both begin by
focusing an editable element, which causes the web view to zoom in. The tests subsequently check that the caret
rect is {{ 16, 13 }, { 2, 15 }}. While the specified caret rect (coming from EditorState) is {{ 16, 13 }, { 3,
15 }}, the narrower caret rect is used because we take measures to preserve the width of the caret relative to
the view (see the inverse scaling logic in -[WKContentView selectedTextRange] for more details).

See below for more details.

* UIProcess/ios/WKContentViewInteraction.h:

Remove _isZoomingToRevealFocusedElement, now that we don't need it anymore (see -observeValueForKeyPath:).

* UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView cleanupInteraction]):
(-[WKContentView observeValueForKeyPath:ofObject:change:context:]):

Stop bailing from a selection update when changing scale, while zooming to reveal the focused element. This
check was added in r239441 to prevent UIWKTextInteractionAssistant's selection scrolling logic from interfering
with WKContentView-driven logic for zooming to the focused element. However, since r244141, this is no longer
necessary since selection scrolling is only driven by code in the web process.

This new update while zooming to reveal the focused element ensures that the WKTextRange returned by
-selectedTextRange after zooming will have a width that is inversely scaled using the content view's current
scale, such that it has a consistent width (relative to the web view) across different scales.

(-[WKContentView _zoomToRevealFocusedElement]):
* UIProcess/ios/WebPageProxyIOS.mm:
(WebKit::WebPageProxy::layerTreeCommitComplete):

Remove any attempt here to notify PageClient about editor states after focus. This logic was actually incorrect,
since it didn't ensure that the layer tree commit that is being completed actually contained an editor state; as
such, the "editor state" received here could be stale.

Tools:

Fixes a couple of flaky layout tests (ModifyInputAssistantItemBarButtonGroups and
OverrideInputAssistantItemBarButtonGroups) by programmatically blurring focused elements and waiting for the
input session to change, rather than relying on -resignFirstResponder and -waitForNextPresentationUpdate to
ensure that the the focused element has been blurred.

* TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/ios/WKContentViewInteraction.h
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm

index 8b3b245..270fa07 100644 (file)
@@ -1,3 +1,44 @@
+2019-05-03  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r244897): Caret may appear wider than normal after zooming to focus an editable element
+        https://bugs.webkit.org/show_bug.cgi?id=197579
+
+        Reviewed by Tim Horton.
+
+        Fixes a couple of flaky tests (CaretSelectionRectAfterRestoringFirstResponderWithRetainActiveFocusedState and
+        CaretSelectionRectAfterRestoringFirstResponder) that began failing after r244897. These tests both begin by
+        focusing an editable element, which causes the web view to zoom in. The tests subsequently check that the caret
+        rect is {{ 16, 13 }, { 2, 15 }}. While the specified caret rect (coming from EditorState) is {{ 16, 13 }, { 3,
+        15 }}, the narrower caret rect is used because we take measures to preserve the width of the caret relative to
+        the view (see the inverse scaling logic in -[WKContentView selectedTextRange] for more details).
+
+        See below for more details.
+
+        * UIProcess/ios/WKContentViewInteraction.h:
+
+        Remove _isZoomingToRevealFocusedElement, now that we don't need it anymore (see -observeValueForKeyPath:).
+
+        * UIProcess/ios/WKContentViewInteraction.mm:
+        (-[WKContentView cleanupInteraction]):
+        (-[WKContentView observeValueForKeyPath:ofObject:change:context:]):
+
+        Stop bailing from a selection update when changing scale, while zooming to reveal the focused element. This
+        check was added in r239441 to prevent UIWKTextInteractionAssistant's selection scrolling logic from interfering
+        with WKContentView-driven logic for zooming to the focused element. However, since r244141, this is no longer
+        necessary since selection scrolling is only driven by code in the web process.
+
+        This new update while zooming to reveal the focused element ensures that the WKTextRange returned by
+        -selectedTextRange after zooming will have a width that is inversely scaled using the content view's current
+        scale, such that it has a consistent width (relative to the web view) across different scales.
+
+        (-[WKContentView _zoomToRevealFocusedElement]):
+        * UIProcess/ios/WebPageProxyIOS.mm:
+        (WebKit::WebPageProxy::layerTreeCommitComplete):
+
+        Remove any attempt here to notify PageClient about editor states after focus. This logic was actually incorrect,
+        since it didn't ensure that the layer tree commit that is being completed actually contained an editor state; as
+        such, the "editor state" received here could be stale.
+
 2019-05-03  Zalan Bujtas  <zalan@apple.com>
 
         [iOS] outlook.live.com: Compose email frame not fully visible and not scrollable
index a61f519..05a05b0 100644 (file)
@@ -321,7 +321,6 @@ struct WKAutoCorrectionData {
     BOOL _didAccessoryTabInitiateFocus;
     BOOL _isExpectingFastSingleTapCommit;
     BOOL _showDebugTapHighlightsForFastClicking;
-    BOOL _isZoomingToRevealFocusedElement;
 
     BOOL _keyboardDidRequestDismissal;
 
index db1dd76..12e8f3d 100644 (file)
@@ -934,7 +934,6 @@ static inline bool hasFocusedElement(WebKit::FocusedElementInformation focusedEl
 
     _hasSetUpInteractions = NO;
     _suppressSelectionAssistantReasons = { };
-    _isZoomingToRevealFocusedElement = NO;
 
 #if ENABLE(POINTER_EVENTS)
     [self _resetPanningPreventionFlags];
@@ -1031,12 +1030,6 @@ static inline bool hasFocusedElement(WebKit::FocusedElementInformation focusedEl
 
     [self _updateTapHighlight];
 
-    if (_isZoomingToRevealFocusedElement) {
-        // When zooming to the focused element, avoid additionally scrolling to reveal the selection. Since the scale transform has not yet been
-        // applied to the content view at this point, we'll end up scrolling to reveal a rect that is computed using the wrong scale.
-        return;
-    }
-
     _selectionNeedsUpdate = YES;
     [self _updateChangedSelection:YES];
 }
@@ -1671,7 +1664,6 @@ static NSValue *nsSizeForTapHighlightBorderRadius(WebCore::IntSize borderRadius,
     if (_suppressSelectionAssistantReasons.contains(WebKit::EditableRootIsTransparentOrFullyClipped) || _suppressSelectionAssistantReasons.contains(WebKit::FocusedElementIsTooSmall))
         return;
 
-    SetForScope<BOOL> isZoomingToRevealFocusedElementForScope { _isZoomingToRevealFocusedElement, YES };
     // In case user scaling is force enabled, do not use that scaling when zooming in with an input field.
     // Zooming above the page's default scale factor should only happen when the user performs it.
     [self _zoomToFocusRect:_focusedElementInformation.elementRect
index a670f9c..9030b7b 100644 (file)
@@ -430,11 +430,6 @@ bool WebPageProxy::updateLayoutViewportParameters(const WebKit::RemoteLayerTreeT
 void WebPageProxy::layerTreeCommitComplete()
 {
     pageClient().layerTreeCommitComplete();
-
-    if (m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement && !m_editorState.isMissingPostLayoutData) {
-        pageClient().didReceiveEditorStateUpdateAfterFocus();
-        m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement = false;
-    }
 }
 
 void WebPageProxy::selectWithGesture(const WebCore::IntPoint point, WebCore::TextGranularity granularity, uint32_t gestureType, uint32_t gestureState, bool isInteractingWithFocusedElement, WTF::Function<void(const WebCore::IntPoint&, uint32_t, uint32_t, uint32_t, CallbackBase::Error)>&& callbackFunction)
index b3f6875..6e7f334 100644 (file)
@@ -1,3 +1,17 @@
+2019-05-03  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r244897): Caret may appear wider than normal after zooming to focus an editable element
+        https://bugs.webkit.org/show_bug.cgi?id=197579
+
+        Reviewed by Tim Horton.
+
+        Fixes a couple of flaky layout tests (ModifyInputAssistantItemBarButtonGroups and
+        OverrideInputAssistantItemBarButtonGroups) by programmatically blurring focused elements and waiting for the
+        input session to change, rather than relying on -resignFirstResponder and -waitForNextPresentationUpdate to
+        ensure that the the focused element has been blurred.
+
+        * TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:
+
 2019-05-02  Alexey Proskuryakov  <ap@apple.com>
 
         Add a tool to block spammer accounts
index 777c999..655ef13 100644 (file)
@@ -213,25 +213,18 @@ TEST(KeyboardInputTests, ModifyInputAssistantItemBarButtonGroups)
     item.leadingBarButtonGroups = @[ leadingItems ];
     item.trailingBarButtonGroups = @[ trailingItems ];
 
-    bool doneWaitingForInputSession = false;
     [inputDelegate setFocusStartsInputSessionPolicyHandler:[&] (WKWebView *, id <_WKFocusedElementInfo>) -> _WKFocusStartsInputSessionPolicy {
-        doneWaitingForInputSession = true;
         return _WKFocusStartsInputSessionPolicyAllow;
     }];
-    [webView evaluateJavaScript:@"document.body.focus()" completionHandler:nil];
-    TestWebKitAPI::Util::run(&doneWaitingForInputSession);
+    [webView evaluateJavaScriptAndWaitForInputSessionToChange:@"document.body.focus()"];
 
     EXPECT_EQ([webView firstResponder], [webView textInputContentView]);
     EXPECT_TRUE([[webView firstResponder].inputAssistantItem.leadingBarButtonGroups containsObject:leadingItems]);
     EXPECT_TRUE([[webView firstResponder].inputAssistantItem.trailingBarButtonGroups containsObject:trailingItems]);
 
     // Now blur and refocus the editable area, and check that the same leading and trailing button items are present.
-    [webView resignFirstResponder];
-    [webView waitForNextPresentationUpdate];
-
-    doneWaitingForInputSession = false;
-    [webView evaluateJavaScript:@"document.body.focus()" completionHandler:nil];
-    TestWebKitAPI::Util::run(&doneWaitingForInputSession);
+    [webView evaluateJavaScriptAndWaitForInputSessionToChange:@"document.body.blur()"];
+    [webView evaluateJavaScriptAndWaitForInputSessionToChange:@"document.body.focus()"];
 
     EXPECT_EQ([webView firstResponder], [webView textInputContentView]);
     EXPECT_TRUE([[webView firstResponder].inputAssistantItem.leadingBarButtonGroups containsObject:leadingItems]);
@@ -244,14 +237,10 @@ TEST(KeyboardInputTests, OverrideInputAssistantItemBarButtonGroups)
     auto webView = adoptNS([[InputAssistantItemTestingWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
     [webView _setInputDelegate:inputDelegate.get()];
     [webView synchronouslyLoadHTMLString:@"<body contenteditable>"];
-
-    bool doneWaitingForInputSession = false;
     [inputDelegate setFocusStartsInputSessionPolicyHandler:[&] (WKWebView *, id <_WKFocusedElementInfo>) -> _WKFocusStartsInputSessionPolicy {
-        doneWaitingForInputSession = true;
         return _WKFocusStartsInputSessionPolicyAllow;
     }];
-    [webView evaluateJavaScript:@"document.body.focus()" completionHandler:nil];
-    TestWebKitAPI::Util::run(&doneWaitingForInputSession);
+    [webView evaluateJavaScriptAndWaitForInputSessionToChange:@"document.body.focus()"];
 
     UIBarButtonItemGroup *leadingItems = [InputAssistantItemTestingWebView leadingItemsForWebView:webView.get()];
     UIBarButtonItemGroup *trailingItems = [InputAssistantItemTestingWebView trailingItemsForWebView:webView.get()];
@@ -260,12 +249,8 @@ TEST(KeyboardInputTests, OverrideInputAssistantItemBarButtonGroups)
     EXPECT_TRUE([[webView firstResponder].inputAssistantItem.trailingBarButtonGroups containsObject:trailingItems]);
 
     // Now blur and refocus the editable area, and check that the same leading and trailing button items are present.
-    [webView resignFirstResponder];
-    [webView waitForNextPresentationUpdate];
-
-    doneWaitingForInputSession = false;
-    [webView evaluateJavaScript:@"document.body.focus()" completionHandler:nil];
-    TestWebKitAPI::Util::run(&doneWaitingForInputSession);
+    [webView evaluateJavaScriptAndWaitForInputSessionToChange:@"document.body.blur()"];
+    [webView evaluateJavaScriptAndWaitForInputSessionToChange:@"document.body.focus()"];
 
     EXPECT_EQ([webView firstResponder], [webView textInputContentView]);
     EXPECT_TRUE([[webView firstResponder].inputAssistantItem.leadingBarButtonGroups containsObject:leadingItems]);