Closed Bug 1670122 Opened 4 years ago Closed 4 years ago

Implement a "Print selection only" checkbox in the new print preview UI

Categories

(Toolkit :: Printing, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
86 Branch
Tracking Status
firefox83 --- wontfix
firefox86 --- verified
firefox87 --- verified

People

(Reporter: jwatt, Assigned: emmamalysz)

References

Details

(Whiteboard: [print2020_v85][old-ui-])

Attachments

(1 file, 1 obsolete file)

+++ 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.

(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.)

No longer depends on: 140718
Assignee: nobody → emalysz

Hey Emilio,
With the WIP applied, here are STR the problem that I'm seeing:

  1. Select text on a web page
  2. 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)
  3. The checkbox should be shown because there is selected text. Click the checkbox
  4. await previewBrowser.frameLoader.printPreview(settings, sourceWinId)); causes an assertion failure NS_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..

Flags: needinfo?(emilio)

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.

Status: NEW → ASSIGNED
Depends on: 1672864

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?

Flags: needinfo?(emilio) → needinfo?(emalysz)

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)?

Flags: needinfo?(emalysz) → needinfo?(emilio)

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?

Flags: needinfo?(emilio) → needinfo?(hsivonen)

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

Flags: needinfo?(hsivonen)

Looks like this is going to be too risky for 84, so we're slipping it.

Whiteboard: [print2020_v84][old-ui-] → [print2020_v85][old-ui-]

emilio, getting this on your radar to see about getting the active browsing context in js

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Depends on: 1681095
Attachment #9183221 - Attachment description: Bug 1670122: check box for selection WIP → Bug 1670122: add check box in print UI to print selection only.
Attachment #9193354 - Attachment is obsolete: true
Pushed by emalysz@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/84d74806a0f7 add check box in print UI to print selection only. r=mstriemer,fluent-reviewers,flod
Regressions: 1683279
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
Regressions: 1683318
Regressions: 1683831
Regressions: 1685184
Flags: qe-verify+

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1697836
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: