1666247 - Add the Simplify Page checkbox to the new print UI

Status: VERIFIED FIXED

(Toolkit :: Printing, defect, P2)

Description

[Jonathan Watt [:jwatt]]

A comment on twitter asking that we don't remove the Simplify Page checkbox got me looking for the bug on that. Apparently we don't already have a bug on file though.

We need this for parity with the old UI, and given this is one of our distinguishing features over what Chrome offers we should really try to implement this for the new UI sooner rather than later.

The infrastructure necessary to implement this will be similar to what's needed for bug 140718.

Comment 1

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #0)

A comment on twitter asking...

More complaints about the lack of Simplify Page in the new UI in the comments on this article FWIW.

Comment 2

[Mark Striemer [:mstriemer]]

Attachment: Bug 1666247 - Simplify page WIP

Comment 3

[Blake Winton (:bwinton) (:☕️)]

This seems a little risky for 84, so slipping to 85.

Comment 4

[Jonathan Watt [:jwatt]]

Another SUMO complaint about this: https://support.mozilla.org/en-US/questions/1313472

Comment 5

[Blake Winton (:bwinton) (:☕️)]

(Moving bugs to 86, part 1.)

Comment 6

[Mark Striemer [:mstriemer]]

Attachment: Bug 1666247 - Part 1: Add a simplify page checkbox r?emalysz

Comment 7

[Alice Wyman]

Related SuMo KB article:
https://support.mozilla.org/en-US/kb/firefox-simplify-page-clutter-free-printing

Comment 8

[Mark Striemer [:mstriemer]]

Attachment: Bug 1666247 - Part 2: Return dialog from tabDialogBox.open r?pbz

Depends on D102356

Comment 9

[jscher2000]

Although I've been telling people for weeks on SUMO and Reddit to use Reader View for a simplified print layout, the margins and default line spacing are very different so there must be some difference in the code path or CSS files for Simplify Page compared with Reader View. I think users accustomed to using Simplify Page would appreciate having the classic tweaks applied.

Comment 10

[jscher2000]

Any chance this will make it into Firefox 87? We probably are close to the cut-off.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Given the invasiveness and complications in this patch, we aren't going to uplift it to 87 (which is already in Beta). I'm still confident that it will land in Firefox 88, though, if that helps… 🙂

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:mstriemer, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 13

[Francesco Lodolo [:flod]]

(I really don't think picking a random reviewer to NI is a great approach)

Comment 14

[jscher2000]

This came up on SUMO again today (https://support.mozilla.org/questions/1332525). There's definitely still user interest in this feature.

Comment 15

[Mark Striemer [:mstriemer]]

Attachment: Bug 1666247 - Part 3: Disable tab modal print on about:sessionrestore r?emalysz

Depends on D103525

Comment 16

[Mark Striemer [:mstriemer]]

Emilio, I'm having some trouble with a leak in the new test for this and was wondering if you had any ideas. The patches here have been sitting a while and this is a much requested feature that I think we're considering an uplift to 90 for.

I ran a try build for these patches [1] and toolkit/components/printing/tests/browser_print_simplified_mode.js is leaking in debug builds on all platforms.

I have done some digging and tracked the issue down to the link tags for the CSS in PrintingChild.jsm. This seemed odd, but commenting out the simplifyMode.css and inlining the aboutReader.css file info a <style> tag results in a passing try build [2].

I'm about out of ideas here, I've been banging away at this trying to figure something out but it's getting beyond what I'm used to looking for. Some things of note:

• This patch moves away from the gBrowser.createBrowser() calls to avoid needing swapDocShells which I think was causing problems (I got that code from a patch of yours I think)
• The preview dialogs are created in PrintPreview.createPreviewBrowser() which is a new custom element to manage the 3 preview browsers that we use
• The old simplify mode was creating the preview browser as a regular tab [3] and it loaded about:printpreview which has some special properties elsewhere [4] that are beyond my understanding at a glance
• The old simplify mode seemed to parse the reader mode version and ask it to start a print preview right away, without waiting for the reader mode processing to finish, [5] this doesn't appear to work with the newer frameLoader.printPreview API, the generated preview will be blank
• With these patches the dialog actually crashes the browser when printing from about:sessionrestore (bug 1714639). Part 3 here disables print on that page when tab modal printing is enabled, but that might indicate a larger issue. It seems to crash inside frameLoader.printPreview().

Any help is greatly appreciated here! Thanks :)

[1] https://treeherder.mozilla.org/jobs?repo=try&revision=d51a575793f26539fceb81a946f242dd6f9213a9
[2] https://treeherder.mozilla.org/jobs?repo=try&revision=88bccf16eff6068c4cc092535b2eb6051098a36e
[3] https://searchfox.org/mozilla-central/rev/b52cf6bbe214bd9d93ed9333d0403f7d556ad7c8/browser/base/content/browser.js#3587
[4] https://searchfox.org/mozilla-central/search?q=about%3Aprintpreview
[5] https://searchfox.org/mozilla-central/source/toolkit/components/printing/content/printUtils.js#792,797,813

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mark Striemer [:mstriemer] from comment #16)

I have done some digging and tracked the issue down to the link tags for the CSS in PrintingChild.jsm. This seemed odd, but commenting out the simplifyMode.css and inlining the aboutReader.css file info a <style> tag results in a passing try build [2].

That is a bit odd indeed.

I'm about out of ideas here, I've been banging away at this trying to figure something out but it's getting beyond what I'm used to looking for. Some things of note:

• This patch moves away from the gBrowser.createBrowser() calls to avoid needing swapDocShells which I think was causing problems (I got that code from a patch of yours I think)

Can we land these changes separately perhaps? It might be worth it regardless.

• The preview dialogs are created in PrintPreview.createPreviewBrowser() which is a new custom element to manage the 3 preview browsers that we use
• The old simplify mode was creating the preview browser as a regular tab [3] and it loaded about:printpreview which has some special properties elsewhere [4] that are beyond my understanding at a glance

These don't seem relevant here (in fact we don't use about:printpreview at all with the new print dialog).

