Skip to content

Commit

Permalink
Cherry-pick 184aa40. rdar://120887424
Browse files Browse the repository at this point in the history
    [iOS] [scroll-anchoring] Software keyboard overlaps input field on webauthn.io
    https://bugs.webkit.org/show_bug.cgi?id=267440
    rdar://120753568

    Reviewed by Simon Fraser.

    It's possible for scroll anchoring to cause the scroll position to jump to the previously anchored
    element, when scrolling to reveal the focused element on top of the software keyboard on iOS. This
    scrolling (driven by `-_zoomToCenter:atScale:animated:` in the UI process) is bookended in Safari by
    calls to `-_(begin|end)InteractiveObscuredInsetsChange` while the keyboard is showing, which puts us
    in unstable state and also passes `ViewportRectStability::ChangingObscuredInsetsInteractively` into
    `AsyncScrollingCoordinator::reconcileScrollingState`. This, in turn, means we'll skip setting the
    layout viewport override rect when syncing the scroll position.

    This isn't normally an issue, because we'll eventually update the layout viewport once we end
    interactive obscured inset changes. However, in the context of scroll anchoring, the fact that the
    scroll position is updated but the layout viewport rect isn't (when reconciling scrolling state)
    means that it's possible for the scroll anchoring controller to adjust the layout viewport rect in
    the following codepath during layout:

    ```
    void LocalFrameView::updateLayoutViewport()
    {
        …
        if (m_layoutViewportOverrideRect) {
            if (currentScrollType() == ScrollType::Programmatic) {
                LOG_WITH_STREAM(Scrolling, stream << "computing new override layout viewport because of programmatic scrolling");
                LayoutPoint newOrigin = computeLayoutViewportOrigin(visualViewportRect(), minStableLayoutViewportOrigin(), maxStableLayoutViewportOrigin(), layoutViewport, StickToDocumentBounds);
                setLayoutViewportOverrideRect(LayoutRect(newOrigin, m_layoutViewportOverrideRect.value().size()));
            }
            layoutOrVisualViewportChanged();
            return;
        }
    ```

    ..._without_ having properly invalidated and updated the current scroll anchor. In turn, this makes
    it possible for us to compare the layout viewport after accounting for the keyboard scrolling amount
    against a stale `m_lastOffsetForAnchorElement`, causing the adjustment to overcompensate and jump
    back up to the top of the page in the middle of the keyboard scrolling animation.

    To fix this, we simply invalidate the anchor element on `ScrollAnchoringController` (along with the
    stale `m_lastOffsetForAnchorElement`) when the layout viewport changes.

    * LayoutTests/fast/scrolling/ios/scroll-anchoring-momentum-scroll.html:

    Drive-by fix: indentation.

    * LayoutTests/fast/scrolling/ios/scroll-anchoring-when-revealing-focused-element-expected.txt: Added.
    * LayoutTests/fast/scrolling/ios/scroll-anchoring-when-revealing-focused-element.html: Added.

    Add a new layout test that fails without this fix. While it doesn't precisely simulate the bug as it
    appears on webauthn.io, it exercises the same root cause by repeatedly triggering scroll anchoring
    adjustment while scrolling to reveal the focused element with the software keyboard, all in the
    scope of interactive obscured inset changes.

    * LayoutTests/resources/ui-helper.js:
    (window.UIHelper.beginInteractiveObscuredInsetsChange):
    (window.UIHelper.endInteractiveObscuredInsetsChange):
    (window.UIHelper.setObscuredInsets):

    Add new `UIHelper` methods to allow layout tests to adjust the obscured insets in the same way that
    Safari does when showing the software keyboard.

    * Source/WebCore/page/LocalFrameView.cpp:
    (WebCore::LocalFrameView::setLayoutViewportOverrideRect):

    See comments above for more detail.

    * Source/WebKit/UIProcess/API/ios/WKWebViewPrivateForTestingIOS.h:
    * Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:
    (-[WKWebView _resetObscuredInsetsForTesting]):

    Add a testing hook to reset the web view's obscured insets; used when resetting state in between
    layout tests.

    * Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
    * Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:
    (WTR::UIScriptController::beginInteractiveObscuredInsetsChange):
    (WTR::UIScriptController::endInteractiveObscuredInsetsChange):
    (WTR::UIScriptController::setObscuredInsets):

    Add boilerplate code to implement the new script controller helper methods.

    * Tools/WebKitTestRunner/ios/TestControllerIOS.mm:
    (WTR::TestController::platformResetStateToConsistentValues):
    * Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h:
    * Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:
    (WTR::UIScriptControllerIOS::beginInteractiveObscuredInsetsChange):
    (WTR::UIScriptControllerIOS::setObscuredInsets):
    (WTR::UIScriptControllerIOS::endInteractiveObscuredInsetsChange):

    Canonical link: https://commits.webkit.org/272957@main

