[iOS] Input field on ddg.gg is auto focused when url is entered with the software keyboard

• Component: WebKit Misc.
• Status: NEW ---

[Per Arne Vollan]

When the url 'ddg.gg' is entered with the software keyboard, the input field is auto selected, and the software keyboard reappears. This does not happen when picking the url from favorites.

[Wenson Hsieh]

I've seen this now and again as well. Just a wild stab...

• Safari preemptively begins loading URL while it is being typed.
• Script on the page focuses the field.
• User hits enter, and Safari makes the web view first responder.

The keyboard then appears, since we're becoming FR with a focused element.

[Per Arne Vollan]

Created attachment 367062 [details]
Patch

[Wenson Hsieh]

IIRC, this activity state change logic was added in <https://trac.webkit.org/changeset/226480/webkit> to (IIRC) support password AutoFill for apps on iOS.

[Wenson Hsieh]

Comment on attachment 367062 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4965
> +                if (changingActivityState && [UIKeyboard isInHardwareKeyboardMode])
> +                    return YES;
> +                

It seems this is equivalent to just moving the `changingActivityState` check above to the #if PLATFORM(WATCHOS) section (since we return YES anyways if [UIKeyboard isInHardwareKeyboardMode] is YES)

[Per Arne Vollan]

(In reply to Wenson Hsieh from comment #3)
> IIRC, this activity state change logic was added in
> <https://trac.webkit.org/changeset/226480/webkit> to (IIRC) support password
> AutoFill for apps on iOS.

Ah, I see. This patch will possibly break having focused restored when re-launching WKWebView. What activity state changes are relevant in this case?

[Per Arne Vollan]

Created attachment 367142 [details]
Patch

[Per Arne Vollan]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 367062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367062&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4965
> > +                if (changingActivityState && [UIKeyboard isInHardwareKeyboardMode])
> > +                    return YES;
> > +                
> 
> It seems this is equivalent to just moving the `changingActivityState` check
> above to the #if PLATFORM(WATCHOS) section (since we return YES anyways if
> [UIKeyboard isInHardwareKeyboardMode] is YES)

I updated the patch to send the changing activity flag bitfield to the UI process, and then use the bitfield to determine if the software keyboard should be shown. I believe this will avoid breaking the fix in https://trac.webkit.org/changeset/226480/webkit.

Thanks for reviewing!

[Jon Lee]

rdar://problem/48970509

[Megan Gardner]

Comment on attachment 367142 [details]
Patch

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

So, I added chagingActivityState originally to make it so that the software keyboard would not be dismissed if you backgrounded Safari, and then brought it forward again. That is the activityState that was changing. I have noted that this has problems when you background a page that has an auto-focused element that has not brought up the keyboard because there has been no user interaction. If you background that page, and bring it forward, the keyboard shows up. Sending the whole bitfield across seems like a good way to make sure that we're only showing the keyboard in the right cases, but at the risk of bikesheding, if you're going to change the type of this flag, please make the name more appropriate as well, Just 'activityState' should be good.

> Source/WebKit/UIProcess/WebPageProxy.h:348
>      RefPtr<API::Object> userData;

Please change to activityState, since this is no longer a bool.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4968
> +                if (changingActivityState != WebCore::ActivityState::IsFocused && changingActivityState)

I think this would read better if the order was switched.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1760
>  

Init this with something appropriate if possible.

[Per Arne Vollan]

(In reply to Megan Gardner from comment #9)
> Comment on attachment 367142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367142&action=review
> 
> So, I added chagingActivityState originally to make it so that the software
> keyboard would not be dismissed if you backgrounded Safari, and then brought
> it forward again. That is the activityState that was changing. I have noted
> that this has problems when you background a page that has an auto-focused
> element that has not brought up the keyboard because there has been no user
> interaction. If you background that page, and bring it forward, the keyboard
> shows up. Sending the whole bitfield across seems like a good way to make
> sure that we're only showing the keyboard in the right cases, but at the
> risk of bikesheding, if you're going to change the type of this flag, please
> make the name more appropriate as well, Just 'activityState' should be good.
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:348
> >      RefPtr<API::Object> userData;
> 
> Please change to activityState, since this is no longer a bool.
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4968
> > +                if (changingActivityState != WebCore::ActivityState::IsFocused && changingActivityState)
> 
> I think this would read better if the order was switched.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:1760
> >  
> 
> Init this with something appropriate if possible.

Thanks for reviewing! I will update the patch.

[Per Arne Vollan]

Created attachment 368046 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 368046 [details]
Patch

Clearing flags on attachment: 368046

Committed r244559: <https://trac.webkit.org/changeset/244559>