Bug 171069

Summary: Cocoa: add private API for suppressing first responder changes
Product: WebKit Reporter: Matt Rajca <mrajca>
Component: WebKit2Assignee: Matt Rajca <mrajca>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz, thorton, wenson_hsieh
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch thorton: review+

Description Matt Rajca 2017-04-20 12:32:54 PDT
Some clients may not want the web view to be programmatically made the first responder.
Comment 1 Matt Rajca 2017-04-20 12:36:04 PDT
Created attachment 307616 [details]
Patch
Comment 2 Tim Horton 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 :|)
Comment 3 Matt Rajca 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.
Comment 4 Matt Rajca 2017-04-20 13:27:48 PDT
Created attachment 307628 [details]
Patch
Comment 5 Wenson Hsieh 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).
Comment 6 Wenson Hsieh 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.
Comment 7 Matt Rajca 2017-04-20 13:55:55 PDT
Created attachment 307633 [details]
Patch
Comment 8 Matt Rajca 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.
Comment 9 Tim Horton 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.
Comment 10 Tim Horton 2017-04-20 14:03:27 PDT
This needs an API test.
Comment 11 Matt Rajca 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.
Comment 12 Tim Horton 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.
Comment 13 Matt Rajca 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.
Comment 14 Tim Horton 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.
Comment 15 Matt Rajca 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.
Comment 16 Matt Rajca 2017-04-20 16:44:22 PDT
Landed with API test:
http://trac.webkit.org/changeset/215587/webkit
Comment 17 mitz 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.
Comment 18 mitz 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
Comment 19 Matt Rajca 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
Comment 20 Matt Rajca 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.