Bug 227265 – [Live Text] [macOS] Add an internal option to disable inline text selection in images

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

Add an (off-by-default) internal feature flag to control whether or not we should allow inline text selection inside images. When disallowing inline text selection in images, we will instead have a context menu action to "Quick Look", which will automatically be retitled to either "Look Up in Quick Look" or "Select Text in Quick Look" depending on the presence of recognized text or visual search results for the image.

[Wenson Hsieh]

rdar://79460142

[Wenson Hsieh]

Created attachment 431990 [details]
For EWS

[Wenson Hsieh]

Created attachment 431991 [details]
For EWS

[Tim Horton]

Comment on attachment 431991 [details]
For EWS

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:238
>  void WebViewImpl::computeHasVisualSearchResults(const URL& imageURL, ShareableBitmap& imageBitmap, CompletionHandler<void(bool)>&& completion)

These seem ... slightly silly, instead of just passing the nice pretty enum. But also fine.

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:553
> +                page->computeHasVisualSearchResults(imageURL, *imageBitmap, [weakThis = makeWeakPtr(protectedThis.get()), quickLookItemToInsertIfNeeded = WTFMove(*quickLookItemToInsertIfNeeded)] (bool hasVisualSearchResults) mutable {

This function is getting a bit out of hand, maybe the visual search stuff can be in its own? (also then you could drop a lot of the `protectedThis`)

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:607
> +        auto item = retainPtr([m_menu itemAtIndex:itemIndex]);

Not sure what the retainPtr() is for! Where's the item going to go?

[Wenson Hsieh]

Comment on attachment 431991 [details]
For EWS

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

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:238
>>  void WebViewImpl::computeHasVisualSearchResults(const URL& imageURL, ShareableBitmap& imageBitmap, CompletionHandler<void(bool)>&& completion)
> 
> These seem ... slightly silly, instead of just passing the nice pretty enum. But also fine.

Good point — I'll change this so that we just have `computeHasImageAnalysisResults` instead of both `computeHasTextRecognitionResults` and `computeHasVisualSearchResults`.

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:553
>> +                page->computeHasVisualSearchResults(imageURL, *imageBitmap, [weakThis = makeWeakPtr(protectedThis.get()), quickLookItemToInsertIfNeeded = WTFMove(*quickLookItemToInsertIfNeeded)] (bool hasVisualSearchResults) mutable {
> 
> This function is getting a bit out of hand, maybe the visual search stuff can be in its own? (also then you could drop a lot of the `protectedThis`)

Sounds good — I pulled the logic in this ENABLE(IMAGE_ANALYSIS) block out into a separate helper method named `insertOrUpdateQuickLookImageItem`.

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:607
>> +        auto item = retainPtr([m_menu itemAtIndex:itemIndex]);
> 
> Not sure what the retainPtr() is for! Where's the item going to go?

I suppose the item's not going anywhere :P removed the RetainPtr.

[Wenson Hsieh]

Created attachment 432004 [details]
Fix GTK/WPE/Win builds

[Wenson Hsieh]

Created attachment 432005 [details]
Fix GTK/WPE/Win builds (2)

[EWS]

Committed r279164 (239061@main): <https://commits.webkit.org/239061@main>

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