Bug 220664

Created attachment 417724 [details]
Patch

Comment on attachment 417724 [details]
Patch

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

r=me, nice work!

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1193
> +    return !rect.isEmpty() && CGRectContainsRect([self _contentRectForUserInteraction], rect);

NIT: here you use `[self _contentRectForUserInteraction]` but below you use `self._contentRectForUserInteraction` 😅

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1196
> +- (void)_scrollToRevealSelectionIfNeeded

NIT: would `_scrollToAndRevealSelectionIfNeeded` be more clear/accurate?

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1208
> +    WebCore::FloatRect userInteractionContentRect = self._contentRectForUserInteraction;

ditto (:1193)

> LayoutTests/editing/selection/ios/scroll-to-reveal-selection-when-showing-software-keyboard.html:18
> +    let initialContentOffsetY = (await UIHelper.contentOffset()).y;

Style: yuck.  While there's nothing technically wrong with this, I personally recommend not using `await` like this so that it's explicitly clear what's being waited for.

> LayoutTests/editing/selection/ios/scroll-to-reveal-selection-when-showing-software-keyboard.html:22
> +    let contentOffsetY = initialContentOffsetY;

Aside: is it worth it (or even possible) to write a test for your horizontal scrolling logic too?  Maybe zoom in on the page and pan over before focusing an `<input>`?

> LayoutTests/editing/selection/ios/scroll-to-reveal-selection-when-showing-software-keyboard.html:25
> +        contentOffsetY = (await UIHelper.contentOffset()).y;

ditto (:18)

Comment on attachment 417724 [details]
Patch

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

Thanks for the review!

>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1193
>> +    return !rect.isEmpty() && CGRectContainsRect([self _contentRectForUserInteraction], rect);
> 
> NIT: here you use `[self _contentRectForUserInteraction]` but below you use `self._contentRectForUserInteraction` 😅

Whoops — I'll make this `self._contentRectForUserInteraction`.

>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1196
>> +- (void)_scrollToRevealSelectionIfNeeded
> 
> NIT: would `_scrollToAndRevealSelectionIfNeeded` be more clear/accurate?

Sounds good to me! Will rename.

>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1208
>> +    WebCore::FloatRect userInteractionContentRect = self._contentRectForUserInteraction;
> 
> ditto (:1193)

This version (dot notation for property access) is preferred.

>> LayoutTests/editing/selection/ios/scroll-to-reveal-selection-when-showing-software-keyboard.html:18
>> +    let initialContentOffsetY = (await UIHelper.contentOffset()).y;
> 
> Style: yuck.  While there's nothing technically wrong with this, I personally recommend not using `await` like this so that it's explicitly clear what's being waited for.

Sounds good! Changing to just `let initialContentOffset = await UIHelper.contentOffset();`, and then using `initialContentOffset.y` below.

>> LayoutTests/editing/selection/ios/scroll-to-reveal-selection-when-showing-software-keyboard.html:22
>> +    let contentOffsetY = initialContentOffsetY;
> 
> Aside: is it worth it (or even possible) to write a test for your horizontal scrolling logic too?  Maybe zoom in on the page and pan over before focusing an `<input>`?

Hmm…unfortunately, I'm not sure it's actually possible to write a test for horizontal scrolling, since it would require the software keyboard to grow horizontally (and overlap the selection as a result).

>> LayoutTests/editing/selection/ios/scroll-to-reveal-selection-when-showing-software-keyboard.html:25
>> +        contentOffsetY = (await UIHelper.contentOffset()).y;
> 
> ditto (:18)

Fixed!

Created attachment 417742 [details]
Patch

Committed r271543: <https://trac.webkit.org/changeset/271543>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417742 [details].