TestWebKitAPI.ScrollViewScrollabilityTests.ScrollableWithOverflowHiddenAndInputView Failed
Ran 1 tests of 1 with 0 successful
------------------------------
Test suite failed

Failed

    TestWebKitAPI.ScrollViewScrollabilityTests.ScrollableWithOverflowHiddenAndInputView
        LEAK: 1 WebProcessPool
        LEAK: 1 WebPageProxy

        /Users/whsieh/work/OpenSource/Tools/TestWebKitAPI/Tests/ios/ScrollViewScrollabilityTests.mm:100
        Expected equality of these values:
          [[webView scrollView] isScrollEnabled]
            Which is: false
          __objc_yes
            Which is: true

The way this API test currently works is a little unexpected. I think it’s intended to test that after focusing an input field (and bringing up the keyboard), we scroll to reveal the focused element even when the body has `overflow: hidden`. The logic responsible for making this work is in -[WKWebView _updateScrollViewForTransaction:]...

    bool hasDockedInputView = !CGRectIsEmpty(_inputViewBounds);
    bool isZoomed = layerTreeTransaction.pageScaleFactor() > layerTreeTransaction.initialScaleFactor();

    …

    bool scrollingEnabled = _page->scrollingCoordinatorProxy()->hasScrollableMainFrame() || hasDockedInputView || isZoomed || scrollingNeededToRevealUI;
    [_scrollView _setScrollEnabledInternal:scrollingEnabled];

However, since this is an API test on iOS (and therefore does not run in the context of a UIApplication), UIKit never attempts to actually bring up an input view; as a result, _inputViewBounds is always empty, so hasDockedInputView will always be NO. Instead, this test currently passes due to how -[WKWebView _zoomToFocusRect:…:] zooms the input field when running on iPhone; by zooming, the `isZoomed` flag is set, and so we end up setting scrollEnabled to YES anyways.

I manually verified on device (by focusing the bottom half of https://whsieh.github.io/examples/multiple-contenteditable) that we do still scroll to reveal focused elements after my change in r256072, since _inputViewBounds is non-empty.

This also explains the need for this snippet in the test:

    BOOL isPhone = [[UIDevice currentDevice] userInterfaceIdiom] == UIUserInterfaceIdiomPhone;
    if (isPhone)
        EXPECT_EQ([[webView scrollView] isScrollEnabled], YES);

Since we don’t zoom on iPad, the `isZoomed` flag is NO and we don’t end up making the scroll view scrollable. This also suggests that if we were to make TestWebKitAPI a proper UI application in the future, this workaround could be removed.

(In reply to Wenson Hsieh from comment #3)
> This also explains the need for this snippet in the test:
> 
>     BOOL isPhone = [[UIDevice currentDevice] userInterfaceIdiom] ==
> UIUserInterfaceIdiomPhone;
>     if (isPhone)
>         EXPECT_EQ([[webView scrollView] isScrollEnabled], YES);
> 
> Since we don’t zoom on iPad, the `isZoomed` flag is NO and we don’t end up
> making the scroll view scrollable. This also suggests that if we were to
> make TestWebKitAPI a proper UI application in the future, this workaround
> could be removed.

On closer inspection, maybe not, because the test also explicitly overrides the input view and input accessory views to be empty UIViews, which means that the _inputViewBounds would be empty anyways :/

    auto inputView = adoptNS([[UIView alloc] init]);
    auto inputAccessoryView = adoptNS([[UIView alloc] init]);
    auto inputDelegate = adoptNS([TestInputDelegate new]);
    [inputDelegate setWillStartInputSessionHandler:[inputView, inputAccessoryView] (WKWebView *, id<_WKFormInputSession> session) {
        session.customInputView = inputView.get();
        session.customInputAccessoryView = inputAccessoryView.get();
    }];

Created attachment 390167 [details]
Patch

Comment on attachment 390167 [details]
Patch

Thanks for the review!

Comment on attachment 390167 [details]
Patch

Clearing flags on attachment: 390167

Committed r256092: <https://trac.webkit.org/changeset/256092>

All reviewed patches have been landed.  Closing bug.