Bug 171069 – Cocoa: add private API for suppressing first responder changes

• Component: WebKit2
• Status: RESOLVED FIXED

[Matt Rajca]

Some clients may not want the web view to be programmatically made the first responder.

[Matt Rajca]

Created attachment 307616 [details]
Patch

[Tim Horton]

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]

(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]

Created attachment 307628 [details]
Patch

[Wenson Hsieh]

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 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]

Created attachment 307633 [details]
Patch

[Matt Rajca]

(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 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]

This needs an API test.

[Matt Rajca]

(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]

(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]

(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]

(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]

(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]

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

[Matt Rajca]

(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]

(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.