Summary: | [iOS] -_didFinishTextInteractionInTextInputContext should only zoom to reveal focused element if it changed | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||||
Component: | WebKit Misc. | Assignee: | Daniel Bates <dbates> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, esprehn+autocc, ews-watchlist, kangil.han, webkit-bug-importer, wenson_hsieh | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | iPhone / iPad | ||||||||||||
OS: | iOS 13 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 210704 | ||||||||||||
Attachments: |
|
Description
Daniel Bates
2020-04-18 14:04:29 PDT
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> |