[iOS] Entering 2FA code on idmsa.apple.com causes unexpected scrolling
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jul 2019 19:55:20 +0000 (19:55 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jul 2019 19:55:20 +0000 (19:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199949
<rdar://problem/49944428>

Reviewed by Tim Horton and Megan Gardner.

Source/WebKit:

Since at least iOS 11, -[UIScrollView _adjustForAutomaticKeyboardInfo:animated:lastAdjustment:] adjusts the
scroll view's content offset to account for updated keyboard bottom insets. In WebKit, we call this method
whenever keyboard geometry changes (based on system notifications, such as UIKeyboardWillHideNotification).

When switching between focused form fields, we hide the keyboard for the previous focused element prior to
showing the keyboard for the newly focused element. This means that we will actually dismiss the keyboard in the
process of changing the focused element, which posts keyboard geometry notifications, which causes us to scroll
WKScrollView.

On iOS 12, this would be immediately followed by re-presenting the keyboard for the new focused element, which
causes us to adjust the scroll view back to its original position right away; this means that the scrolling that
happens as a result of adjusting for the keyboard insets after dismissal doesn't result in any visible change.

However, on iOS 13, after r239441 and r244546, we now defer scrolling and zooming to reveal the focused element
until later; this means the scrolling that happens as a result of initially dismissing the keyboard now causes a
consistent jump in the scroll view's scroll position (whereas on iOS 12, this only happens rarely, and the jump
is also less noticeable).

To mitigate this, we detect the case where we're moving focus from one element to another; if we're about to
show a keyboard for the newly focused element, then we should avoid scrolling as a result of the impending
"keyboard will hide" notification.

Test: fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html

* UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _keyboardChangedWithInfo:adjustScrollView:]):
(-[WKWebView _keyboardWillHide:]):
* UIProcess/ios/WKContentViewInteraction.h:
* UIProcess/ios/WKContentViewInteraction.mm:
(shouldShowKeyboardForElement):

Add a helper to determine whether we're focusing an element which presents a "keyboard" (i.e. a UIKit input
view, as opposed to modal select pickers, modal date pickers, or fields with inputmode="none", for which we
don't show an input view).

(-[WKContentView _elementDidFocus:userIsInteracting:blurPreviousNode:activityStateChanges:userObject:]):
(-[WKContentView shouldIgnoreKeyboardWillHideNotification]):

LayoutTests:

Add a new layout test to verify that moving focus between horizontally adjacent form controls doesn't induce
vertical scrolling.

* fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields-expected.txt: Added.
* fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html [new file with mode: 0644]
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
Source/WebKit/UIProcess/ios/WKContentViewInteraction.h
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm

index 7a822bb..567a631 100644 (file)
@@ -1,3 +1,17 @@
+2019-07-19  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [iOS] Entering 2FA code on idmsa.apple.com causes unexpected scrolling
+        https://bugs.webkit.org/show_bug.cgi?id=199949
+        <rdar://problem/49944428>
+
+        Reviewed by Tim Horton and Megan Gardner.
+
+        Add a new layout test to verify that moving focus between horizontally adjacent form controls doesn't induce
+        vertical scrolling.
+
+        * fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields-expected.txt: Added.
+        * fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html: Added.
+
 2019-07-19  Antoine Quint  <graouts@apple.com>
 
         Links stop working after long-pressing a link (WK1)
