Bug 183154 – Make it possible to set suggestions in extra zoom mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Yongjun Zhang]

In extra zoom mode, a WebKit client should be able to set suggestions to the input view controller.

[Yongjun Zhang]

rdar://problem/35227450

[Yongjun Zhang]

Created attachment 334660 [details]
Patch.

[Wenson Hsieh]

Comment on attachment 334660 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=334660&action=review

> Source/WebKit/ChangeLog:11
> +        suggestions is updated. Later, when we present WKTextInputViewController, we can pass the cached

Nit - s/is/are/

> Source/WebKit/ChangeLog:15
> +        (-[WKContentView presentFocusedFormControlViewController:]): Set _viewControllerForFullScreenPresentationFromView as

Nit - I think you meant "Set _focusedFormControlViewController as the inputDelegate"?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4211
> +    [self setInputDelegate:_focusedFormControlViewController.get()];

On second thought, it's a bit weird for the focused form control VC to be a UITextInput (I thought this was limited to the WKTextInputViewController). The focused form control overlay doesn't really deal with text input, but rather, with moving between focusable form controls on a page. Can we make WKTextInputVC the UITextInput, and just -setDelegate when presenting/dismissing the text input VC?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4389
> +        [_textInputViewController setSuggestions:controller.suggestions];

Nit - We don't need this if statement here, since it'll just be a noop if _textInputViewController is nil.

[Yongjun Zhang]

Comment on attachment 334660 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=334660&action=review

>> Source/WebKit/ChangeLog:15
>> +        (-[WKContentView presentFocusedFormControlViewController:]): Set _viewControllerForFullScreenPresentationFromView as
> 
> Nit - I think you meant "Set _focusedFormControlViewController as the inputDelegate"?

Oh right.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4211
>> +    [self setInputDelegate:_focusedFormControlViewController.get()];
> 
> On second thought, it's a bit weird for the focused form control VC to be a UITextInput (I thought this was limited to the WKTextInputViewController). The focused form control overlay doesn't really deal with text input, but rather, with moving between focusable form controls on a page. Can we make WKTextInputVC the UITextInput, and just -setDelegate when presenting/dismissing the text input VC?

That was my first attempt, but it only works when the text input VC is brought up by [WKContentView _startAssistingNode:] (i.e., when tapping a text input element without the focused form control VC showing). If the focus form control VC is showing, and we tapping the input box, the text input VC will be brought up by [WKContentView focusedFormControlControllerDidBeginEditing:], which is too late to pick up the suggestions. I think the reason this happens is before moving between focusable text inputs, we dismiss the text input VC and nil it, then we move the focus to the next input and update the suggestions from _startAssistingNode:, but the input VC is now nil. That is why we need to somehow cache the suggestions while text input VC is nil and apply that when it is presented.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4389
>> +        [_textInputViewController setSuggestions:controller.suggestions];
> 
> Nit - We don't need this if statement here, since it'll just be a noop if _textInputViewController is nil.

Ok, will fix.

[Wenson Hsieh]

Comment on attachment 334660 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=334660&action=review

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4211
>>> +    [self setInputDelegate:_focusedFormControlViewController.get()];
>> 
>> On second thought, it's a bit weird for the focused form control VC to be a UITextInput (I thought this was limited to the WKTextInputViewController). The focused form control overlay doesn't really deal with text input, but rather, with moving between focusable form controls on a page. Can we make WKTextInputVC the UITextInput, and just -setDelegate when presenting/dismissing the text input VC?
> 
> That was my first attempt, but it only works when the text input VC is brought up by [WKContentView _startAssistingNode:] (i.e., when tapping a text input element without the focused form control VC showing). If the focus form control VC is showing, and we tapping the input box, the text input VC will be brought up by [WKContentView focusedFormControlControllerDidBeginEditing:], which is too late to pick up the suggestions. I think the reason this happens is before moving between focusable text inputs, we dismiss the text input VC and nil it, then we move the focus to the next input and update the suggestions from _startAssistingNode:, but the input VC is now nil. That is why we need to somehow cache the suggestions while text input VC is nil and apply that when it is presented.

I see! This makes a bit more sense now. It still feels a bit weird that the text suggestions need to be stored in two places (the focused form VC as well as the text input VC). I would still prefer just storing it on the focused form VC and then adding a delegate method to WKTextFormControlViewControllerDelegate so that the WKTextInputVC (a subclass of WKTextFormControlVC) can ask the content view for text suggestions, and the content view would then get text suggestions off of the focused form control VC.

This is probably fine for now, though. Also, WKTextInputVC is just a stop-gap solution, and we'll need to rewrite parts of this soon anyways, so we can always refactor this later.

r=me, but this will also require a WK2 owner.

[Yongjun Zhang]

Created attachment 334695 [details]
Addressing review comments.

[WebKit Commit Bot]

Comment on attachment 334695 [details]
Addressing review comments.

Clearing flags on attachment: 334695

Committed r229067: <https://trac.webkit.org/changeset/229067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.