RESOLVED FIXED 219986
Fix "Open with Preview" menu item in PDF context menus on Big Sur
https://bugs.webkit.org/show_bug.cgi?id=219986
Summary Fix "Open with Preview" menu item in PDF context menus on Big Sur
Alex Christensen
Reported 2020-12-17 11:10:29 PST
Fix "Open with Preview" menu item in PDF context menus on Big Sur
Attachments
Patch (8.36 KB, patch)
2020-12-17 11:14 PST, Alex Christensen
ews-feeder: commit-queue-
Patch (8.87 KB, patch)
2020-12-17 11:31 PST, Alex Christensen
ews-feeder: commit-queue-
Patch (9.82 KB, patch)
2020-12-17 11:39 PST, Alex Christensen
no flags
Alex Christensen
Comment 1 2020-12-17 11:14:53 PST
Alex Christensen
Comment 2 2020-12-17 11:14:56 PST
Geoffrey Garen
Comment 3 2020-12-17 11:24:57 PST
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 4 2020-12-17 11:27:18 PST
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
Comment 5 2020-12-17 11:29:26 PST
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
Comment 6 2020-12-17 11:31:21 PST
Alex Christensen
Comment 7 2020-12-17 11:39:41 PST
Geoffrey Garen
Comment 8 2020-12-17 12:34:45 PST
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 9 2020-12-17 14:12:25 PST
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
Comment 10 2020-12-17 14:21:34 PST
Committed r270946: <https://trac.webkit.org/changeset/270946> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416444 [details].
Note You need to log in before you can comment on or make changes to this bug.