diff --git a/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields-expected.txt b/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields-expected.txt
new file mode 100644 (file)
index 0000000..042da75
--- /dev/null
@@ -0,0 +1,7 @@
+PASS Did not observe unnecessary scrolling
+  
+This test verifies that moving focus between two fields that are at the same y offset does not induce vertical scrolling. To test manually, focus the left field and press any key. Focus should move to the right field, and there should be no scrolling.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
diff --git a/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html b/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html
new file mode 100644 (file)
index 0000000..7f02ba2
--- /dev/null
@@ -0,0 +1,112 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ shouldIgnoreMetaViewport=true ] -->
+<html>
+<head>
+    <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
+    <script src="../../../resources/js-test-pre.js"></script>
+    <script src="../../../resources/ui-helper.js"></script>
+    <style>
+        html, body {
+            margin: 0;
+            width: 320px;
+            height: 100%;
+            overflow: scroll;
+        }
+
+        input {
+            width: 44px;
+            height: 40px;
+            font-size: 18px;
+        }
+
+        .outer-container {
+            display: flex;
+            align-items: center;
+            justify-content: center;
+            height: 400px;
+            width: 100%;
+        }
+
+        .inner-container {
+            margin: 0 auto;
+            text-align: center;
+        }
+    </style>
+</head>
+<body>
+    <div class="outer-container">
+        <div class="inner-container">
+            <input name="search" type="tel" id="first"></input>
+            <input type="tel" id="second"></input>
+        </div>
+    </div>
+    <div id="description"></div>
+    <div id="output"></div>
+    <script>
+    if (window.testRunner) {
+        testRunner.dumpAsText();
+        testRunner.waitUntilDone();
+    }
+
+    firstField = document.getElementById("first");
+    secondField = document.getElementById("second");
+    output = document.getElementById("output");
+    currentScrollPosition = 0;
+    shouldLogScrollDeltas = false;
+    observedUnnecessaryScrolling = false;
+
+    function moveFocusToOtherField(event) {
+        event.target.value = event.data;
+        const fieldToFocus = event.target == firstField ? secondField : firstField;
+        fieldToFocus.focus();
+        event.preventDefault();
+    }
+
+    firstField.addEventListener("input", moveFocusToOtherField);
+    secondField.addEventListener("input", moveFocusToOtherField);
+
+    document.addEventListener("scroll", () => {
+        const scrollDelta = document.scrollingElement.scrollTop - currentScrollPosition;
+        if (shouldLogScrollDeltas && Math.abs(scrollDelta) > 1) {
+            testFailed(`Scrolled by amount: ${scrollDelta}`);
+            observedUnnecessaryScrolling = true;
+        }
+        currentScrollPosition = document.scrollingElement.scrollTop;
+    });
+
+    addEventListener("load", async () => {
+        description("This test verifies that moving focus between two fields that are at the same y offset does not induce vertical scrolling. To test manually, focus the left field and press any key. Focus should move to the right field, and there should be no scrolling.");
+        if (!window.testRunner) {
+            shouldLogScrollDeltas = true;
+            return;
+        }
+
+        await UIHelper.setHardwareKeyboardAttached(false);
+        await UIHelper.activateElementAndWaitForInputSession(firstField);
+        await UIHelper.ensureVisibleContentRectUpdate();
+
+        shouldLogScrollDeltas = true;
+
+        for (let i = 0; i < 10; ++i) {
+            const observedInputEvent = new Promise(resolve => {
+                const resolveAfterZeroDelay = () => setTimeout(resolve, 0);
+                document.activeElement.addEventListener("input", resolveAfterZeroDelay, { "once" : true });
+            });
+            await UIHelper.keyDown(String(i));
+            await observedInputEvent;
+        }
+
+        if (observedUnnecessaryScrolling)
+            testFailed("Observed unnecessary scrolling");
+        else
+            testPassed("Did not observe unnecessary scrolling");
+
+        shouldLogScrollDeltas = false;
+
+        document.activeElement.blur();
+        await UIHelper.waitForKeyboardToHide();
+
+        testRunner.notifyDone();
+    });
+    </script>
+</body>
+</html>
index d3ee6c7..e5fb38c 100644 (file)
@@ -1,3 +1,49 @@
+2019-07-19  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [iOS] Entering 2FA code on idmsa.apple.com causes unexpected scrolling
+        https://bugs.webkit.org/show_bug.cgi?id=199949
+        <rdar://problem/49944428>
+
+        Reviewed by Tim Horton and Megan Gardner.
+
+        Since at least iOS 11, -[UIScrollView _adjustForAutomaticKeyboardInfo:animated:lastAdjustment:] adjusts the
+        scroll view's content offset to account for updated keyboard bottom insets. In WebKit, we call this method
+        whenever keyboard geometry changes (based on system notifications, such as UIKeyboardWillHideNotification).
+
+        When switching between focused form fields, we hide the keyboard for the previous focused element prior to
+        showing the keyboard for the newly focused element. This means that we will actually dismiss the keyboard in the
+        process of changing the focused element, which posts keyboard geometry notifications, which causes us to scroll
+        WKScrollView.
+
+        On iOS 12, this would be immediately followed by re-presenting the keyboard for the new focused element, which
+        causes us to adjust the scroll view back to its original position right away; this means that the scrolling that
+        happens as a result of adjusting for the keyboard insets after dismissal doesn't result in any visible change.
+
+        However, on iOS 13, after r239441 and r244546, we now defer scrolling and zooming to reveal the focused element
+        until later; this means the scrolling that happens as a result of initially dismissing the keyboard now causes a
+        consistent jump in the scroll view's scroll position (whereas on iOS 12, this only happens rarely, and the jump
+        is also less noticeable).
+
+        To mitigate this, we detect the case where we're moving focus from one element to another; if we're about to
+        show a keyboard for the newly focused element, then we should avoid scrolling as a result of the impending
+        "keyboard will hide" notification.
+
+        Test: fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html
+
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView _keyboardChangedWithInfo:adjustScrollView:]):
+        (-[WKWebView _keyboardWillHide:]):
+        * UIProcess/ios/WKContentViewInteraction.h:
+        * UIProcess/ios/WKContentViewInteraction.mm:
+        (shouldShowKeyboardForElement):
+
+        Add a helper to determine whether we're focusing an element which presents a "keyboard" (i.e. a UIKit input
+        view, as opposed to modal select pickers, modal date pickers, or fields with inputmode="none", for which we
+        don't show an input view).
+
+        (-[WKContentView _elementDidFocus:userIsInteracting:blurPreviousNode:activityStateChanges:userObject:]):
+        (-[WKContentView shouldIgnoreKeyboardWillHideNotification]):
+
 2019-07-18  Alex Christensen  <achristensen@webkit.org>
 
         Fix warning when importing WebKit in Swift