Identifier: 270272.1644@safari-7619.0.1-branch
  • Loading branch information
MyahCobbs committed Jan 12, 2024
1 parent 078f394 commit 7d1ede9
Show file tree
Hide file tree
Showing 12 changed files with 176 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
document.querySelector("#block1").style.height = "200px";
await UIHelper.waitForTargetScrollAnimationToSettle(document.scrollingElement);
shouldBeGreaterThan("document.scrollingElement.scrollTop", "900");
finishJSTest();
finishJSTest();
}
</script>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Verifies that we scroll to avoid obscuring the focused element with the software keyboard, when scroll anchoring is enabled. To run the test manually, focus the text field without a hardware keyboard attached and check that the text field is visible

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

PASS distanceToTopOfKeyboard is > distanceToBottomOfTextField
PASS successfullyParsed is true

TEST COMPLETE
Scroll Anchor Element

Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true focusStartsInputSessionPolicy=allow ] -->
<html>
<head>
<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
<style>
body, html {
width: 100%;
margin: 0;
}

body {
height: 5000px;
}

input {
font-size: 18px;
position: absolute;
width: 100%;
top: 85vh;
overflow-anchor: none;
}

#content-above-anchor {
width: 100%;
}

#scroll-anchor {
width: 100%;
height: 100px;
font-weight: bold;
line-height: 100px;
color: white;
background: tomato;
text-align: center;
}

#description, #console {
overflow-anchor: none;
position: absolute;
top: 0;
}
</style>
<script src="../../../resources/js-test.js"></script>
<script src="../../../resources/ui-helper.js"></script>
</head>
<body>
<p id="description"></p>
<p id="console"></p>
<div id="content-above-anchor"></div>
<div id="scroll-anchor">Scroll Anchor Element</div>
<input placeholder="Focus me" />
<script>
jsTestIsAsync = true;
finalContentAboveAnchorHeight = 25;
const contentAboveAnchor = document.getElementById("content-above-anchor");

addEventListener("load", async () => {
description("Verifies that we scroll to avoid obscuring the focused element with the software keyboard, when scroll anchoring is enabled. To run the test manually, focus the text field without a hardware keyboard attached and check that the text field is visible");

let textField = document.querySelector("input");
textField.addEventListener("focus", async function() {
while (true) {
const currentHeight = parseInt(getComputedStyle(contentAboveAnchor).height);
contentAboveAnchor.style.height = `${currentHeight + 1}px`;
if (currentHeight >= finalContentAboveAnchorHeight)
return;
await UIHelper.renderingUpdate();
}
}, { once: true });

document.scrollingElement.scrollTo(0, 10);
if (!window.testRunner)
return;

await UIHelper.setObscuredInsets(59, 0, 134, 0);
await UIHelper.setHardwareKeyboardAttached(false);
await UIHelper.beginInteractiveObscuredInsetsChange();

textField.focus();

await UIHelper.waitForKeyboardToShow();
await UIHelper.endInteractiveObscuredInsetsChange();

distanceToTopOfKeyboard = innerHeight - (await UIHelper.inputViewBounds()).height;
distanceToBottomOfTextField = textField.offsetTop + textField.offsetHeight - pageYOffset;
shouldBeGreaterThan("distanceToTopOfKeyboard", "distanceToBottomOfTextField");

textField.blur();
await UIHelper.waitForKeyboardToHide();

finishJSTest();
});
</script>
</body>
</html>
30 changes: 30 additions & 0 deletions LayoutTests/resources/ui-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -1670,6 +1670,36 @@ window.UIHelper = class UIHelper {
});
}

static beginInteractiveObscuredInsetsChange()
{
if (!this.isWebKit2() || !this.isIOSFamily())
return Promise.resolve();

return new Promise(resolve => {
testRunner.runUIScript("uiController.beginInteractiveObscuredInsetsChange()", resolve);
});
}

static endInteractiveObscuredInsetsChange()
{
if (!this.isWebKit2() || !this.isIOSFamily())
return Promise.resolve();

return new Promise(resolve => {
testRunner.runUIScript("uiController.endInteractiveObscuredInsetsChange()", resolve);
});
}

static setObscuredInsets(top, right, bottom, left)
{
if (!this.isWebKit2() || !this.isIOSFamily())
return Promise.resolve();

return new Promise(resolve => {
testRunner.runUIScript(`uiController.setObscuredInsets(${top}, ${right}, ${bottom}, ${left})`, resolve);
});
}

