Bug 210697

<rdar://problem/60997530>

The effect of this bug can be seen on a google.com using the following steps: 1. Visit google.com and search for "Disneyland" and scroll the page until the search field becomes fixed to the top of the page. 2. Use -_requestTextInputContextsInRect to get the context for the search field at the top of the page. 3. Call -_willBeginTextInteractionInTextInputContext with this context. 4. Call -_focusTextInputContext with this context. 5. Call -_didFinishTextInteractionInTextInputContext with this context. Then the page will scroll to the top because -_didFinishTextInteractionInTextInputContext marks the view as needing to reveal the focused element.

(In reply to Daniel Bates from comment #2) > The effect of this bug can be seen on a google.com using the following steps: > > 1. Visit google.com and search for "Disneyland" and scroll the page until > the search field becomes fixed to the top of the page. 1.5. Tap on the search field > 2. Use -_requestTextInputContextsInRect to get the context for the search > field at the top of the page. > 3. Call -_willBeginTextInteractionInTextInputContext with this context. > 4. Call -_focusTextInputContext with this context. > 5. Call -_didFinishTextInteractionInTextInputContext with this context. > > Then the page will scroll to the top because > -_didFinishTextInteractionInTextInputContext marks the view as needing to > reveal the focused element.

Created attachment 396866 [details] Patch and tests

This patch depends on the patch for bug #210624 due to placement of the added tests in the file.

Created attachment 396872 [details] Patch and tests

Comment on attachment 396872 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=396872&action=review > Source/WebKit/ChangeLog:26 > + making this a FIXME, but this function is otherwise capable of handling multiple innocations innocations => invocations > Source/WebKit/ChangeLog:31 > + do everything we do now except for marking the page as needing reveal the focused element Needing reveal => needing to reveal > Source/WebKit/ChangeLog:39 > + element. Passing true to visiblePositionInFocusedNodeForPoint() asserts this to be true. asserts => will ultimately assert

Comment on attachment 396872 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=396872&action=review > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5104 > + _textInteractionDidChangeFocusedElement |= NO; Was this intended to be `_textInteractionDidChangeFocusedElement = NO;`? If not, can we just remove this line? > Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:583 > + // Zoom scale of scroll view should change to reveal the focused element. I don’t believe this is true for iPad. Maybe we can only do the scale check if we’re on iPhone? > Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:605 > + // Zoom scale of scroll view should change to reveal the focused element. Ditto.

Comment on attachment 396872 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=396872&action=review Thanks for the review, Wenson. >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5104 >> + _textInteractionDidChangeFocusedElement |= NO; > > Was this intended to be `_textInteractionDidChangeFocusedElement = NO;`? If not, can we just remove this line? OK I'll remove this line. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:583 >> + // Zoom scale of scroll view should change to reveal the focused element. > > I don’t believe this is true for iPad. Maybe we can only do the scale check if we’re on iPhone? I'll check on iPad.

Created attachment 397109 [details] To Land

Created attachment 397110 [details] To Land

Comment on attachment 397110 [details] To Land Clearing flags on attachment: 397110 Committed r260449: <https://trac.webkit.org/changeset/260449>

All reviewed patches have been landed. Closing bug.

(In reply to Daniel Bates from comment #12) > Comment on attachment 397110 [details] > To Land > > Clearing flags on attachment: 397110 > > Committed r260449: <https://trac.webkit.org/changeset/260449> This change broke the iOS build: /Volumes/Data/slave/ios-13-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:550:5: error: unknown type name 'IPhoneUserInterfaceSwizzler'; did you mean 'TestWebKitAPI::IPhoneUserInterfaceSwizzler'? https://build.webkit.org/builders/Apple%20iOS%2013%20Release%20%28Build%29/builds/6599/steps/compile-webkit/logs/stdio

(In reply to Ryan Haddad from comment #14) > (In reply to Daniel Bates from comment #12) > > Comment on attachment 397110 [details] > > To Land > > > > Clearing flags on attachment: 397110 > > > > Committed r260449: <https://trac.webkit.org/changeset/260449> > This change broke the iOS build: > /Volumes/Data/slave/ios-13-release/build/Tools/TestWebKitAPI/Tests/ > WebKitCocoa/RequestTextInputContext.mm:550:5: error: unknown type name > 'IPhoneUserInterfaceSwizzler'; did you mean > 'TestWebKitAPI::IPhoneUserInterfaceSwizzler'? > > https://build.webkit.org/builders/Apple%20iOS%2013%20Release%20%28Build%29/ > builds/6599/steps/compile-webkit/logs/stdio Fixed in <https://trac.webkit.org/changeset/260469>