- Revision
- 247651
- Author
- wenson_hs...@apple.com
- Date
- 2019-07-19 12:55:20 -0700 (Fri, 19 Jul 2019)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (247650 => 247651)
--- trunk/LayoutTests/ChangeLog 2019-07-19 19:47:54 UTC (rev 247650)
+++ trunk/LayoutTests/ChangeLog 2019-07-19 19:55:20 UTC (rev 247651)
@@ -1,3 +1,17 @@
+2019-07-19 Wenson Hsieh <wenson_hs...@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 <grao...@apple.com>
Links stop working after long-pressing a link (WK1)
Added: trunk/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields-expected.txt (0 => 247651)
--- trunk/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields-expected.txt 2019-07-19 19:55:20 UTC (rev 247651)
@@ -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".
+
+
Added: trunk/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html (0 => 247651)
--- trunk/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html (rev 0)
+++ trunk/LayoutTests/fast/forms/ios/no-scrolling-when-moving-focus-between-adjacent-fields.html 2019-07-19 19:55:20 UTC (rev 247651)
@@ -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=""
+ <script src=""
+ <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>
Modified: trunk/Source/WebKit/ChangeLog (247650 => 247651)
--- trunk/Source/WebKit/ChangeLog 2019-07-19 19:47:54 UTC (rev 247650)
+++ trunk/Source/WebKit/ChangeLog 2019-07-19 19:55:20 UTC (rev 247651)
@@ -1,3 +1,49 @@
+2019-07-19 Wenson Hsieh <wenson_hs...@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 <achristen...@webkit.org>
Fix warning when importing WebKit in Swift
Modified: trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm (247650 => 247651)
--- trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm 2019-07-19 19:47:54 UTC (rev 247650)
+++ trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm 2019-07-19 19:55:20 UTC (rev 247651)
@@ -3329,6 +3329,9 @@
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 @@
- (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];
Modified: trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.h (247650 => 247651)
--- trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.h 2019-07-19 19:47:54 UTC (rev 247650)
+++ trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.h 2019-07-19 19:55:20 UTC (rev 247651)
@@ -345,6 +345,7 @@
BOOL _resigningFirstResponder;
BOOL _needsDeferredEndScrollingSelectionUpdate;
BOOL _isChangingFocus;
+ BOOL _isFocusingElementWithKeyboard;
BOOL _isBlurringFocusedElement;
BOOL _focusRequiresStrongPasswordAssistance;
@@ -397,6 +398,7 @@
@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;
Modified: trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm (247650 => 247651)
--- trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2019-07-19 19:47:54 UTC (rev 247650)
+++ trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2019-07-19 19:55:20 UTC (rev 247651)
@@ -5159,6 +5159,20 @@
}
}
+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 @@
- (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 @@
_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)
