Closed Bug 1666247 Opened 2 years ago Closed 11 months ago

Add the Simplify Page checkbox to the new print UI

Categories

(Toolkit :: Printing, defect, P2)

defect

Tracking

()

VERIFIED FIXED
91 Branch
Tracking Status
relnote-firefox --- 91+
firefox91 --- verified
firefox92 --- verified

People

(Reporter: jwatt, Assigned: mstriemer)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

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

Attachments

(3 files, 1 obsolete file)

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.

See Also: 1649204140718

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

Severity: -- → S2
Type: task → defect
Assignee: nobody → mstriemer
Status: NEW → ASSIGNED
Attached file Bug 1666247 - Simplify page WIP (obsolete) —
Whiteboard: [print2020_v83][old-ui-] → [print2020_v84][old-ui-]

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

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

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

(Moving bugs to 86, part 1.)

Whiteboard: [print2020_v85][old-ui-] → [print2020_v86][old-ui-]
Attachment #9177520 - Attachment is obsolete: true
Attachment #9198041 - Attachment description: Bug 1666247 - Add a simplify page checkbox → Bug 1666247 - Add a simplify page checkbox r?emalysz
Attachment #9198041 - Attachment description: Bug 1666247 - Add a simplify page checkbox r?emalysz → Bug 1666247 - Part 1: Add a simplify page checkbox r?emalysz
See Also: → 1666812
Whiteboard: [print2020_v86][old-ui-] → [print2020_v87][old-ui-]

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.

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

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… 🙂

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.

Flags: needinfo?(mstriemer)
Flags: needinfo?(francesco.lodolo)

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

Flags: needinfo?(francesco.lodolo)

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

Whiteboard: [print2020_v87][old-ui-] → [print2020_v90][old-ui-]
Blocks: 1714639

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

Flags: needinfo?(mstriemer) → needinfo?(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.

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

Depends on: 1718200
Depends on: 1718204

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

Flags: needinfo?(emilio)

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

Flags: needinfo?(mstriemer)
Pushed by mstriemer@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
Flags: needinfo?(mstriemer)

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

Flags: needinfo?(mstriemer)
Regressions: 1720178
Flags: qe-verify+

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

relnote-firefox: --- → ?
Flags: needinfo?(mstriemer)

Note added top our beta release notes, thanks.

Regressions: 1720877
Regressions: 1720454
Regressions: 1721817

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1702501

This was included in the final 91.0 release notes.

Regressions: 1747997
You need to log in before you can comment on or make changes to this bug.