Some clients may not want the web view to be programmatically made the first responder.
Created attachment 307616 [details] Patch
Comment on attachment 307616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307616&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:210 > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress; Technically you're only suppressing first responder changes that originate inside WebKit; someone else could still make us first responder. Under what circumstances is the PageClientImpl function called? (maybe we can come up with a more specific name with that information). Alternatively, is there no way for the client to do this without WebKit changes? (it sounds like no, that NSWindow is just going to do their thing, but you could always subclass the window :|)
(In reply to Tim Horton from comment #2) > Comment on attachment 307616 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307616&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:210 > > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress; > > Technically you're only suppressing first responder changes that originate > inside WebKit; someone else could still make us first responder. In case of what I need, that would not happen because the client of this does not want first responder changes in the first place. If this were a problem, the client could always override NSWindow, like you mentioned below. > Under what > circumstances is the PageClientImpl function called? (maybe we can come up > with a more specific name with that information). It's called in a number of cases, including in response to form field focus changes, mouse down events, etc. > Alternatively, is there no > way for the client to do this without WebKit changes? (it sounds like no, > that NSWindow is just going to do their thing, but you could always subclass > the window :|) Unfortunately, no. In case of my client, I do not own the NSWindow.
Created attachment 307628 [details] Patch
Comment on attachment 307628 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307628&action=review > Source/WebKit2/UIProcess/PageClient.h:265 > + virtual void setShouldSuppressFirstResponderChanges(bool) = 0; If we're going with this approach, I think you'll need to implement this in PageClientImplIOS as well (with an empty stub).
Comment on attachment 307628 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307628&action=review > Source/WebKit2/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=171069 Please include a link to <rdar://problem/________> under this line.
Created attachment 307633 [details] Patch
(In reply to Wenson Hsieh from comment #6) > Comment on attachment 307628 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307628&action=review > > > Source/WebKit2/ChangeLog:4 > > + https://bugs.webkit.org/show_bug.cgi?id=171069 > > Please include a link to <rdar://problem/________> under this line. Done.
Comment on attachment 307633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307633&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3725 > +#if PLATFORM(MAC) > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress This shouldn't be platform specific, this concept applies to both platforms.
This needs an API test.
(In reply to Tim Horton from comment #9) > Comment on attachment 307633 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307633&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3725 > > +#if PLATFORM(MAC) > > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress > > This shouldn't be platform specific, this concept applies to both platforms. The method exists on iOS, but it's implemented as "notImplemented()". Not sure we should expose this there then.
(In reply to Matt Rajca from comment #11) > (In reply to Tim Horton from comment #9) > > Comment on attachment 307633 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=307633&action=review > > > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3725 > > > +#if PLATFORM(MAC) > > > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress > > > > This shouldn't be platform specific, this concept applies to both platforms. > > The method exists on iOS, but it's implemented as "notImplemented()". Not > sure we should expose this there then. While this is true of the PageClient implementation, there are a number of places that e.g. WKContentViewInteraction makes itself first responder.
(In reply to Tim Horton from comment #12) > (In reply to Matt Rajca from comment #11) > > (In reply to Tim Horton from comment #9) > > > Comment on attachment 307633 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=307633&action=review > > > > > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3725 > > > > +#if PLATFORM(MAC) > > > > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress > > > > > > This shouldn't be platform specific, this concept applies to both platforms. > > > > The method exists on iOS, but it's implemented as "notImplemented()". Not > > sure we should expose this there then. > > While this is true of the PageClient implementation, there are a number of > places that e.g. WKContentViewInteraction makes itself first responder. Wouldn't it be better to just keep this private API Mac-only since that's all that is needed? I don't want to add more infrastructure for this that no-one will use.
(In reply to Matt Rajca from comment #13) > (In reply to Tim Horton from comment #12) > > (In reply to Matt Rajca from comment #11) > > > (In reply to Tim Horton from comment #9) > > > > Comment on attachment 307633 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=307633&action=review > > > > > > > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3725 > > > > > +#if PLATFORM(MAC) > > > > > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress > > > > > > > > This shouldn't be platform specific, this concept applies to both platforms. > > > > > > The method exists on iOS, but it's implemented as "notImplemented()". Not > > > sure we should expose this there then. > > > > While this is true of the PageClient implementation, there are a number of > > places that e.g. WKContentViewInteraction makes itself first responder. > > Wouldn't it be better to just keep this private API Mac-only since that's > all that is needed? I don't want to add more infrastructure for this that > no-one will use. That is fine for now :) Do add a test, though.
(In reply to Tim Horton from comment #14) > (In reply to Matt Rajca from comment #13) > > (In reply to Tim Horton from comment #12) > > > (In reply to Matt Rajca from comment #11) > > > > (In reply to Tim Horton from comment #9) > > > > > Comment on attachment 307633 [details] > > > > > Patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=307633&action=review > > > > > > > > > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3725 > > > > > > +#if PLATFORM(MAC) > > > > > > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress > > > > > > > > > > This shouldn't be platform specific, this concept applies to both platforms. > > > > > > > > The method exists on iOS, but it's implemented as "notImplemented()". Not > > > > sure we should expose this there then. > > > > > > While this is true of the PageClient implementation, there are a number of > > > places that e.g. WKContentViewInteraction makes itself first responder. > > > > Wouldn't it be better to just keep this private API Mac-only since that's > > all that is needed? I don't want to add more infrastructure for this that > > no-one will use. > > That is fine for now :) Do add a test, though. Yup, working on it now.
Landed with API test: http://trac.webkit.org/changeset/215587/webkit
Comment on attachment 307633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307633&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:210 > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress; This is missing an availability attribute.
A few other notes about this patch: - it’s generally bad form to add a write-only property - it’s generally preferable to allow the client to make a decision on a case-by-case basis; this can be accomplished by consulting the UI delegate when the decision needs to be made instead of having a global policy pushed to the web view
(In reply to mitz from comment #17) > Comment on attachment 307633 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307633&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:210 > > +- (void)_setShouldSuppressFirstResponderChanges:(BOOL)shouldSuppress; > > This is missing an availability attribute. Added in a follow-up patch: https://bugs.webkit.org/show_bug.cgi?id=171224
(In reply to mitz from comment #18) > A few other notes about this patch: > > - it’s generally bad form to add a write-only property > - it’s generally preferable to allow the client to make a decision on a > case-by-case basis; this can be accomplished by consulting the UI delegate > when the decision needs to be made instead of having a global policy pushed > to the web view I guess I didn't want to add complexity/infrastructure we don't need at the time being.