Bug 181510 – Implement MultiDocument protocol for restoring focus to a WKWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Implement MultiDocument protocol for restoring focus to a WKWebView

[Megan Gardner]

Created attachment 331001 [details]
Patch

[Megan Gardner]

<rdar://problem/35468814>

[Tim Horton]

Comment on attachment 331001 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.messages.in:193
> +    RestoreFocusCallback(bool didRestore, WebKit::CallbackID callbackID)

You could just piggy-back off of UnsignedCallback, but this is fine. Either way.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2389
> +    m_page->setFocus(true);

Can’t you just grab m_page->focusController() and setFocused() on it? And then not add setFocus to page? No need to add things to Page just to turn around and call things on FocusController

[Wenson Hsieh]

Comment on attachment 331001 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.messages.in:193
>> +    RestoreFocusCallback(bool didRestore, WebKit::CallbackID callbackID)
> 
> You could just piggy-back off of UnsignedCallback, but this is fine. Either way.

IMO, adding a BooleanCallback doesn't seem unreasonable, and I can see future IPC mechanisms using it as well. Not a strong opinion though.

Also IMO, using UnsignedCallback here wouldn't make it as clear that the response should only be one of two values (false or true).

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:245
> +@interface WKContentView (WKInteraction) <UIGestureRecognizerDelegate, UIWebTouchEventsGestureRecognizerDelegate, UITextInputPrivate, UIWebFormAccessoryDelegate, UIWKInteractionViewProtocol, WKFileUploadPanelDelegate, WKActionSheetAssistantDelegate, UITextAutoscrolling, UITextInputMultiDocument

I think we still build against iOS 11, so this probably needs a declaration in UIKitSPI (even when using the internal SDK). Or you could make this protocol conformance conditional on iOS version.

[Tim Horton]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 331001 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331001&action=review
> 
> >> Source/WebKit/UIProcess/WebPageProxy.messages.in:193
> >> +    RestoreFocusCallback(bool didRestore, WebKit::CallbackID callbackID)
> > 
> > You could just piggy-back off of UnsignedCallback, but this is fine. Either way.
> 
> IMO, adding a BooleanCallback doesn't seem unreasonable, and I can see
> future IPC mechanisms using it as well. Not a strong opinion though.
> 
> Also IMO, using UnsignedCallback here wouldn't make it as clear that the
> response should only be one of two values (false or true).

Sure! I’d rather BooleanCallback over RestoreFocusCallback.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:245
> > +@interface WKContentView (WKInteraction) <UIGestureRecognizerDelegate, UIWebTouchEventsGestureRecognizerDelegate, UITextInputPrivate, UIWebFormAccessoryDelegate, UIWKInteractionViewProtocol, WKFileUploadPanelDelegate, WKActionSheetAssistantDelegate, UITextAutoscrolling, UITextInputMultiDocument
> 
> I think we still build against iOS 11, so this probably needs a declaration
> in UIKitSPI (even when using the internal SDK). Or you could make this
> protocol conformance conditional on iOS version.

Yes (but why is EWS green)?

[Wenson Hsieh]

