Bug 143482 – Calling makeFirstResponder on WKWebView doesn't work

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

In the modern WebKit API, WKWebView has a subview WKView that becomes the first responder when the user clicks the view.
However, manually calling [NSWindow makeFirstResponder:] on WKWebView doesn't forward the first responder role to its WKView and therefore fails.

rdar://problem/20298166

[Enrica Casucci]

Created attachment 250271 [details]
Patch

[Enrica Casucci]

Created attachment 250274 [details]
Patch2

Fixes iOS build.

[Ryosuke Niwa]

Comment on attachment 250274 [details]
Patch2

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:474
> +    return [self superview] == nextResponder;

Perhaps we should store [self superview] into a local variable named webView so that the intent is clear?

Comment on attachment 250274 [details]
Patch2

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

>> Source/WebKit2/UIProcess/API/mac/WKView.mm:474
>> +    return [self superview] == nextResponder;
> 
> Perhaps we should store [self superview] into a local variable named webView so that the intent is clear?

Why are we checking for the superview and not our WKWebView?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:495
>      _data->_page->viewStateDidChange(ViewState::IsFocused);

Shouldn’t we also prevent other things happening in this method from happening if the loss of first responder is temporary? Specifically things like interrupting text input and sending DOM events related to focused state (if we do that)?

[Enrica Casucci]

(In reply to comment #4)
> Comment on attachment 250274 [details]
> Patch2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250274&action=review
> 
> >> Source/WebKit2/UIProcess/API/mac/WKView.mm:474
> >> +    return [self superview] == nextResponder;
> > 
> > Perhaps we should store [self superview] into a local variable named webView so that the intent is clear?
> 
> Why are we checking for the superview and not our WKWebView?
> 
How do I get from the WKView to the WKWebView? Anders suggested to use the superview.

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:495
> >      _data->_page->viewStateDidChange(ViewState::IsFocused);
> 
> Shouldn’t we also prevent other things happening in this method from
> happening if the loss of first responder is temporary? Specifically things
> like interrupting text input and sending DOM events related to focused state
> (if we do that)?
I don't fully understand what we do in WebHTMLView.mm. I'll hold off landing the change
until we had a chance to talk about this.

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 250274 [details]
> > Patch2
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=250274&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/mac/WKView.mm:474
> > >> +    return [self superview] == nextResponder;
> > > 
> > > Perhaps we should store [self superview] into a local variable named webView so that the intent is clear?
> > 
> > Why are we checking for the superview and not our WKWebView?
> > 
> How do I get from the WKView to the WKWebView? Anders suggested to use the
> superview.

It would be trivial to add a way to get the WKWebView from the page client. It seems like using the superview like this would do the wrong thing when it’s not a WKWebView and it is legitimately becoming the first responder.

[Enrica Casucci]

Created attachment 250360 [details]
Patch3

After discussing with Dan, here is a new patch.

Comment on attachment 250360 [details]
Patch3

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:639
> +#if !PLATFORM(IOS)
> +- (BOOL)becomeFirstResponder
> +{
> +    return [[self window] makeFirstResponder: _wkView.get()];
> +}
> +
> +- (BOOL)acceptsFirstResponder
> +{
> +    return [_wkView acceptsFirstResponder];
> +}
> +#endif
> +

Can you move this into the iOS-specific methods block that follows?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:224
> +    bool _willBecomeFirstResponderAgain;

Despite two counterexamples above, we normally use BOOL for ivars (see good example below).

> Source/WebKit2/UIProcess/API/mac/WKView.mm:480
> +    if ([nextResponder isKindOfClass:[WKWebView class]] && [self superview] == nextResponder) {

self.superview

[Enrica Casucci]

(In reply to comment #8)
> Comment on attachment 250360 [details]
> Patch3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250360&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:639
> > +#if !PLATFORM(IOS)
> > +- (BOOL)becomeFirstResponder
> > +{
> > +    return [[self window] makeFirstResponder: _wkView.get()];
> > +}
> > +
> > +- (BOOL)acceptsFirstResponder
> > +{
> > +    return [_wkView acceptsFirstResponder];
> > +}
> > +#endif
> > +
> 
> Can you move this into the iOS-specific methods block that follows?
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:224
> > +    bool _willBecomeFirstResponderAgain;
> 
> Despite two counterexamples above, we normally use BOOL for ivars (see good
> example below).
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:480
> > +    if ([nextResponder isKindOfClass:[WKWebView class]] && [self superview] == nextResponder) {
> 
> self.superview

Will do. Thanks for the review.

[Enrica Casucci]

Committed revision 182554