WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(6.26 KB, patch)
2017-04-20 13:27 PDT
,
Matt Rajca
no flags
Details
Formatted Diff
Diff
Patch
(6.32 KB, patch)
2017-04-20 13:55 PDT
,
Matt Rajca
thorton
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Matt Rajca
Comment 1
2017-04-20 12:36:04 PDT
Created
attachment 307616
[details]
Patch
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
Created
attachment 307628
[details]
Patch
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
Created
attachment 307633
[details]
Patch
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
Landed with API test:
http://trac.webkit.org/changeset/215587/webkit
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.
Top of Page
Format For Printing
XML
Clone This Bug