Implement a "Print selection only" checkbox in the new print preview UI
Categories
(Toolkit :: Printing, enhancement, P1)
Tracking
()
People
(Reporter: jwatt, Assigned: emmamalysz)
References
Details
(Whiteboard: [print2020_v85][old-ui-])
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details |
+++ This bug was initially created as a clone of Bug #140718 +++
In bug 140718 Emilio implemented a "Print selection" context menu item. That's definetly something we want, but I think having a checkbox in the settings sidebar for the new print UI is important. Personally I think this would be a significant feature advantage over Chrome if we implement it "right", as I'll outline below.
My general impression from bugs and comments over the years is that "Print selection only" is really valued by the people who have become aware of it, but that not many people I've talked to (most of whom use Chrome) know it exists. When I've been chatting to people and pointed it out in response to complaints about the amount of ink/paper the crap on websites wastes, the response has always been one of "oh, that's a super useful feature!"
I think "Print selection only" is far more discoverable in the legacy Firefox print UI than in Chrome because the Print dialog window always has that option visible, and only greys it out when there is no selection (at least on Windows) rather than hiding it. In Chrome it is not nearly as discoverable since Chrome hides the option unless you make a selection. So the issue for Chrome users is that users have to be aware that printing a selection is "a thing", before they can "discover" it. I.e., the vast majority never will...
(I wish I'd kept links, but on multiple occassions in the past I've seen people mention "Print selection only" as something that Chrome is missing vs Firefox. More recently, after noticing that Chrome does "now" have a checkbox, I thought Chrome had actually added it. But maybe Chrome has had it for a long time and people, expecting to see a disabled checkbox in the UI like you see in the Firefox printing dialog, had drawn the conclusion it isn't something Chrome has.)
For the exact same reason that a hidden checkbox isn't discoverable, I think most users will never discover and come to love "Print selection only" if we only implement it as a context menu - you have to know such a thing exists in order to make a selection and have it appear in your context menu.
Reporter | ||
Comment 1•4 years ago
|
||
(I should also note that when a user prints via the print selection context menu in Chrome, it makes the checkbox in the chrome print UI appear (and checked). Not that that changes any of my argument above, I'm just noting that we should wire that up if we implement this. I guess it also has the advantage that if the user did not intend to print a selection, then the checked checkbox gives them a hint about why content is missing from the preview rather than them potentially getting confused, and it allows them to uncheck it rather than having to exit print preview, remove the selection, and print again.)
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
Assignee | ||
Comment 3•4 years ago
|
||
Hey Emilio,
With the WIP applied, here are STR the problem that I'm seeing:
- Select text on a web page
- Open print preview with command P (or any way that isn't through the context menu, I'm going to have to change a few things there)
- The checkbox should be shown because there is selected text. Click the checkbox
await previewBrowser.frameLoader.printPreview(settings, sourceWinId));
causes an assertion failureNS_ENSURE_TRUE(!mDisallowSelectionPrint && printData->mSelectionRoot) failed: file /Users/emmamalysz/Desktop/firefox-source/layout/printing/nsPrintJob.cpp:2506
and catches an non-descriptive exception:(new UnknownError("Print preview failed", (void 0)))
The failure is from mSelectionRoot
being null. I successfuly tried swapping in selectedPreviewBrowser for the previewBrowser the first time we open the preview to make sure it wasn't an error with how I'm creating the second browser, and that seemed to work. So I'm a little stumped as to why we can't resolve the printPreview promise..
Updated•4 years ago
|
Comment 4•4 years ago
|
||
I think this is because we don't have a sourceWinId
after the first render. If that is instead stored as this.souceWinId
then the switching will work, albeit by re-rendering from the page being printed rather than the last preview.
We'll only want to pass the window id on the first render for each preview browser, and ideally we'd use the full preview as the source for the selection browser if we've already rendered that preview. Passing the preview browser's outerWindowId
seems to error if I try that though.
Updated•4 years ago
|
Comment 5•4 years ago
|
||
I don't know if you updated the patch or if bug 1672864 fixed the issues you were seeing but comment 3 seems to work for me now?
Assignee | ||
Comment 6•4 years ago
|
||
The problem I'm now seeing is when there are two iframes. Taking https://www.w3schools.com/tags/tryit.asp?filename=tryhtml_iframe as an example, if you select text, try to print, and then check the "print selection only checkbox," it will give a print preview error. This is because we are reusing the sourceWinId and not using the selection browser as the source browser. If you select text, print via the context menu, and then uncheck the checkbox, then it will only show the rhs of the page. That is currently how Chrome implements it in each scenario, so it looks like they always pass the selection as the source browser.
I tried asking if it'd be possible to get the focused subframe from the source browser, but the concern is that the parent process wouldn't know which frame is focused. I guess the other option is to pass the selection as the source browser, but how would that be different than the gBrowser.selectedBrowser.browsingContext
(https://searchfox.org/mozilla-central/rev/d866b96d74ec2a63f09ee418f048d23f4fd379a2/browser/base/content/browser.js#2800-2803)?
Comment 7•4 years ago
|
||
gBrowser.selectedBrowser.browsingContext
is always the top browsing context.
The focus manager has the current focused browsing context, but I don't know if it's exposed to chrome JS, or even whether it'd be the right thing to check (I think so though). Henri, do you know?
I believe the focused browsing context is not exposed to JS, because there hasn't been a use case so far. It could be exposed in a manner similar to how the active browsing context is exposed:
https://searchfox.org/mozilla-central/rev/c37038c592a352eda0f5e77dfb58c4929bf8bcd3/dom/interfaces/base/nsIFocusManager.idl#58
Comment 9•4 years ago
|
||
Looks like this is going to be too risky for 84, so we're slipping it.
Assignee | ||
Comment 10•4 years ago
|
||
emilio, getting this on your radar to see about getting the active browsing context in js
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 11•4 years ago
|
||
Updated•4 years ago
|
Comment 12•4 years ago
|
||
Comment 13•4 years ago
|
||
bugherder |
Updated•4 years ago
|
Updated•4 years ago
|
Comment 14•4 years ago
|
||
I managed to reproduce the issue using an oldere version on Nightly (2020-10-08) on Windows 10 x64.
I retested everything using Nightly 87.0a1 and Firefox 86.0b5 on Windows 10 x64, Ubuntu 18.04 x64 and macOS 10.13. The issue is not reproducing anymore.
Description
•