Quip print from NSMenuItem shows up as an empty page #2744

rr-codes · 2022-07-26T18:17:46Z

`30ffd1a`

Quip print from NSMenuItem shows up as an empty page
https://bugs.webkit.org/show_bug.cgi?id=243218
rdar://90054391

Reviewed by NOBODY (OOPS!).

When a user pressed Cmd+P, some websites (Quip, Google Docs, etc)
intercept this event and do their own processing before sending a print
event. However, when doing `File > Print`, the default print behavior
is used. This causes discrepancies for how these websites are printed
between the two different methods.

This PR addresses this by synthesizing a Cmd+P event whenever
`File > Print` is invoked or when `Print` is selected from the context
menu, which allows for consistent behavior.

* Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _generateSyntheticCommandP:]):
* Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::generateSyntheticCommandP):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:
(WebKit::WebPageProxy::generateSyntheticEditingCommand): Deleted.
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::generateSyntheticCommandP):
* Source/WebKit/WebProcess/WebPage/WebPage.h:
* Source/WebKit/WebProcess/WebPage/WebPage.messages.in:

darinadler · 2022-07-26T19:50:18Z

Source/WebKit/UIProcess/WebPageProxy.cpp

+        return;
+    }
+
+    sendWithAsyncReply(Messages::WebPage::GenerateSyntheticCommandP(), WTFMove(callbackFunction));


This seems to hard-code the command bindings of the web browser.

Is command-P the shortcut for printing worldwide in all the various translated versions of Safari?

I think it’s really sad that the websites hard code a keyboard equivalent, but it seems like this could entirely break printing for some Safari localizations.

We may need to do a more sophisticated fix that always looks like Command-P to the webpage, but then makes sure to actually do a print action once it gets back to the Mac menu system.

Here’s a testing idea: Customize the menu key shortcut for Print, changing it to something like Command-T, using the Keyboard Settings, Keyboard Shortcut, App Shortcuts, and see what happens.

Is command-P the shortcut for printing worldwide in all the various translated versions of Safari?

Yes, it is (Command+P isn't localized in Safari)/

Here’s a testing idea: Customize the menu key shortcut for Print, changing it to something like Command-T, using the Keyboard Settings, Keyboard Shortcut, App Shortcuts, and see what happens.

Yup, I already tested this. Specifically, I tested:

if it works in the DVORAK keyboard layout

if it works for when the user changes the print command key binding (to Command+Option+L for example)

for a non-Latin system language and keyboard (specifically, Hebrew)

and all of these worked.

Note that I also have a corresponding Safari PR which is linked in the Radar. The Safari change will trigger the normal print flow if the command+P event was not handled

whsieh · 2022-07-27T01:47:50Z

Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h

@@ -268,6 +268,8 @@ for this property.

 - (void)_doAfterNextVisibleContentRectUpdate:(void (^)(void))updateBlock WK_API_AVAILABLE(macos(13.0), ios(16.0));

+- (void)_generateSyntheticCommandP:(void (^)(BOOL))completionBlock;


This should be marked as WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA))

do the TBAs mean like a future release? If that's the case, will that affect the Safari PR's mergability?

Source/WebKit/WebProcess/WebPage/WebPage.messages.in

aproskuryakov · 2022-07-27T16:35:54Z

This approach looks very surprising to me, based on years old memories of working in this area. We don't have to synthesize any other keyboard shortcuts. It's up to the embedding application to override those that shouldn't be handled by WebKit (like Cmd+Q or Cmd+W), and the normal behavior is that everything is routed through WebKit.

rr-codes · 2022-07-27T16:57:26Z

This approach looks very surprising to me, based on years old memories of working in this area. We don't have to synthesize any other keyboard shortcuts. It's up to the embedding application to override those that shouldn't be handled by WebKit (like Cmd+Q or Cmd+W), and the normal behavior is that everything is routed through WebKit.

@aproskuryakov this is just a compat hack to make Print from the menu bar work for web apps that currently attempt to intercept all printing by handling Cmd+P. For sites such as Quip, the system print mechanism doesn't work at all and/or doesn't produce reasonable/accurate pages.

https://bugs.webkit.org/show_bug.cgi?id=243218 rdar://90054391 Reviewed by NOBODY (OOPS!). When a user pressed Cmd+P, some websites (Quip, Google Docs, etc) intercept this event and do their own processing before sending a print event. However, when doing `File > Print`, the default print behavior is used. This causes discrepancies for how these websites are printed between the two different methods. This PR addresses this by synthesizing a Cmd+P event whenever `File > Print` is invoked or when `Print` is selected from the context menu, which allows for consistent behavior. * Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm: (-[WKWebView _generateSyntheticCommandP:]): * Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h: * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::generateSyntheticCommandP): * Source/WebKit/UIProcess/WebPageProxy.h: * Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm: (WebKit::WebPageProxy::generateSyntheticEditingCommand): Deleted. * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::generateSyntheticCommandP): * Source/WebKit/WebProcess/WebPage/WebPage.h: * Source/WebKit/WebProcess/WebPage/WebPage.messages.in:

aproskuryakov · 2022-07-27T17:19:07Z

Right, what I'm saying is that ALL shortcuts go to the website, and if Cmd+P doesn't, then it's an exception that needs to be aligned with the norm. There shouldn't be a need to make it even more exceptional.

rr-codes · 2022-07-27T17:28:41Z

Right, what I'm saying is that ALL shortcuts go to the website, and if Cmd+P doesn't, then it's an exception that needs to be aligned with the norm. There shouldn't be a need to make it even more exceptional.

