Changeset 247651 in webkit

Timestamp:

Jul 19, 2019 12:55:20 PM (5 years ago)

Author:

Wenson Hsieh

Message:

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

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields-expected.txt (added)
• LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html (added)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm (modified) (2 diffs)
• Source/WebKit/UIProcess/ios/WKContentViewInteraction.h (modified) (2 diffs)
• Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

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

trunk/Source/WebKit/ChangeLog

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

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

@@ -3330 +3330 @@
         [_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;
@@ -3372 +3375 @@
 - (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;
 

trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.h

@@ -346 +346 @@
     BOOL _needsDeferredEndScrollingSelectionUpdate;
     BOOL _isChangingFocus;
+    BOOL _isFocusingElementWithKeyboard;
     BOOL _isBlurringFocusedElement;
 
@@ -398 +399 @@
 @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;

trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm

@@ -5160 +5160 @@
 }
 
+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)
 {
@@ -5206 +5220 @@
 {
     SetForScope<BOOL> isChangingFocusForScope { _isChangingFocus, hasFocusedElement(_focusedElementInformation) };
+    SetForScope<BOOL> isFocusingElementWithKeyboardForScope { _isFocusingElementWithKeyboard, shouldShowKeyboardForElement(information) };
+
     auto inputViewUpdateDeferrer = std::exchange(_inputViewUpdateDeferrer, nullptr);
 
@@ -5399 +5415 @@
     if (!_isChangingFocus)
         _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;
 }