Bug 150634 – Inner height behavior when the keyboard is shown should match on WKWebView and MobileSafari

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

Inner height behavior when the keyboard is shown should match on WKWebView and MobileSafari. More specifically, WKWebView should match MobileSafari's behavior by not sending resize events and changing window.innerHeight when bring the keyboard up or down.

[Wenson Hsieh]

<rdar://problem/23202254>

[Wenson Hsieh]

Created attachment 264244 [details]
Patch

[Wenson Hsieh]

Created attachment 264246 [details]
Patch

[Wenson Hsieh]

Comment on attachment 264246 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264246&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1693
> +        if (_currentlyAdjustingScrollViewInsetsForKeyboard)

On second thought, it doesn't look like calling _updateVisibleContentRects from within _adjustForAutomaticKeyboardInfo is intended in the first place, especially since we call _updateVisibleContentRects after adjusting for the keyboard anyways. I think it would make more sense to simply return early from _updateVisibleContentRects when _currentlyAdjustingScrollViewInsetsForKeyboard is YES. This also means we don't have to store the old bottom inset before adjustment, which is nice.

[Wenson Hsieh]

(In reply to comment #4)
> Comment on attachment 264246 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264246&action=review
> 
> ...especially since we call _updateVisibleContentRects after adjusting for the
> keyboard anyways...

My bad, we currently call _updateVisibleContentRects before adjusting for the keyboard. I propose calling _updateVisibleContentRects *after* adjusting for the keyboard, and adding the early return to _updateVisibleContentRects so we don't call it twice while calling _adjustForAutomaticKeyboardInfo.

[Wenson Hsieh]

Created attachment 264333 [details]
Patch

[Wenson Hsieh]

*** Bug 150401 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

Comment on attachment 264333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264333&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1680
> +        || [_scrollView isZoomBouncing]
> +        || _currentlyAdjustingScrollViewInsetsForKeyboard)

These seems similar to the 'isStableState' things below. Do you have to early return here, or can you just treat them as non-stable state?

[Wenson Hsieh]

Comment on attachment 264333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264333&action=review

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1680
>> +        || _currentlyAdjustingScrollViewInsetsForKeyboard)
> 
> These seems similar to the 'isStableState' things below. Do you have to early return here, or can you just treat them as non-stable state?

The early return involving _currentlyAdjustingScrollViewInsetsForKeyboard is quite specific to when we're adjusting the insets for the keyboard changing frame. This if statement was originally three separate early returns, and instead of adding a fourth early return I changed it to be a single if. The early return checks were all added in separate commits, and it doesn't look like they're related to the notion of a stable state.

[Benjamin Poulain]

Comment on attachment 264333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264333&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:215
> +    CGFloat _totalScrollViewBottomInsetAdjustment;

The name is a bit generic.

I think I would prefer a name mentioning the keyboard + a comment explaining what the attribute/ivar is for.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1686
> +    UIEdgeInsets computedContentInset = [self _computedContentInset];

IMHO, you need to make an argument about why "_totalScrollViewBottomInsetAdjustment" is adjusted here specifically and not in -_computedContentInset.
Such argument can go in the changelog.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1687
> +    // <rdar://problem/23202254> Adjust the bottom inset to not take the keyboard adjustments into account if the obscured insets were not explicitly set through SPI.

Comments like this are less valuable than what they look like. You have to read the radar to know the context.

A complete explanation is better IMHO. You can have such explanation with _totalScrollViewBottomInsetAdjustment and/or in the changelog.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1766
> +        _currentlyAdjustingScrollViewInsetsForKeyboard = YES;

You can use WTF's TemporaryChange to protect against reentrancy.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1770
> +        if (bottomInsetBeforeAdjustment != [_scrollView contentInset].bottom)

Let's have a temporary for [_scrollView contentInset].bottom to avoid making the call twice for the next line.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1771
> +            _totalScrollViewBottomInsetAdjustment += [_scrollView contentInset].bottom - bottomInsetBeforeAdjustment;

I am a bit confused by this, wouldn't "_totalScrollViewBottomInsetAdjustment" grow every time the keyboard show up?

[Wenson Hsieh]

Comment on attachment 264333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264333&action=review

Thanks for taking a look! I'll address the issues with an updated patch soon.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:215
>> +    CGFloat _totalScrollViewBottomInsetAdjustment;
> 
> The name is a bit generic.
> 
> I think I would prefer a name mentioning the keyboard + a comment explaining what the attribute/ivar is for.

Sounds good. I'll rename and add more description (it will be in the changelog)

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1686
>> +    UIEdgeInsets computedContentInset = [self _computedContentInset];
> 
> IMHO, you need to make an argument about why "_totalScrollViewBottomInsetAdjustment" is adjusted here specifically and not in -_computedContentInset.
> Such argument can go in the changelog.

Got it. I'll add more detail the changelog explaining why/how this fixes the problem.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1687
>> +    // <rdar://problem/23202254> Adjust the bottom inset to not take the keyboard adjustments into account if the obscured insets were not explicitly set through SPI.
> 
> Comments like this are less valuable than what they look like. You have to read the radar to know the context.
> 
> A complete explanation is better IMHO. You can have such explanation with _totalScrollViewBottomInsetAdjustment and/or in the changelog.

Sounds good. I'll include a thorough explanation of how this works in the changelog (and maybe a "see changelog for more details" here).

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1766
>> +        _currentlyAdjustingScrollViewInsetsForKeyboard = YES;
> 
> You can use WTF's TemporaryChange to protect against reentrancy.

Got it -- changed.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1770
>> +        if (bottomInsetBeforeAdjustment != [_scrollView contentInset].bottom)
> 
> Let's have a temporary for [_scrollView contentInset].bottom to avoid making the call twice for the next line.

Sounds good.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1771
>> +            _totalScrollViewBottomInsetAdjustment += [_scrollView contentInset].bottom - bottomInsetBeforeAdjustment;
> 
> I am a bit confused by this, wouldn't "_totalScrollViewBottomInsetAdjustment" grow every time the keyboard show up?

It also shrinks when the keyboard hides.  However, I should double check that there's no way for the keyboard frame to be shown/dismissed without _keyboardDidChangeFrame or _keyboardWillChangeFrame being called.

[Wenson Hsieh]

Created attachment 264395 [details]
Patch

[Benjamin Poulain]

Comment on attachment 264395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264395&action=review

> Source/WebKit2/ChangeLog:19
> +        We perform this readjustment in _updateVisibleContentRects rather than in _computedContentInset since some users
> +        of _computedContentInset may still expect the bottom inset to account for the keyboard height. However, when
> +        updating visible content rects, we should not account for the keyboard height since we don't want the inner height
> +        to change when showing or hiding the keyboard.

Pretty unfortunate but that looks right. The method -_navigationGestureDidBegin expects the offset without keyboard for example :(

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1690
> +    UIEdgeInsets computedContentInset = [self _computedContentInset];

I think a better name would be useful.
The variable shares the name with -_computedContentInset but not the value.

[Wenson Hsieh]

Committed r191834: <http://trac.webkit.org/changeset/191834>