Yes, all shortcuts (including Cmd+P) do indeed go through the website. It's just that when using the menu item (File > Print), the website is unaware of this, and unable to intercept it to do its own handling

aproskuryakov · 2022-07-27T17:41:56Z

Ah, now I understand why you took this path.

Yet, I'm very torn on faking keypresses when there are none, and it doesn't appear to be necessary anyway. Chrome doesn't do this, and yet quip pages print fine from it. What is the actual cause of quip pages being blank when printed in Safari?

And if we do this after all, doing this with an SPI seems like a liability, as embedders that don't use the SPI don't get the fix, and embedders that use it even slightly incorrectly still fail when printing via Apple Events.

darinadler · 2022-07-27T17:45:40Z

Chrome doesn't do this, and yet quip pages print fine from it. What is the actual cause of quip pages being blank when printed in Safari?

I would like to know more about this.

rr-codes · 2022-07-27T17:49:28Z

@aproskuryakov @darinadler Chrome has this same issue as well. Using Command+P on a Quip page in Chrome works as expected, but File > Print has the same issue as Safari (it doesn't get processed by the site and so it renders incorrectly like Safari)

aproskuryakov · 2022-07-27T17:54:30Z

I see a completely blank page in print preview in Safari, and Chrome works. Yes, it's not the same content as with Cmd+P, but I wouldn't say that Chrome has an issue here, much less "the same issue".

aproskuryakov · 2022-07-27T18:03:11Z

There are multiple other approaches for making printing work that I've seen in practice:

print stylesheets;
a "Print" button that opens a popup window formatted for printing, and executes window.print();
generate a PDF, potentially with embedded JS code that also executes window.print().

Intercepting Cmd+P does not catch printing via menu or automation (like Apple Events) in other browsers. Thinking about this as an innovation that other browsers might pick up in the future, we need to consider localizability issues not just on macOS, but on other platforms too, as well as resistance for standardizing such a hacky API.

rr-codes · 2022-07-27T18:08:32Z

Even if we fix whatever caused the preview in Safari to be blank (such that our behavior would match Chrome’s) that’s still a user-facing bug, since the user would probably expect cmd+P to match File > Print.

Do you have any alternative solutions in mind @aproskuryakov ?

aproskuryakov · 2022-07-27T18:54:35Z

Personally, I expect the menu to bypass whatever bugs (or intentional "DRM" features) the site has, and print what I see. But that's likely me not being a normal user.

But anyone would expect some kind of consciously aligned behavior between "Print", "Share" and "Export as PDF" Safari menus, although I'm not sure what exactly it should be.

One solution that doesn't seem like a total hack is having the Print menu dispatch an equivalent of "window.print()" on the frontmost HTML document. That wouldn't fix Quip automatically, but this is something that can be adopted by websites AFAICT. I'm not sure what forums we have for aligning with other browsers on a behavior like this.

rr-codes requested a review from cdumez as a code owner July 26, 2022 18:17

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jul 26, 2022

rr-codes force-pushed the eng/90054391 branch 2 times, most recently from 50b7213 to 2c7dbf8 Compare July 26, 2022 19:15

darinadler requested changes Jul 26, 2022

View reviewed changes

darinadler approved these changes Jul 27, 2022

View reviewed changes

whsieh reviewed Jul 27, 2022

View reviewed changes

Source/WebKit/WebProcess/WebPage/WebPage.messages.in Outdated Show resolved Hide resolved

rr-codes force-pushed the eng/90054391 branch from 2c7dbf8 to 30ffd1a Compare July 27, 2022 17:15

rr-codes closed this Aug 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quip print from NSMenuItem shows up as an empty page #2744

Quip print from NSMenuItem shows up as an empty page #2744

rr-codes commented Jul 26, 2022 •

edited

darinadler Jul 26, 2022 •

edited

rr-codes Jul 26, 2022

whsieh Jul 27, 2022 •

edited

rr-codes Jul 27, 2022

aproskuryakov commented Jul 27, 2022 •

edited

rr-codes commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

rr-codes commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

darinadler commented Jul 27, 2022

rr-codes commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

rr-codes commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

		@@ -268,6 +268,8 @@ for this property.

		- (void)_doAfterNextVisibleContentRectUpdate:(void (^)(void))updateBlock WK_API_AVAILABLE(macos(13.0), ios(16.0));

		- (void)_generateSyntheticCommandP:(void (^)(BOOL))completionBlock;

Quip print from NSMenuItem shows up as an empty page #2744

Quip print from NSMenuItem shows up as an empty page #2744

Conversation

rr-codes commented Jul 26, 2022 • edited

30ffd1a

darinadler Jul 26, 2022 • edited

Choose a reason for hiding this comment

rr-codes Jul 26, 2022

Choose a reason for hiding this comment

whsieh Jul 27, 2022 • edited

Choose a reason for hiding this comment

rr-codes Jul 27, 2022

Choose a reason for hiding this comment

aproskuryakov commented Jul 27, 2022 • edited

rr-codes commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

rr-codes commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

darinadler commented Jul 27, 2022

rr-codes commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

rr-codes commented Jul 27, 2022

aproskuryakov commented Jul 27, 2022

rr-codes commented Jul 26, 2022 •

edited

`30ffd1a`

darinadler Jul 26, 2022 •

edited

whsieh Jul 27, 2022 •

edited

aproskuryakov commented Jul 27, 2022 •

edited