We have SPI for this, but it could be useful for other WebKit clients.
<rdar://problem/63406241>
Created attachment 411491 [details] Patch
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?
(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).
(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.
(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.
(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).
Created attachment 411498 [details] Patch
(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.
Created attachment 411506 [details] Patch
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.
(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.
Then it should be on WKPreferences.
Comment on attachment 411506 [details] Patch Also, _textInteractionGesturesEnabled should be deprecated with this as a replacement.
Created attachment 411575 [details] Patch
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?
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.
(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.
(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.
(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.
(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.
(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.
<rdar://problem/70589521>
Created attachment 415233 [details] Patch
Created attachment 415240 [details] Patch
Created attachment 415253 [details] Patch
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
(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.
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
(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.
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?
Created attachment 415256 [details] Patch
(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.
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.
Created attachment 415432 [details] Patch for landing
(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!
Committed r270446: <https://trac.webkit.org/changeset/270446> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415432 [details].
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) :(
Reopening to attach new patch.
Created attachment 438153 [details] Patch
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].