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
Created attachment 250271 [details] Patch
Created attachment 250274 [details] Patch2 Fixes iOS build.
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)?
(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.
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
(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.
Committed revision 182554