Bug 196740 - [iOS] Input field on ddg.gg is auto focused when url is entered with the software keyboard
Summary: [iOS] Input field on ddg.gg is auto focused when url is entered with the soft...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-09 12:08 PDT by Per Arne Vollan
Modified: 2019-04-23 13:27 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.24 KB, patch)
2019-04-09 12:32 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (12.78 KB, patch)
2019-04-10 11:10 PDT, Per Arne Vollan
megan_gardner: review+
Details | Formatted Diff | Diff
Patch (15.83 KB, patch)
2019-04-23 11:29 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-04-09 12:08:33 PDT
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.
Comment 1 Wenson Hsieh 2019-04-09 12:11:51 PDT
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.
Comment 2 Per Arne Vollan 2019-04-09 12:32:28 PDT
Created attachment 367062 [details]
Patch
Comment 3 Wenson Hsieh 2019-04-09 12:52:50 PDT
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.
Comment 4 Wenson Hsieh 2019-04-09 13:09:01 PDT
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)
Comment 5 Per Arne Vollan 2019-04-09 13:16:49 PDT
(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?
Comment 6 Per Arne Vollan 2019-04-10 11:10:51 PDT
Created attachment 367142 [details]
Patch
Comment 7 Per Arne Vollan 2019-04-10 11:15:23 PDT
(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!
Comment 8 Jon Lee 2019-04-22 16:23:31 PDT
rdar://problem/48970509
Comment 9 Megan Gardner 2019-04-22 16:43:33 PDT
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.
Comment 10 Per Arne Vollan 2019-04-22 16:50:52 PDT
(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.
Comment 11 Per Arne Vollan 2019-04-23 11:29:44 PDT
Created attachment 368046 [details]
Patch
Comment 12 WebKit Commit Bot 2019-04-23 13:27:30 PDT
Comment on attachment 368046 [details]
Patch

Clearing flags on attachment: 368046

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