Bug 219986 – Fix "Open with Preview" menu item in PDF context menus on Big Sur

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Fix "Open with Preview" menu item in PDF context menus on Big Sur

[Alex Christensen]

Created attachment 416440 [details]
Patch

[Alex Christensen]

<rdar://problem/72406073>

[Geoffrey Garen]

    using Arguments = std::tuple<const WebKit::PDFContextMenu&, const WebKit::PDFPluginIdentifier&>;
                                                                      ~~~~~~~~^
/Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/Release/DerivedSources/WebKit2/WebPageProxyMessages.h:5784:81: error: no type named 'PDFPluginIdentifier' in namespace 'WebKit'
    ShowPDFContextMenu(const WebKit::PDFContextMenu& contextMenu, const WebKit::PDFPluginIdentifier& identifier)
                                                                        ~~~~~~~~^
/Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/Release/DerivedSources/WebKit2/WebPageProxyMessages.h:5789:11: error: unknown type name 'Arguments'
    const Arguments& arguments() const
          ^
/Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/Release/DerivedSources/WebKit2/WebPageProxyMessages.h:5795:5: error: unknown type name 'Arguments'

[Geoffrey Garen]

Comment on attachment 416440 [details]
Patch

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

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:2333
> +    Optional<int> openInPreviewIndex;
>      Vector<PDFContextMenuItem> items;
>      auto itemCount = [nsMenu numberOfItems];
>      for (int i = 0; i < itemCount; i++) {
>          auto item = [nsMenu itemAtIndex:i];
>          if ([item submenu])
>              continue;
> +        if ([NSStringFromSelector(item.action) isEqualToString:@"openWithPreview"])
> +            openInPreviewIndex = i;
>          PDFContextMenuItem menuItem { String([item title]), !![item isEnabled], !![item isSeparatorItem], static_cast<int>([item state]), !![item action], i };
>          items.append(WTFMove(menuItem));
>      }
> -    PDFContextMenu contextMenu { point, WTFMove(items) };
> +    PDFContextMenu contextMenu { point, WTFMove(items), WTFMove(openInPreviewIndex) };

Why do we compute openInPreviewIndex in the WebContent process? Are we not able to compute it in the UI Process? (I think we might be able to compute it in the UI Process inside WebPageProxy::showPDFContextMenu.)

[Alex Christensen]

We are unable to compute it in the UI process because the only attributes it has to distinguish it are its title, which is a localized string that we can't do unlocalized string comparison with, and its selector, which only exists in the web process.

[Alex Christensen]

Created attachment 416442 [details]
Patch

[Alex Christensen]

Created attachment 416444 [details]
Patch

[Geoffrey Garen]

Comment on attachment 416444 [details]
Patch

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

r=me

Can an API test use a fake mouse event to test this in at least a simple case?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:2327
> +        if ([NSStringFromSelector(item.action) isEqualToString:@"openWithPreview"])
> +            openInPreviewIndex = i;

I think this means that a rename inside the PDFLayerController project would reintroduce this bug.

Could you file a request for explicit support for getting this item index?

[Alex Christensen]

Comment on attachment 416444 [details]
Patch

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

>> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:2327
>> +            openInPreviewIndex = i;
> 
> I think this means that a rename inside the PDFLayerController project would reintroduce this bug.
> 
> Could you file a request for explicit support for getting this item index?

This is one of the many, many bugs related to PDFs that we don't have a good way to test.

[EWS]

Committed r270946: <https://trac.webkit.org/changeset/270946>

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