index 46647ce..1a75fa3 100644 (file)
@@ -3329,6 +3329,9 @@ static int32_t activeOrientation(WKWebView *webView)
         SetForScope<BOOL> insetAdjustmentGuard(_currentlyAdjustingScrollViewInsetsForKeyboard, YES);
         [_scrollView _adjustForAutomaticKeyboardInfo:keyboardInfo animated:YES lastAdjustment:&_lastAdjustmentForScroller];
         CGFloat bottomInsetAfterAdjustment = [_scrollView contentInset].bottom;
+        // FIXME: This "total bottom content inset adjustment" mechanism hasn't worked since iOS 11, since -_adjustForAutomaticKeyboardInfo:animated:lastAdjustment:
+        // no longer sets -[UIScrollView contentInset] for apps linked on or after iOS 11. We should consider removing this logic, since the original bug this was
+        // intended to fix, <rdar://problem/23202254>, remains fixed through other means.
         if (bottomInsetBeforeAdjustment != bottomInsetAfterAdjustment)
             _totalScrollViewBottomInsetAdjustmentForKeyboard += bottomInsetAfterAdjustment - bottomInsetBeforeAdjustment;
     }
@@ -3371,10 +3374,7 @@ static int32_t activeOrientation(WKWebView *webView)
 
 - (void)_keyboardWillHide:(NSNotification *)notification
 {
-    // Ignore keyboard will hide notifications sent during rotation. They're just there for
-    // backwards compatibility reasons and processing the will hide notification would
-    // temporarily screw up the unobscured view area.
-    if ([[UIPeripheralHost sharedInstance] rotationState])
+    if ([_contentView shouldIgnoreKeyboardWillHideNotification])
         return;
 
     [self _keyboardChangedWithInfo:notification.userInfo adjustScrollView:YES];
index 0fc028a..36efc6c 100644 (file)
@@ -345,6 +345,7 @@ struct WKAutoCorrectionData {
     BOOL _resigningFirstResponder;
     BOOL _needsDeferredEndScrollingSelectionUpdate;
     BOOL _isChangingFocus;
+    BOOL _isFocusingElementWithKeyboard;
     BOOL _isBlurringFocusedElement;
 
     BOOL _focusRequiresStrongPasswordAssistance;
@@ -397,6 +398,7 @@ struct WKAutoCorrectionData {
 @property (nonatomic, readonly) CGPoint lastInteractionLocation;
 @property (nonatomic, readonly) BOOL isEditable;
 @property (nonatomic, readonly) BOOL shouldHideSelectionWhenScrolling;
+@property (nonatomic, readonly) BOOL shouldIgnoreKeyboardWillHideNotification;
 @property (nonatomic, readonly) const WebKit::InteractionInformationAtPosition& positionInformation;
 @property (nonatomic, readonly) const WebKit::WKAutoCorrectionData& autocorrectionData;
 @property (nonatomic, readonly) const WebKit::FocusedElementInformation& focusedElementInformation;
index 359bd6e..470b9fb 100644 (file)
@@ -5159,6 +5159,20 @@ static bool mayContainSelectableText(WebKit::InputType type)
     }
 }
 
+static bool shouldShowKeyboardForElement(const WebKit::FocusedElementInformation& information)
+{
+    if (information.inputMode == WebCore::InputMode::None)
+        return false;
+
+    if (information.elementType == WebKit::InputType::Drawing)
+        return false;
+
+    if (mayContainSelectableText(information.elementType))
+        return true;
+
+    return !currentUserInterfaceIdiomIsPad();
+}
+
 static WebCore::FloatRect rectToRevealWhenZoomingToFocusedElement(const WebKit::FocusedElementInformation& elementInfo, const WebKit::EditorState& editorState)
 {
     WebCore::IntRect elementInteractionRect;
@@ -5205,6 +5219,8 @@ static RetainPtr<NSObject <WKFormPeripheral>> createInputPeripheralWithView(WebK
 - (void)_elementDidFocus:(const WebKit::FocusedElementInformation&)information userIsInteracting:(BOOL)userIsInteracting blurPreviousNode:(BOOL)blurPreviousNode activityStateChanges:(OptionSet<WebCore::ActivityState::Flag>)activityStateChanges userObject:(NSObject <NSSecureCoding> *)userObject
 {
     SetForScope<BOOL> isChangingFocusForScope { _isChangingFocus, hasFocusedElement(_focusedElementInformation) };
+    SetForScope<BOOL> isFocusingElementWithKeyboardForScope { _isFocusingElementWithKeyboard, shouldShowKeyboardForElement(information) };
+
     auto inputViewUpdateDeferrer = std::exchange(_inputViewUpdateDeferrer, nullptr);
 
     _didAccessoryTabInitiateFocus = _isChangingFocusUsingAccessoryTab;
@@ -5400,6 +5416,20 @@ static RetainPtr<NSObject <WKFormPeripheral>> createInputPeripheralWithView(WebK
         _didAccessoryTabInitiateFocus = NO;
 }
 
+- (BOOL)shouldIgnoreKeyboardWillHideNotification
+{
+    // Ignore keyboard will hide notifications sent during rotation. They're just there for
+    // backwards compatibility reasons and processing the will hide notification would
+    // temporarily screw up the unobscured view area.
+    if (UIPeripheralHost.sharedInstance.rotationState)
+        return YES;
+
+    if (_isChangingFocus && _isFocusingElementWithKeyboard)
+        return YES;
+
+    return NO;
+}
+
 - (void)_hardwareKeyboardAvailabilityChanged
 {
 #if USE(UIKIT_KEYBOARD_ADDITIONS)