<rdar://problem/68400471>
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].