RESOLVED FIXED 171069
Cocoa: add private API for suppressing first responder changes
https://bugs.webkit.org/show_bug.cgi?id=171069
Summary Cocoa: add private API for suppressing first responder changes
Matt Rajca
Reported 2017-04-20 12:32:54 PDT
Some clients may not want the web view to be programmatically made the first responder.
Attachments
Patch (6.45 KB, patch)
2017-04-20 12:36 PDT, Matt Rajca
no flags
Patch (6.26 KB, patch)
2017-04-20 13:27 PDT, Matt Rajca
no flags
Patch (6.32 KB, patch)
2017-04-20 13:55 PDT, Matt Rajca
thorton: review+
Matt Rajca
Comment 1 2017-04-20 12:36:04 PDT
Tim Horton
Comment 2 2017-04-20 13:16:03 PDT
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 :|)
Matt Rajca
Comment 3 2017-04-20 13:26:44 PDT
(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.
Matt Rajca
Comment 4 2017-04-20 13:27:48 PDT
Wenson Hsieh
Comment 5 2017-04-20 13:31:52 PDT
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).
Wenson Hsieh
Comment 6 2017-04-20 13:34:49 PDT
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.
Matt Rajca
Comment 7 2017-04-20 13:55:55 PDT
Matt Rajca
Comment 8 2017-04-20 13:56:19 PDT
(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.
Tim Horton
Comment 9 2017-04-20 14:03:15 PDT
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.
Tim Horton
Comment 10 2017-04-20 14:03:27 PDT
This needs an API test.
Matt Rajca
Comment 11 2017-04-20 14:07:21 PDT
(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.
Tim Horton
Comment 12 2017-04-20 14:13:15 PDT
(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.
Matt Rajca
Comment 13 2017-04-20 14:16:03 PDT
(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.
Tim Horton
Comment 14 2017-04-20 14:17:34 PDT
(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.
Matt Rajca
Comment 15 2017-04-20 14:18:48 PDT
(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.
Matt Rajca
Comment 16 2017-04-20 16:44:22 PDT
mitz
Comment 17 2017-04-20 17:22:55 PDT
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.
mitz
Comment 18 2017-04-21 10:18:24 PDT
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
Matt Rajca
Comment 19 2017-04-24 09:29:01 PDT
(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
Matt Rajca
Comment 20 2017-04-24 09:31:20 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.