[webkit-reviews] review granted: [Bug 220664] [iOS] Emoji keyboard covers text field on twitter.com/messages : [Attachment 417724] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 220664: [iOS] Emoji keyboard covers text field on twitter.com/messages
https://bugs.webkit.org/show_bug.cgi?id=220664

Attachment 417724: Patch

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




--- Comment #2 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 417724
  --> https://bugs.webkit.org/attachment.cgi?id=417724
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-softw
are-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-softw
are-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-softw
are-keyboard.html:25
> +	   contentOffsetY = (await UIHelper.contentOffset()).y;

ditto (:18)