Bug 210697 – [iOS] -_didFinishTextInteractionInTextInputContext should only zoom to reveal focused element if it changed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Bates]

-_didFinishTextInteractionInTextInputContext should only zoom to reveal the focused element if it changed via a call to -_focusTextInputContext.

[Daniel Bates]

<rdar://problem/60997530>

[Daniel Bates]

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.

[Daniel Bates]

(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.

[Daniel Bates]

Created attachment 396866 [details]
Patch and tests

[Daniel Bates]

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

[Daniel Bates]

Created attachment 396872 [details]
Patch and tests

[Daniel Bates]

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

[Wenson Hsieh]

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.

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 397109 [details]
To Land

[Daniel Bates]

Created attachment 397110 [details]
To Land

[Daniel Bates]

Comment on attachment 397110 [details]
To Land

Clearing flags on attachment: 397110

Committed r260449: <https://trac.webkit.org/changeset/260449>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(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

[Daniel Bates]

(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>