Bug 220664 - [iOS] Emoji keyboard covers text field on twitter.com/messages
Summary: [iOS] Emoji keyboard covers text field on twitter.com/messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-15 12:27 PST by Wenson Hsieh
Modified: 2021-01-15 15:57 PST (History)
5 users (show)

See Also:


Attachments
Patch (21.94 KB, patch)
2021-01-15 13:01 PST, Wenson Hsieh
hi: review+
Details | Formatted Diff | Diff
Patch (21.93 KB, patch)
2021-01-15 14:45 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-01-15 12:27:55 PST
<rdar://problem/68400471>
Comment 1 Wenson Hsieh 2021-01-15 13:01:17 PST
Created attachment 417724 [details]
Patch
Comment 2 Devin Rousso 2021-01-15 13:28:49 PST
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 3 Wenson Hsieh 2021-01-15 13:33:49 PST
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!
Comment 4 Wenson Hsieh 2021-01-15 14:45:17 PST
Created attachment 417742 [details]
Patch
Comment 5 EWS 2021-01-15 15:57:24 PST
Committed r271543: <https://trac.webkit.org/changeset/271543>

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