(In reply to Tim Horton from comment #5)
> (In reply to Wenson Hsieh from comment #4)
> > Comment on attachment 331001 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=331001&action=review
> > 
> > >> Source/WebKit/UIProcess/WebPageProxy.messages.in:193
> > >> +    RestoreFocusCallback(bool didRestore, WebKit::CallbackID callbackID)
> > > 
> > > You could just piggy-back off of UnsignedCallback, but this is fine. Either way.
> > 
> > IMO, adding a BooleanCallback doesn't seem unreasonable, and I can see
> > future IPC mechanisms using it as well. Not a strong opinion though.
> > 
> > Also IMO, using UnsignedCallback here wouldn't make it as clear that the
> > response should only be one of two values (false or true).
> 
> Sure! I’d rather BooleanCallback over RestoreFocusCallback.
> 
> > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:245
> > > +@interface WKContentView (WKInteraction) <UIGestureRecognizerDelegate, UIWebTouchEventsGestureRecognizerDelegate, UITextInputPrivate, UIWebFormAccessoryDelegate, UIWKInteractionViewProtocol, WKFileUploadPanelDelegate, WKActionSheetAssistantDelegate, UITextAutoscrolling, UITextInputMultiDocument
> > 
> > I think we still build against iOS 11, so this probably needs a declaration
> > in UIKitSPI (even when using the internal SDK). Or you could make this
> > protocol conformance conditional on iOS version.
> 
> Yes (but why is EWS green)?

Ah, I didn't see that it's in the IPI section in UIKitSPI.h. This'll definitely work.

[Megan Gardner]

Created attachment 331008 [details]
Patch

How does this use case relate to the existing -[WKWebView _retainActiveFocusedState] mechanism?

In particular, is it desirable to leave the DOM exposed to the loss of focus in the first place?

(In reply to mitz from comment #8)
> How does this use case relate to the existing -[WKWebView
> _retainActiveFocusedState] mechanism?

It does seem like WKWebView could conform more correctly to the protocol (by responding correctly to the “preserve” side) using -_retainActiveFocusedState.

And perhaps if you do that, then current callers of -_retainActiveFocusedState could switch off of it and use the protocol methods.

[Megan Gardner]

Looking at _retainActiveFocusedState, this is incongruent with the purpose of this protocol. We must lose first-responderness and allow another view to become first responder, so using _retainActiveFocusedState breaks the other half of this feature. It did look like a promising idea, though, so that's for pointing it out!

(In reply to Megan Gardner from comment #11)
> Looking at _retainActiveFocusedState, this is incongruent with the purpose
> of this protocol. We must lose first-responderness and allow another view to
> become first responder, so using _retainActiveFocusedState breaks the other
> half of this feature. It did look like a promising idea, though, so that's
> for pointing it out!

-_retainActiveFocusedState is intended (and used) exactly for cases in which the web view resigns first responder (e.g. when using the camera to capture a credit card in Safari), so I am not sure I understand your comment.

[Megan Gardner]

Created attachment 331126 [details]
Patch

Comment on attachment 331126 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        Support the UIKit protocol for restoring focus to a what previously had focus.
> +        WebKit already knows what node was previously being focused by the DOM, we merely
> +        need to be asked to turn the focus on again.

Not sure how closely the wording here matches what you ended up doing, but I don’t have great ideas on how to word it. I think the gist is that when asked to preserve focus, you disconnect the DOM’s notion of focus from the web view’s notion of focus (i.e. first responderness), so when asked to restore, you don’t need to focus anything. Or something to that effect.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:245
> +@interface WKContentView (WKInteraction) <UIGestureRecognizerDelegate, UIWebTouchEventsGestureRecognizerDelegate, UITextInputPrivate, UIWebFormAccessoryDelegate, UIWKInteractionViewProtocol, WKFileUploadPanelDelegate, WKActionSheetAssistantDelegate, UITextAutoscrolling, UITextInputMultiDocument

Is this a good opportunity to sort the list of protocols?

[EWS Watchlist]

Comment on attachment 331126 [details]
Patch

Attachment 331126 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6042212

New failing tests:
fast/history/page-cache-suspended-audiocontext.html

[EWS Watchlist]

Created attachment 331143 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Megan Gardner]

https://trac.webkit.org/r226826

[Radar WebKit Bug Importer]

<rdar://problem/36455565>

[Ryan Haddad]

Reverted r226826 for reason:

Breaks internal builds.

Committed r226903: <https://trac.webkit.org/changeset/226903>

[Wenson Hsieh]

> > > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:245
> > > > +@interface WKContentView (WKInteraction) <UIGestureRecognizerDelegate, UIWebTouchEventsGestureRecognizerDelegate, UITextInputPrivate, UIWebFormAccessoryDelegate, UIWKInteractionViewProtocol, WKFileUploadPanelDelegate, WKActionSheetAssistantDelegate, UITextAutoscrolling, UITextInputMultiDocument
> > > 
> > > I think we still build against iOS 11, so this probably needs a declaration
> > > in UIKitSPI (even when using the internal SDK). Or you could make this
> > > protocol conformance conditional on iOS version.
> > 
> > Yes (but why is EWS green)?
> 
> Ah, I didn't see that it's in the IPI section in UIKitSPI.h. This'll
> definitely work.

On closer inspection, this protocol declaration is not, in fact, in the IPI section in UIKitSPI.h, which would explain why our internal builders still on iOS 11 are having a bad time.

We just need to move this to IPI.

[Megan Gardner]

https://trac.webkit.org/r226911

[Megan Gardner]

Did include incorrectly
https://trac.webkit.org/r226912

It creates a problem for us mentioned https://bugs.webkit.org/show_bug.cgi?id=187843.I guess you guys keep something in navigationcontroller strongly. if we remove a webview when it is not on the top of navigationcontroller's viewcontrollers. when back to the 'webviewpage(had removed)', a crash will happen low frequently.