Bug 217784 – Create API to enable/disable text interaction gestures in WKWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

We have SPI for this, but it could be useful for other WebKit clients.

[Kate Cheney]

<rdar://problem/63406241>

[Kate Cheney]

Created attachment 411491 [details]
Patch

[Tim Horton]

Comment on attachment 411491 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:469
> + @discussion The default value is YES for iOS, and NO for WatchOS.

watchOS is spelled watchOS

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:471
> +@property (nonatomic, setter=setTextInteractionGesturesEnabled:) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(ios(WK_IOS_TBA));

You don't need to specify the setter if it isn't special.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:472
> +#endif

Has the name been API-reviewed?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:369
> +#if PLATFORM(WATCHOS)
> +    [self setTextInteractionGesturesEnabled:NO];

Are watchOS clients going to need to set this now, in addition to the other API? (since if either one is NO, you bail from installing the gestures?)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2524
> -    if (!self.webView.configuration._textInteractionGesturesEnabled)
> +    if (!self.webView.configuration._textInteractionGesturesEnabled || !self.webView.textInteractionGesturesEnabled)

Odd that the SPI is on WKWebViewConfiguration and the API is on WKWebView. Any reason for that?

[Kate Cheney]

(In reply to Tim Horton from comment #3)
> Comment on attachment 411491 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411491&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:469
> > + @discussion The default value is YES for iOS, and NO for WatchOS.
> 
> watchOS is spelled watchOS
> 

Oops, will fix.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:471
> > +@property (nonatomic, setter=setTextInteractionGesturesEnabled:) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(ios(WK_IOS_TBA));
> 
> You don't need to specify the setter if it isn't special.
> 

Got it -- will remove.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:472
> > +#endif
> 
> Has the name been API-reviewed?
> 

The review process has been initiated, but the name has not been formally reviewed yet.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:369
> > +#if PLATFORM(WATCHOS)
> > +    [self setTextInteractionGesturesEnabled:NO];
> 
> Are watchOS clients going to need to set this now, in addition to the other
> API? (since if either one is NO, you bail from installing the gestures?)
> 

I copied this behavior from the SPI, but maybe I misunderstood what was going on?

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2524
> > -    if (!self.webView.configuration._textInteractionGesturesEnabled)
> > +    if (!self.webView.configuration._textInteractionGesturesEnabled || !self.webView.textInteractionGesturesEnabled)
> 
> Odd that the SPI is on WKWebViewConfiguration and the API is on WKWebView.
> Any reason for that?

Yes, so it can be changed on the fly (see WebKit ChangeLog).

[Tim Horton]

(In reply to katherine_cheney from comment #4)
> (In reply to Tim Horton from comment #3)
> > Comment on attachment 411491 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=411491&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:469
> > > + @discussion The default value is YES for iOS, and NO for WatchOS.
> > 
> > watchOS is spelled watchOS
> > 
> 
> Oops, will fix.
> 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:471
> > > +@property (nonatomic, setter=setTextInteractionGesturesEnabled:) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(ios(WK_IOS_TBA));
> > 
> > You don't need to specify the setter if it isn't special.
> > 
> 
> Got it -- will remove.
> 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:472
> > > +#endif
> > 
> > Has the name been API-reviewed?
> > 
> 
> The review process has been initiated, but the name has not been formally
> reviewed yet.
> 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:369
> > > +#if PLATFORM(WATCHOS)
> > > +    [self setTextInteractionGesturesEnabled:NO];
> > 
> > Are watchOS clients going to need to set this now, in addition to the other
> > API? (since if either one is NO, you bail from installing the gestures?)
> > 
> 
> I copied this behavior from the SPI, but maybe I misunderstood what was
> going on?

Maybe it's OK because nobody ever turns the SPI ON on watchOS? But if they /did/, you would be introducing a new source of "false", and overriding their preference, right?

> > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2524
> > > -    if (!self.webView.configuration._textInteractionGesturesEnabled)
> > > +    if (!self.webView.configuration._textInteractionGesturesEnabled || !self.webView.textInteractionGesturesEnabled)
> > 
> > Odd that the SPI is on WKWebViewConfiguration and the API is on WKWebView.
> > Any reason for that?
> 
> Yes, so it can be changed on the fly (see WebKit ChangeLog).

I guess we'll eventually transition away from the SPI.

[Kate Cheney]

(In reply to Tim Horton from comment #5)
> (In reply to katherine_cheney from comment #4)
> > (In reply to Tim Horton from comment #3)
> > > Comment on attachment 411491 [details]
> > > Patch
> > > 
> > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:369
> > > > +#if PLATFORM(WATCHOS)
> > > > +    [self setTextInteractionGesturesEnabled:NO];
> > > 
> > > Are watchOS clients going to need to set this now, in addition to the other
> > > API? (since if either one is NO, you bail from installing the gestures?)
> > > 
> > 
> > I copied this behavior from the SPI, but maybe I misunderstood what was
> > going on?
> 
> Maybe it's OK because nobody ever turns the SPI ON on watchOS? But if they
> /did/, you would be introducing a new source of "false", and overriding
> their preference, right?
> 

It is pretty explicitly set to NO in WebViewConfiguration init for watchOS, maybe Wenson can explain why that was done for the SPI so we can understand what we need to do here.

[Wenson Hsieh]

(In reply to katherine_cheney from comment #6)
> (In reply to Tim Horton from comment #5)
> > (In reply to katherine_cheney from comment #4)
> > > (In reply to Tim Horton from comment #3)
> > > > Comment on attachment 411491 [details]
> > > > Patch
> > > > 
> > > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:369
> > > > > +#if PLATFORM(WATCHOS)
> > > > > +    [self setTextInteractionGesturesEnabled:NO];
> > > > 
> > > > Are watchOS clients going to need to set this now, in addition to the other
> > > > API? (since if either one is NO, you bail from installing the gestures?)
> > > > 
> > > 
> > > I copied this behavior from the SPI, but maybe I misunderstood what was
> > > going on?
> > 
> > Maybe it's OK because nobody ever turns the SPI ON on watchOS? But if they
> > /did/, you would be introducing a new source of "false", and overriding
> > their preference, right?
> > 
> 
> It is pretty explicitly set to NO in WebViewConfiguration init for watchOS,
> maybe Wenson can explain why that was done for the SPI so we can understand
> what we need to do here.

The default value for that configuration flag is NO on watchOS because we don't want WKWebView clients to allow text selection on watchOS; AFAIK, there are no WebKit clients on watchOS that would want to enable text selection.

(I think Tim is suggesting that this could theoretically break behavior for watchOS WebKit clients that currently attempt to enable text selection, but I don't think anyone actually does this, so taking either flag to mean "disable selection" should be fine).

[Kate Cheney]

Created attachment 411498 [details]
Patch

[Kate Cheney]

(In reply to Wenson Hsieh from comment #7)
> (In reply to katherine_cheney from comment #6)
> > (In reply to Tim Horton from comment #5)
> > > (In reply to katherine_cheney from comment #4)
> > > > (In reply to Tim Horton from comment #3)
> > > > > Comment on attachment 411491 [details]
> > > > > Patch
> > > > > 
> > > > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:369
> > > > > > +#if PLATFORM(WATCHOS)
> > > > > > +    [self setTextInteractionGesturesEnabled:NO];
> > > > > 
> > > > > Are watchOS clients going to need to set this now, in addition to the other
> > > > > API? (since if either one is NO, you bail from installing the gestures?)
> > > > > 
> > > > 
> > > > I copied this behavior from the SPI, but maybe I misunderstood what was
> > > > going on?
> > > 
> > > Maybe it's OK because nobody ever turns the SPI ON on watchOS? But if they
> > > /did/, you would be introducing a new source of "false", and overriding
> > > their preference, right?
> > > 
> > 
> > It is pretty explicitly set to NO in WebViewConfiguration init for watchOS,
> > maybe Wenson can explain why that was done for the SPI so we can understand
> > what we need to do here.
> 
> The default value for that configuration flag is NO on watchOS because we
> don't want WKWebView clients to allow text selection on watchOS; AFAIK,
> there are no WebKit clients on watchOS that would want to enable text
> selection.
> 
> (I think Tim is suggesting that this could theoretically break behavior for
> watchOS WebKit clients that currently attempt to enable text selection, but
> I don't think anyone actually does this, so taking either flag to mean
> "disable selection" should be fine).

OK, I see, that's a good point.

[Kate Cheney]

Created attachment 411506 [details]
Patch

[Alex Christensen]

Comment on attachment 411506 [details]
Patch

This shouldn't be on WKWebView.  It should be on WKWebViewConfiguration if it's something that shouldn't change during the lifetime of the WKWebView and WKPreferences otherwise.  Also, the boolean is backed only by ObjC, which is something we try to avoid in WebKit, where the ObjC layer is just a thin wrapper around the C++ objects.

[Wenson Hsieh]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 411506 [details]
> Patch
> 
> This shouldn't be on WKWebView.  It should be on WKWebViewConfiguration if
> it's something that shouldn't change during the lifetime of the WKWebView
> and WKPreferences otherwise.  Also, the boolean is backed only by ObjC,
> which is something we try to avoid in WebKit, where the ObjC layer is just a
> thin wrapper around the C++ objects.

From the ChangeLog entry, the intention was to make this something that can change on the fly.

[Alex Christensen]

Then it should be on WKPreferences.

[Alex Christensen]

Comment on attachment 411506 [details]
Patch

Also, _textInteractionGesturesEnabled should be deprecated with this as a replacement.

[Kate Cheney]

Created attachment 411575 [details]
Patch

[Kate Cheney]

Thanks for the comments Alex, I made those changes.

I wasn't sure how to set a default for a specific platform (watchOS) in WKPreferences. Is that possible?

[Sam Weinig]

Comment on attachment 411575 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:67
> +#if TARGET_OS_IPHONE
> +/*! @abstract A Boolean value indicating whether text interaction is disabled.
> +*/
> +@property (nonatomic) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(ios(WK_IOS_TBA));
> +#endif

What makes this iOS specific? Or to put this another way, can we make this work on macOS as well? Seems like disabling interacting with text there is still a meaningful concept.

[Kate Cheney]

(In reply to Sam Weinig from comment #17)
> Comment on attachment 411575 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411575&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:67
> > +#if TARGET_OS_IPHONE
> > +/*! @abstract A Boolean value indicating whether text interaction is disabled.
> > +*/
> > +@property (nonatomic) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(ios(WK_IOS_TBA));
> > +#endif
> 
> What makes this iOS specific? Or to put this another way, can we make this
> work on macOS as well? Seems like disabling interacting with text there is
> still a meaningful concept.

I have not heard of any requests for it to be for macOS, but maybe it could be useful. Potentially out of scope for this patch, which intends only to promote current iOS behavior to API.

[Sam Weinig]

(In reply to katherine_cheney from comment #18)
> (In reply to Sam Weinig from comment #17)
> > Comment on attachment 411575 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=411575&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:67
> > > +#if TARGET_OS_IPHONE
> > > +/*! @abstract A Boolean value indicating whether text interaction is disabled.
> > > +*/
> > > +@property (nonatomic) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(ios(WK_IOS_TBA));
> > > +#endif
> > 
> > What makes this iOS specific? Or to put this another way, can we make this
> > work on macOS as well? Seems like disabling interacting with text there is
> > still a meaningful concept.
> 
> I have not heard of any requests for it to be for macOS, but maybe it could
> be useful. Potentially out of scope for this patch, which intends only to
> promote current iOS behavior to API.

I think we should almost always make having these APIs cross-platform the default, unless there is a specific reason against it. Due to Mac Catalyst, this will have to behave correctly on macOS anyway so by not providing it here, we are just creating a false barrier to creating AppKit based apps.

[Sam Weinig]

(In reply to Sam Weinig from comment #19)
> (In reply to katherine_cheney from comment #18)
> > (In reply to Sam Weinig from comment #17)
> > > Comment on attachment 411575 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=411575&action=review
> > > 
> > > > Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:67
> > > > +#if TARGET_OS_IPHONE
> > > > +/*! @abstract A Boolean value indicating whether text interaction is disabled.
> > > > +*/
> > > > +@property (nonatomic) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(ios(WK_IOS_TBA));
> > > > +#endif
> > > 
> > > What makes this iOS specific? Or to put this another way, can we make this
> > > work on macOS as well? Seems like disabling interacting with text there is
> > > still a meaningful concept.
> > 
> > I have not heard of any requests for it to be for macOS, but maybe it could
> > be useful. Potentially out of scope for this patch, which intends only to
> > promote current iOS behavior to API.

Promotion to API is a big deal. When we do it, we need to consider the wider landscape, and re-evaluate decisions made in the past. I would push you to reconsider the scope.

[Kate Cheney]

(In reply to Sam Weinig from comment #20)
> (In reply to Sam Weinig from comment #19)
> > (In reply to katherine_cheney from comment #18)
> > > (In reply to Sam Weinig from comment #17)
> > > > Comment on attachment 411575 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=411575&action=review
> > > > 
> > > > > Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:67
> > > > > +#if TARGET_OS_IPHONE
> > > > > +/*! @abstract A Boolean value indicating whether text interaction is disabled.
> > > > > +*/
> > > > > +@property (nonatomic) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(ios(WK_IOS_TBA));
> > > > > +#endif
> > > > 
> > > > What makes this iOS specific? Or to put this another way, can we make this
> > > > work on macOS as well? Seems like disabling interacting with text there is
> > > > still a meaningful concept.
> > > 
> > > I have not heard of any requests for it to be for macOS, but maybe it could
> > > be useful. Potentially out of scope for this patch, which intends only to
> > > promote current iOS behavior to API.
> 
> Promotion to API is a big deal. When we do it, we need to consider the wider
> landscape, and re-evaluate decisions made in the past. I would push you to
> reconsider the scope.

Ok. How about a FIXME to implement for macOS which I can work on next? I agree it's a big deal to promote to API and re-evaluating the scope seems like a good idea. It seems logical to me to separate these into two patches, because macOS would require new implementation instead of just copying the SPI, meaning its more likely to potentially introduce a regression and need to be rolled out, in which case it would be nice to have it isolated in its own patch.

[Sam Weinig]

(In reply to katherine_cheney from comment #21)
> (In reply to Sam Weinig from comment #20)
> > (In reply to Sam Weinig from comment #19)
> > > (In reply to katherine_cheney from comment #18)
> > > > (In reply to Sam Weinig from comment #17)
> > > > > Comment on attachment 411575 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=411575&action=review
> > > > > 
> > > > > > Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:67
> > > > > > +#if TARGET_OS_IPHONE
> > > > > > +/*! @abstract A Boolean value indicating whether text interaction is disabled.
> > > > > > +*/
> > > > > > +@property (nonatomic) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(ios(WK_IOS_TBA));
> > > > > > +#endif
> > > > > 
> > > > > What makes this iOS specific? Or to put this another way, can we make this
> > > > > work on macOS as well? Seems like disabling interacting with text there is
> > > > > still a meaningful concept.
> > > > 
> > > > I have not heard of any requests for it to be for macOS, but maybe it could
> > > > be useful. Potentially out of scope for this patch, which intends only to
> > > > promote current iOS behavior to API.
> > 
> > Promotion to API is a big deal. When we do it, we need to consider the wider
> > landscape, and re-evaluate decisions made in the past. I would push you to
> > reconsider the scope.
> 
> Ok. How about a FIXME to implement for macOS which I can work on next? I
> agree it's a big deal to promote to API and re-evaluating the scope seems
> like a good idea. It seems logical to me to separate these into two patches,
> because macOS would require new implementation instead of just copying the
> SPI, meaning its more likely to potentially introduce a regression and need
> to be rolled out, in which case it would be nice to have it isolated in its
> own patch.

If you are going to make it work cross platform, I would argue you should do that first and delay introducing API until you are ready, and have tested it in all the relevant configurations.

[Radar WebKit Bug Importer]

<rdar://problem/70589521>

[Kate Cheney]

Created attachment 415233 [details]
Patch

[Kate Cheney]

Created attachment 415240 [details]
Patch

[Kate Cheney]

Created attachment 415253 [details]
Patch

[Tim Horton]

Comment on attachment 415253 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:571
> +WK_EXPORT void WKPreferencesSetTextInteractionGesturesEnabled(WKPreferencesRef, bool flag);

Let's not add new C SPI in 2020 if at all possible.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:145
> +    ALLOW_DEPRECATED_DECLARATIONS_BEGIN

This doesn't make sense around an ivar (to me)

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:359
>      self._textInteractionGesturesEnabled = [coder decodeBoolForKey:@"textInteractionGesturesEnabled"];

This one I understand.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:419
> +    ALLOW_DEPRECATED_DECLARATIONS_BEGIN

Nor does it make sense here

[Kate Cheney]

(In reply to Tim Horton from comment #27)
> Comment on attachment 415253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415253&action=review
> 
> > Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:571
> > +WK_EXPORT void WKPreferencesSetTextInteractionGesturesEnabled(WKPreferencesRef, bool flag);
> 
> Let's not add new C SPI in 2020 if at all possible.

This is the only way I see WebPreferences set in WebKitTestRunner, am I missing another way to do it without using C SPI?

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:145
> > +    ALLOW_DEPRECATED_DECLARATIONS_BEGIN
> 
> This doesn't make sense around an ivar (to me)

Ok, will remove.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:359
> >      self._textInteractionGesturesEnabled = [coder decodeBoolForKey:@"textInteractionGesturesEnabled"];
> 
> This one I understand.

yay! :) 

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:419
> > +    ALLOW_DEPRECATED_DECLARATIONS_BEGIN
> 
> Nor does it make sense here

Ditto, will remove.

[Wenson Hsieh]

Comment on attachment 415253 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:571
>>> +WK_EXPORT void WKPreferencesSetTextInteractionGesturesEnabled(WKPreferencesRef, bool flag);
>> 
>> Let's not add new C SPI in 2020 if at all possible.
> 
> This is the only way I see WebPreferences set in WebKitTestRunner, am I missing another way to do it without using C SPI?

I think you should be able to do an ObjC bridging cast from `WKPreferencesRef` to `WKPreferences *` (i.e. `(__bridge WKPreferences *)`). This has the drawback of not being cross-platform (the code would probably be in TestControllerCocoa), but maybe that's okay here since the API is specific to Cocoa anyways :P

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #29)
> Comment on attachment 415253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415253&action=review
> 
> >>> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:571
> >>> +WK_EXPORT void WKPreferencesSetTextInteractionGesturesEnabled(WKPreferencesRef, bool flag);
> >> 
> >> Let's not add new C SPI in 2020 if at all possible.
> > 
> > This is the only way I see WebPreferences set in WebKitTestRunner, am I missing another way to do it without using C SPI?
> 
> I think you should be able to do an ObjC bridging cast from
> `WKPreferencesRef` to `WKPreferences *` (i.e. `(__bridge WKPreferences *)`).
> This has the drawback of not being cross-platform (the code would probably
> be in TestControllerCocoa), but maybe that's okay here since the API is
> specific to Cocoa anyways :P

On second thought, a bridging cast is actually unnecessary, since TestController::cocoaResetStateToConsistentValues can just grab the WKWebView and update the WKWebView's preferences.

[Alex Christensen]

Comment on attachment 415253 [details]
Patch

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

>>>>> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:571
>>>>> +WK_EXPORT void WKPreferencesSetTextInteractionGesturesEnabled(WKPreferencesRef, bool flag);
>>>> 
>>>> Let's not add new C SPI in 2020 if at all possible.
>>> 
>>> This is the only way I see WebPreferences set in WebKitTestRunner, am I missing another way to do it without using C SPI?
>> 
>> I think you should be able to do an ObjC bridging cast from `WKPreferencesRef` to `WKPreferences *` (i.e. `(__bridge WKPreferences *)`). This has the drawback of not being cross-platform (the code would probably be in TestControllerCocoa), but maybe that's okay here since the API is specific to Cocoa anyways :P
> 
> On second thought, a bridging cast is actually unnecessary, since TestController::cocoaResetStateToConsistentValues can just grab the WKWebView and update the WKWebView's preferences.

Can WKPreferencesSetInternalDebugFeatureForKey or WKPreferencesSetExperimentalFeatureForKey work?

[Kate Cheney]

Created attachment 415256 [details]
Patch

[Kate Cheney]

(In reply to Alex Christensen from comment #31)
> Comment on attachment 415253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415253&action=review
> 
> >>>>> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:571
> >>>>> +WK_EXPORT void WKPreferencesSetTextInteractionGesturesEnabled(WKPreferencesRef, bool flag);
> >>>> 
> >>>> Let's not add new C SPI in 2020 if at all possible.
> >>> 
> >>> This is the only way I see WebPreferences set in WebKitTestRunner, am I missing another way to do it without using C SPI?
> >> 
> >> I think you should be able to do an ObjC bridging cast from `WKPreferencesRef` to `WKPreferences *` (i.e. `(__bridge WKPreferences *)`). This has the drawback of not being cross-platform (the code would probably be in TestControllerCocoa), but maybe that's okay here since the API is specific to Cocoa anyways :P
> > 
> > On second thought, a bridging cast is actually unnecessary, since TestController::cocoaResetStateToConsistentValues can just grab the WKWebView and update the WKWebView's preferences.
> 
> Can WKPreferencesSetInternalDebugFeatureForKey or
> WKPreferencesSetExperimentalFeatureForKey work?

I don't think this makes sense because it is not an experimental feature or debug feature. Setting preferences in TestController::cocoaResetStateToConsistentValues() worked though.

[Wenson Hsieh]

Comment on attachment 415256 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferences.yaml:2286
> +      default: true

Should this default to false on watchOS? (I'd imagine that once we remove the old _textInteractionGesturesEnabled SPI, watchOS WebKit clients shouldn't be required to set `textInteractionGesturesEnabled` to `NO` to get the right behavior).

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:65
> +@property (nonatomic) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I'm not sure if "text interaction gestures" is a term that is really used on macOS to describe selecting text and modifying caret selections…but I'd imagine other folks (both WebKit and outside of WebKit) will have more input here. IMO, `textSelectionEnabled` or `textInteractionEnabled` seem more concise and accurate.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewDisableSelection.mm:58
> +    EXPECT_STREQ("", selectedText2.UTF8String);

Nit - you can use EXPECT_WK_STREQ here instead to avoid the extra `.UTF8String`.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewDisableSelection.mm:93
> +    EXPECT_STREQ("", selectedText2.UTF8String);

Ditto.

[Kate Cheney]

Created attachment 415432 [details]
Patch for landing

[Kate Cheney]

(In reply to Wenson Hsieh from comment #34)
> Comment on attachment 415256 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415256&action=review
> 
> > Source/WTF/Scripts/Preferences/WebPreferences.yaml:2286
> > +      default: true
> 
> Should this default to false on watchOS? (I'd imagine that once we remove
> the old _textInteractionGesturesEnabled SPI, watchOS WebKit clients
> shouldn't be required to set `textInteractionGesturesEnabled` to `NO` to get
> the right behavior).

Yes, you're right. Good catch. I fixed this in the patch for landing.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:65
> > +@property (nonatomic) BOOL textInteractionGesturesEnabled WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> I'm not sure if "text interaction gestures" is a term that is really used on
> macOS to describe selecting text and modifying caret selections…but I'd
> imagine other folks (both WebKit and outside of WebKit) will have more input
> here. IMO, `textSelectionEnabled` or `textInteractionEnabled` seem more
> concise and accurate.

Ok, I renamed the new preference to 'textInteractionEnabled', and can make adjustments during the API review process based on feedback if needed.

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewDisableSelection.mm:58
> > +    EXPECT_STREQ("", selectedText2.UTF8String);
> 
> Nit - you can use EXPECT_WK_STREQ here instead to avoid the extra
> `.UTF8String`.
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewDisableSelection.mm:93
> > +    EXPECT_STREQ("", selectedText2.UTF8String);
> 
> Ditto.

Fixed both of these.


Thanks for the review!

[EWS]

Committed r270446: <https://trac.webkit.org/changeset/270446>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415432 [details].

[Tim Horton]

Comment on attachment 415432 [details]
Patch for landing

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

> Source/WTF/Scripts/Preferences/WebPreferences.yaml:2284
> +      "PLATFORM(WATCH_OS)": false

PLATFORM(WATCH_OS) is not a thing, it's PLATFORM(WATCHOS) :(

[Kate Cheney]

Reopening to attach new patch.

[Kate Cheney]

Created attachment 438153 [details]
Patch

[EWS]

Committed r282399 (241660@main): <https://commits.webkit.org/241660@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438153 [details].