static rotateDevice(orientationName, animatedResize = false)
{
if (!this.isWebKit2() || !this.isIOSFamily())
Expand Down
5 changes: 5 additions & 0 deletions Source/WebCore/page/LocalFrameView.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1740,6 +1740,11 @@ void LocalFrameView::setLayoutViewportOverrideRect(std::optional<LayoutRect> rec
m_layoutViewportOverrideRect = rect;
LayoutRect newRect = layoutViewportRect();

if (oldRect != newRect) {
invalidateScrollAnchoringElement();
updateScrollAnchoringElement();
}

// Triggering layout on height changes is necessary to make bottom-fixed elements behave correctly.
if (oldRect.height() != newRect.height())
layoutTriggering = TriggerLayoutOrNot::Yes;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@

- (void)_setDeviceHasAGXCompilerServiceForTesting;

- (void)_resetObscuredInsetsForTesting;
- (BOOL)_hasResizeAssertion;
- (void)_simulateSelectionStart;

Expand Down
6 changes: 6 additions & 0 deletions Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,12 @@ - (void)_setDeviceHasAGXCompilerServiceForTesting
_page->setDeviceHasAGXCompilerServiceForTesting();
}

- (void)_resetObscuredInsetsForTesting
{
if (self._haveSetObscuredInsets)
[self _resetObscuredInsets];
}

- (BOOL)_hasResizeAssertion
{
#if HAVE(UIKIT_RESIZABLE_WINDOWS)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,10 @@ interface UIScriptController {

undefined setScrollViewKeyboardAvoidanceEnabled(boolean enabled);

undefined beginInteractiveObscuredInsetsChange();
undefined endInteractiveObscuredInsetsChange();
undefined setObscuredInsets(double top, double right, double bottom, double left);

undefined becomeFirstResponder();
undefined resignFirstResponder();
readonly attribute boolean isPresentingModally;
Expand Down
4 changes: 4 additions & 0 deletions Tools/TestRunnerShared/UIScriptContext/UIScriptController.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ class UIScriptController : public JSWrappable {

virtual void setSafeAreaInsets(double, double, double, double) { notImplemented(); }

virtual void beginInteractiveObscuredInsetsChange() { notImplemented(); }
virtual void endInteractiveObscuredInsetsChange() { notImplemented(); }
virtual void setObscuredInsets(double, double, double, double) { notImplemented(); }

// View Parenting and Visibility

virtual void becomeFirstResponder() { notImplemented(); }
Expand Down
1 change: 1 addition & 0 deletions Tools/WebKitTestRunner/ios/TestControllerIOS.mm
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ static _WKFocusStartsInputSessionPolicy focusStartsInputSessionPolicy(const Test
webView.usesSafariLikeRotation = NO;
webView.overrideSafeAreaInsets = UIEdgeInsetsZero;
[webView _clearOverrideLayoutParameters];
[webView _resetObscuredInsetsForTesting];
[webView _clearInterfaceOrientationOverride];
[webView setAllowedMenuActions:nil];
webView._dragInteractionPolicy = dragInteractionPolicy(options);
Expand Down
4 changes: 4 additions & 0 deletions Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ class UIScriptControllerIOS final : public UIScriptControllerCocoa {
void setDidEndScrollingCallback(JSValueRef) override;
void clearAllCallbacks() override;

void beginInteractiveObscuredInsetsChange() final;
void endInteractiveObscuredInsetsChange() final;
void setObscuredInsets(double top, double right, double bottom, double left) final;

bool suppressSoftwareKeyboard() const final;
void setSuppressSoftwareKeyboard(bool) final;

Expand Down
15 changes: 15 additions & 0 deletions Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1315,6 +1315,21 @@ static UIDeviceOrientation toUIDeviceOrientation(DeviceOrientation orientation)
webView().overrideSafeAreaInsets = insets;
}

void UIScriptControllerIOS::beginInteractiveObscuredInsetsChange()
{
[webView() _beginInteractiveObscuredInsetsChange];
}

void UIScriptControllerIOS::setObscuredInsets(double top, double right, double bottom, double left)
{
webView()._obscuredInsets = UIEdgeInsetsMake(top, left, bottom, right);
}

void UIScriptControllerIOS::endInteractiveObscuredInsetsChange()
{
[webView() _endInteractiveObscuredInsetsChange];
}

void UIScriptControllerIOS::beginBackSwipe(JSValueRef callback)
{
[webView() _beginBackSwipeForTesting];
Expand Down

0 comments on commit 7d1ede9

Please sign in to comment.