• The old simplify mode seemed to parse the reader mode version and ask it to start a print preview right away, without waiting for the reader mode processing to finish, [5] this doesn't appear to work with the newer frameLoader.printPreview API, the generated preview will be blank

When you say "the generated preview will be blank", does that mean "a blank <browser>" (without even a blank page etc), or just a blank page, with no content?

• With these patches the dialog actually crashes the browser when printing from about:sessionrestore (bug 1714639). Part 3 here disables print on that page when tab modal printing is enabled, but that might indicate a larger issue. It seems to crash inside frameLoader.printPreview().

This fixes the crash, but shows a blank <browser>:

diff --git a/dom/base/nsFrameLoader.cpp b/dom/base/nsFrameLoader.cpp
index aea24d56c451f..bc0421a8902dd 100644
--- a/dom/base/nsFrameLoader.cpp
+++ b/dom/base/nsFrameLoader.cpp
@@ -3347,7 +3347,7 @@ already_AddRefed<Promise> nsFrameLoader::PrintPreview(
     return promise.forget();
   }
 
-  nsIDocShell* docShellToCloneInto = nullptr;
+  nsCOMPtr<nsIDocShell> docShellToCloneInto;
   if (aSourceBrowsingContext) {
     // We're going to call `Print()` below on a window that is not our own,
     // which happens when we are creating a new print preview document instead
@@ -3359,6 +3359,8 @@ already_AddRefed<Promise> nsFrameLoader::PrintPreview(
       promise->MaybeRejectWithNotSupportedError("No print preview docShell");
       return promise.forget();
     }
+    // Ensure we have a content viewer.
+    Unused << docShellToCloneInto->GetDocument();
   }
 
   // Unfortunately we can't pass `resolve` directly here because IPDL, for now,

I can try to dig into the leak anyhow.

Comment 18

[Mark Striemer [:mstriemer]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

(In reply to Mark Striemer [:mstriemer] from comment #16)

• This patch moves away from the gBrowser.createBrowser() calls to avoid needing swapDocShells which I think was causing problems (I got that code from a patch of yours I think)

Can we land these changes separately perhaps? It might be worth it regardless.

So I updated the patch for this to use the old gBrowser.createBrowser() method when it was previously doing so (not window.print()) and it didn't fix the leak

I could theoretically do this change outside of this patch, but it doesn't appear to be the problem with these changes.

• The old simplify mode seemed to parse the reader mode version and ask it to start a print preview right away, without waiting for the reader mode processing to finish, [5] this doesn't appear to work with the newer frameLoader.printPreview API, the generated preview will be blank

When you say "the generated preview will be blank", does that mean "a blank <browser>" (without even a blank page etc), or just a blank page, with no content?

Without waiting for the reader mode code to finish the preview will just have the headers and footers (if they're enabled). Waiting a bit and changing a setting will get the reader mode version rendered properly.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Bug 1718200 fixes the leak, bug 1718204 the about:sessionrestore crash.

Comment 20

[:Gijs (he/him)]

Mark, are you unblocked here now, or is there anything else I can help with wrt about:reader / reader mode ?

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8df8fb3d2e0e
Part 1: Add a simplify page checkbox r=emalysz,flod
https://hg.mozilla.org/integration/autoland/rev/f3503fff397e
Part 2: Return dialog from tabDialogBox.open r=pbz
https://hg.mozilla.org/integration/autoland/rev/532840a9ae88
Part 3: Disable tab modal print on about:sessionrestore r=emalysz

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8df8fb3d2e0e
https://hg.mozilla.org/mozilla-central/rev/f3503fff397e
https://hg.mozilla.org/mozilla-central/rev/532840a9ae88

Comment 23

[Pascal Chevrel:pascalc]

Mark, should we mention that this option is back in our release notes? Could you request an addition to our beta release notes if you think this needs promoting? Thanks
https://wiki.mozilla.org/Release_Management/Release_Notes#How_to_decide_whether_a_change_should_be_included_in_release_notes.3F

Comment 24

[Mark Striemer [:mstriemer]]

Release Note Request (optional, but appreciated)
[Why is this notable]: This is an often requested feature that was previously supported and has now been added back

[Suggested wording]: The simplify page when printing feature is back! When printing, under More settings > Format select the Simplified option when available to get a clutter-free page.

[Links (documentation, blog post, etc)]:
SUMO page (will be updated prior to beta->release): https://support.mozilla.org/en-US/kb/print-friendly-pages-firefox-clutter-free-printing
Last SUMO article with the old version of this feature (open the Revision Content section): https://support.mozilla.org/en-US/kb/print-friendly-pages-firefox-clutter-free-printing/revision/214871

Comment 25

[Pascal Chevrel:pascalc]

Note added top our beta release notes, thanks.

Comment 26

[Oana Botisan, Desktop QA]

I verified the fix using latest Nightly 92.0a1 and Firefox 91.0b8 on macOS 10.15, Windows 10 x64 and Ubuntu 18.04 x64. The option for printing a simplified page can be seen in the "Format" section.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This was included in the final 91.0 release notes.