Bug 149646 – Add a WKWebView input delegate SPI

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

Add a delegate API that allows WebKit clients to manipulate an input session, such as being able to override preventing the keyboard from being raised when a field is programmatically focused.

[Wenson Hsieh]

Created attachment 262112 [details]
Patch

[WebKit Commit Bot]

Attachment 262112 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:504:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:504:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Comment on attachment 262112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262112&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKFormDelegate.h:35
> +@protocol WKFormDelegate <NSObject>

Since this is intended to be a public API, we'll need a lot of documentation here explaining when these methods are called/what they do. When we're more sure about which methods to include as public API, I'll write detailed descriptions similar to those in WKUIDelegate.h

> Source/WebKit2/UIProcess/API/Cocoa/WKFormDelegate.h:43
> +- (BOOL)webView:(WKWebView *)webView shouldShowKeyboard:(BOOL)userIsInteracting WK_AVAILABLE(10_11, 9_0);

Since we only have 1 piece of information right now (userIsInteracting) I thought it would make more sense to just pass it in directly here as a parameter -- when we figure out exactly what pieces of information we should give to our clients, we should refactor this method to take in an object or maybe a dictionary containing information about the focused element.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:71
> +@property (WK_NULLABLE_PROPERTY nonatomic, weak) id <WKFormDelegate> formDelegate WK_AVAILABLE(10_11, 9_0);

I changed this from _formDelegate to just formDelegate since we're now making it public. However, this means we'll need to do some small changes to Safari where we use the form delegate to make Safari not break.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:160
> +    std::unique_ptr<WebKit::FormDelegate> _formDelegateWrapper;

I made the WKWebView access the delegate through a C++ wrapper class, like we do in the case of WKUIDelegate and WKNavigationDelegate.

> Source/WebKit2/UIProcess/API/Cocoa/_WKFormDelegate.h:33
> +WK_ASSUME_NONNULL_BEGIN

I believe this was in error. I'll remove this.

[Wenson Hsieh]

Created attachment 262165 [details]
Patch

[WebKit Commit Bot]

Attachment 262165 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:501:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:501:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3020:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3022:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3023:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 262178 [details]
Patch

[WebKit Commit Bot]

Attachment 262178 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:501:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:501:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3020:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3022:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3023:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Comment on attachment 262178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262178&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKFormDelegate.h:26
> +#import <WebKit/WKFoundation.h>

This file will need lots of documentation. I will write it when we're more certain about what information we want to expose.

[Wenson Hsieh]

Created attachment 262273 [details]
Patch

Comment on attachment 262273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262273&action=review

When making changes that affect API, binary- and source-compatiblity need to be considered. Some of these apply even when changing private API. We need to keep TOT WebKit working with the shipping version of iOS Safari, so we can’t just remove the _formDelegate property’s accessor methods, because shipping Safari calls them (at least the setter). We also can’t remove a header like _WFormDelegate.h or remove other declarations from the header files, because it breaks compatibility with some source code internally at Apple, so the changes will need to be staged over a short period of time until that source can be updated.

> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:63
> +@protocol _WKElementFocusContext <NSObject>

I am not sure what “focus context” means. We have been using the suffix “info” for immutable snapshots of objects, such as WKFrameInfo, _WKActivatedElementInfo. Perhaps a name similar to _WKFocusedElementInfo would be more in line with those. Or perhaps this is more about the focusing action and not just the element, in which case maybe _WKElementFocusAction (similar to WKNavigationAction) might work?

Why is this a protocol and not a class?

> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:69
> +@property (nonatomic, readonly) WKInputType inputType;
> +
> +/* The value of the input at the time it is focused. */
> +@property (nonatomic, readonly, copy) NSString *inputValue;

You may be able to drop the “input” suffix from these.

> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:76
> +@property (nonatomic, readonly) BOOL userIsInteracting;

If you followed in the footsteps of WKNavigationAction you’d call this property userInitiated (with a custom isUserInitiated getter).

> Source/WebKit2/UIProcess/API/Cocoa/_WKInputDelegate.h:44
> +- (BOOL)_webView:(WKWebView *)webView shouldStartInputSession:(id <_WKElementFocusContext>)context;

It’s a little confusing that didStartInputSession: takes an id <_WKFormInputSession> and shouldStartInputSession: takes a different type. Perhaps this method should be called -_webView:focusShouldStartInputSession: or even -_webView:focusActionShouldStartInputSession: depending on how you name the protocol.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:285
> +- (instancetype)initWithAssistedNodeInformation:(const AssistedNodeInformation *)information isInteracting:(BOOL)userIsInteracting;

Can this take a reference instead of a pointer?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:290
> +    NSString *_inputValue;

This should be a RetainPtr<NSString>.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:296
> +    if (self = [super init]) {

Instead of having most of the method indented like this, you should reverse the condition and early-return nil.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:350
> +        default:

Why do we need a default case here?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3111
> +        WKElementFocusContext *context = [[WKElementFocusContext alloc] initWithAssistedNodeInformation:&information isInteracting:userIsInteracting];

This is leaking! You should use a RetainPtr to hold this object so that it doesn’t leak.

[Wenson Hsieh]

Comment on attachment 262273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262273&action=review

>> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:63
>> +@protocol _WKElementFocusContext <NSObject>
> 
> I am not sure what “focus context” means. We have been using the suffix “info” for immutable snapshots of objects, such as WKFrameInfo, _WKActivatedElementInfo. Perhaps a name similar to _WKFocusedElementInfo would be more in line with those. Or perhaps this is more about the focusing action and not just the element, in which case maybe _WKElementFocusAction (similar to WKNavigationAction) might work?
> 
> Why is this a protocol and not a class?

_WKFocusedElementInfo sounds much better. The purpose of this entity is to provide clients with information that can help customize the assistance behavior of a focused element -- this includes both a "snapshot" of the focused element, as well as some information about the action itself (e.g. isUserInitiated).

I made this a protocol because I was following the footsteps of WKFormInputSession, which also has a private protocol declaration and an implementation in WKContentViewInteraction.mm where it needs to be initialized.

>> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:69
>> +@property (nonatomic, readonly, copy) NSString *inputValue;
> 
> You may be able to drop the “input” suffix from these.

Changed to just type and value.

>> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:76
>> +@property (nonatomic, readonly) BOOL userIsInteracting;
> 
> If you followed in the footsteps of WKNavigationAction you’d call this property userInitiated (with a custom isUserInitiated getter).

Got it. Renamed to isUserInitiated

>> Source/WebKit2/UIProcess/API/Cocoa/_WKInputDelegate.h:44
>> +- (BOOL)_webView:(WKWebView *)webView shouldStartInputSession:(id <_WKElementFocusContext>)context;
> 
> It’s a little confusing that didStartInputSession: takes an id <_WKFormInputSession> and shouldStartInputSession: takes a different type. Perhaps this method should be called -_webView:focusShouldStartInputSession: or even -_webView:focusActionShouldStartInputSession: depending on how you name the protocol.

Good point. Renamed shouldStartInputSession to focusShouldStartInputSession.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:285
>> +- (instancetype)initWithAssistedNodeInformation:(const AssistedNodeInformation *)information isInteracting:(BOOL)userIsInteracting;
> 
> Can this take a reference instead of a pointer?

Yes -- changed to use a const reference.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:290
>> +    NSString *_inputValue;
> 
> This should be a RetainPtr<NSString>.

Fixed.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:296
>> +    if (self = [super init]) {
> 
> Instead of having most of the method indented like this, you should reverse the condition and early-return nil.

Sounds good! Changed.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:350
>> +        default:
> 
> Why do we need a default case here?

Good point, it isn't necessary, since we have a 1-1 mapping. I was thinking of using default to catch input types we (for any reason) don't want to expose.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3111
>> +        WKElementFocusContext *context = [[WKElementFocusContext alloc] initWithAssistedNodeInformation:&information isInteracting:userIsInteracting];
> 
> This is leaking! You should use a RetainPtr to hold this object so that it doesn’t leak.

Good catch! Fixed.

[Wenson Hsieh]

Created attachment 262626 [details]
Patch

Comment on attachment 262626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262626&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:206
> +@property (nonatomic, weak, getter=_formDelegate, setter=_setFormDelegate:) id <_WKFormDelegate> _formDelegate;

No need for getter=_formDelegate since it’s exactly the same as the property name.

Instead of comment (which is not entirely correct: the declaration is here for source compatibility, the implementation is what gives us binary compatibility) you can annotate this with WK_DEPRECATED(10_10, WK_MAC_TBA, 8_0, WK_IOS_TBA, "use _inputDelegate")

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:207
> +@property (nonatomic, weak, setter=_setInputDelegate:) id <_WKInputDelegate> _inputDelegate;

This needs to be annotated with WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA) since it’s new.

> Source/WebKit2/UIProcess/API/Cocoa/_WKFormDelegate.h:27
> +#ifndef _WKFormDelegate_h
> +#define _WKFormDelegate_h

We don’t use such guards in headers that include Objective-C declarations (unless the declarations themselves are guarded with #if __OBJC__), because they are only imported from Objective-C files, which use #import directives which don’t require such guards.

> Source/WebKit2/UIProcess/API/Cocoa/_WKFormDelegate.h:33
> +#import "_WKInputDelegate.h"

This needs to be a framework-style import, <WebKit/_WKInputDelegate.h>, because this is a framework header.

> Source/WebKit2/UIProcess/API/Cocoa/_WKFormDelegate.h:36
> + * We are transitioning _WKFormDelegate to _WKInputDelegate. This header is here temporarily for binary compatibility and

source compatibility

> Source/WebKit2/UIProcess/API/Cocoa/_WKInputDelegate.h:34
> +@protocol _WKFormInputSession;
> +@protocol _WKFocusedElementInfo;

Please keep these sorted, c before r.

> Source/WebKit2/UIProcess/API/Cocoa/_WKInputDelegate.h:36
> +@protocol _WKInputDelegate <NSObject>

I believe that we should annotate the entire protocol with availability information, but I am not sure exactly how.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:298
> +    self = [super init];
> +    if (!self)
> +        return nil;

We usually save a line by writing this:

if (!(self = [super init]))
    return nil;

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:5303
> +				37A64E5418F38E3C00EB30F1 /* _WKInputDelegate.h */,
>  				37A64E5618F38F4600EB30F1 /* _WKFormInputSession.h */,
> +				2E7A94491BBD95C600945547 /* _WKFocusedElementInfo.h */,

Can you keep these sorted?

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:53
> -#import "WKWebProcessPluginFrameInternal.h"
> -#import "WKWebProcessPlugInInternal.h"
>  #import "WKWebProcessPlugInFormDelegatePrivate.h"
> +#import "WKWebProcessPlugInInternal.h"
>  #import "WKWebProcessPlugInLoadDelegate.h"
>  #import "WKWebProcessPlugInNodeHandleInternal.h"
>  #import "WKWebProcessPlugInPageGroupInternal.h"
>  #import "WKWebProcessPlugInScriptWorldInternal.h"
> +#import "WKWebProcessPluginFrameInternal.h"

What’s this about? Seems unrelated to this patch.

[Wenson Hsieh]

Committed r191228: <http://trac.webkit.org/changeset/191228>

[Wenson Hsieh]

This is work towards <rdar://problem/17355265>