Closed Bug 1660739 Opened 4 years ago Closed 4 years ago

Print Preview Redesign Experiment breaks 'tabs.printPreview' API and 'afterprint' listener

Categories

(Core :: Printing: Output, defect, P1)

Firefox 81
defect

Tracking

()

VERIFIED FIXED
82 Branch
Tracking Status
firefox81 --- verified
firefox82 --- verified

People

(Reporter: dw-dev, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [print2020_v81])

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

I am the developer of the recommended Print Edit WE extension. I tried using Print Edit WE with the Print Preview Redesign experiment in Firefox Nightly 80.0a1. This experiment breaks the 'tabs.printPreview' API and 'afterprint' listener.

The steps to reproduce are simple:

  1. Open a web page (e.g. Wikipedia).
  2. Click on the Print Edit WE button on Firefox's toolbar.
  3. Click on the Preview button on Print Edit WE's toolbar.
  4. Click on the Cancel button in the print preview panel.

Actual results:

  1. Web page opens.
  2. Print Edit WE starts correctly.
  3. The print preview panel appears, but displays blank pages.
  4. The print preview panel closes and a blank 'about:printpreview' page is displayed.

Expected results:

  1. The print preview panel should display the pages of the web page.
  2. The print preview panel shoud close and return to editing the web page.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Printing: Output
Product: Firefox → Core

Thanks for filing. I think this may be fixed by bug 1636728 (will test with that patch ASAP).

Blocks: 1631440

One thing I forgot to mention - I'm not convinced that the Print Preview Redesign is visually an improvement.

On my system, in portrait orientation:

  • with the new design, the size of the displayed page is 416mm x 540mm and does not scale with the window size.
  • with the old design, the size of the displayed page is 685mm x 967 mm and scales with the window size.

In portrait orientation, the size of the displayed page with the new design is only 37% compared to the old design. In landscape orientation, this falls to 18%.

For anyone who wants to see in some detail how the pages will look, or for anyone with visual impairment, the new design will be much less useful and much more difficult to see.

Is it not possible to make the new print preview panel scale with the window size and change apsect ratio depending on the orientation?

Flags: needinfo?(emilio)

Ah, blerg, this is just the tabs API poking directly at the old implementation of print preview.

Yes, resizing the new panel is on the works, that's bug 1654962. (Sorry, previous comment mid-aired with yours)

Flags: needinfo?(emilio)

afterprint is fixed with bug 1636728. I'll fix the web-extensions API.

Just for reference, is it fine if the printPreview result promise only resolves after the user has closed the dialog? Right now it resolves at a rather odd time.

Assignee: nobody → emilio
Flags: needinfo?(dw-dev)

Return a promise from PrintUtils.printPreview when the cloning of the
document is ready. Alternatively, we could resolve when the dialog
closes, though this is closer to what the current code does.

Depends on D87966

Depends on: 1636728

From the point of view of Print Edit WE, it doesn't matter when the promise is resolved. After calling tabs.printPreview(), Print Edit WE just waits for the 'afterprint' event, when the dialog is closed, and then resumes.

I think it would be best for the tabs.printPreview() promise to be resolved when the cloning of the document is complete, which is similar to the currrent behaviour.

However, with the experiment enabled, I have noticed that 'File > Print' also opens the new print preview panel.

This leads to several implications and questions

  1. It means that the tabs.print() API will also open the new print panel.
  2. So do we still need both the tabs.print() and the tabs.printPreview() APIs? (Note: Chrome doesn't support either of these APIs)
  3. If we don't need both APIs, which one should we keep and when should it be resolved?
  4. Will the old 'Print' dialog will disappear completely?

Finally, with the old print preview and print functionality, there were many web pages where the preview and printed document only contained the first page or first few pages and often contained many blank pages. Does the new print preview design fix these problems?

(In reply to dw-dev from comment #8)

From the point of view of Print Edit WE, it doesn't matter when the promise is resolved. After calling tabs.printPreview(), Print Edit WE just waits for the 'afterprint' event, when the dialog is closed, and then resumes.

I think it would be best for the tabs.printPreview() promise to be resolved when the cloning of the document is complete, which is similar to the currrent behaviour.

However, with the experiment enabled, I have noticed that 'File > Print' also opens the new print preview panel.

This leads to several implications and questions

  1. It means that the tabs.print() API will also open the new print panel.
  2. So do we still need both the tabs.print() and the tabs.printPreview() APIs? (Note: Chrome doesn't support either of these APIs)

Probably, or they should be kept as an alias of each other...

  1. If we don't need both APIs, which one should we keep and when should it be resolved?

That's not my call at all, I'm not familiar with the process to add / deprecate web extension APIs :)

  1. Will the old 'Print' dialog will disappear completely?

The old regular print dialog can still be reached via "Print using the system dialog". The old print preview page yes, it'll go away completely.

Finally, with the old print preview and print functionality, there were many web pages where the preview and printed document only contained the first page or first few pages and often contained many blank pages. Does the new print preview design fix these problems?

That seems like a problem in the layout side of stuff. Please do file them as separate bugs. We've fixed a bunch of these problems, but they should be looked case by case, as the root cause of each page with missing content may be different.

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

  1. So do we still need both the tabs.print() and the tabs.printPreview() APIs? (Note: Chrome doesn't support either of these APIs)
  2. If we don't need both APIs, which one should we keep and when should it be resolved?

That's not my call at all, I'm not familiar with the process to add / deprecate web extension APIs :)

As you may know, I did the original implementation of tabs.print() and tabs.printPreview().

I think it would be more logical to change tabs.print() so that it still calls the old regular print dialog.

This should be checked with Shane Caraveo.


Finally, with the old print preview and print functionality, there were many web pages where the preview and printed document only contained the first page or first few pages and often contained many blank pages. Does the new print preview design fix these problems?

That seems like a problem in the layout side of stuff. Please do file them as separate bugs. We've fixed a bunch of these problems, but they should be looked case by case, as the root cause of each page with missing content may be different.

I do report them if I see any that I think are new. And Print Edit WE includes a command that fixes some of the common cases.

Flags: needinfo?(dw-dev)

(In reply to dw-dev from comment #10)

As you may know, I did the original implementation of tabs.print() and tabs.printPreview().

I think it would be more logical to change tabs.print() so that it still calls the old regular print dialog.

This should be checked with Shane Caraveo.

I think code wise is just a matter of changing this line to call PrintUtils.printWindow instead.

Shane,

The Firefox Nightly 'Print Preview Redesign' experiment will result in both 'File > Print' and 'File Print Preview' opening the new print preview panel, which has a link to the old sytem 'Print' dialog.

As things stand, both tabs.print() and tabs.printPreview() will open the new print preview panel.

However, I think it would be more logical for tabs.printPreview() to open the new print preview panel, and for tabs.print() to open the old system 'Print' dialog (as it does at present). This would require a small change to the tabs.print() code as mentioned in Comment 11.

Do you agree with changing the tabs.print() code so that is still opens the old system 'Print' dialog?

Flags: needinfo?(mixedpuppy)
Severity: -- → S2
Priority: -- → P1

Either option sounds reasonable to me, but if we're changing behavior, even just when promises are resolved, please update MDN and ni? :Fallen to mention it in regular blog post.

for tabs.print() to open the old system 'Print' dialog (as it does at present).

If it's simple, I'd keep the same behavior here, and only change the behavior in tabs.printPreview.

However, if for some reason that really doesn't make sense from a Firefox perspective (I'll defer to @emilio and @mstriemer for that), then aliasing tabs.printPreview to tabs.print and marking tabs.printPreview as deprecated in the schema is probably a better path. edit: IOW, if Firefox will move away from supporting the system print dialog, then tabs.print should be the api to open the new ui.

Flags: needinfo?(mixedpuppy)

I agree that it seems silly to have both APIs but only expose one
behavior, and this makes tabs.print() keep doing what it's doing now.

Depends on D88013

I think it would be better to have tabs.print() go through the tab modal version since it provides a better experience in most cases than the system dialog. At that point the tabs.printPreview() version could be deprecated.

I understand the desire to open the dialog directly, so perhaps an argument could be added to do that? tabs.print({ useSystemDialog: true })

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c4f1618b72c1 Fix tabs.printPreview API with the new print preview design. r=mstriemer,mixedpuppy
Blocks: 1661241

Given there's no consensus on the last bit per the above I moved the patch to a different bug.

Comment on attachment 9172019 [details]
Bug 1660739 - Make tabs.print() keep using the system print dialog even when the new print UI is enabled. r=mstriemer!,mixedpuppy!

Revision D88201 was moved to bug 1661241. Setting attachment 9172019 [details] to obsolete.

Attachment #9172019 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

Reminder to request Beta approval.

Flags: needinfo?(emilio)

Comment on attachment 9171663 [details]
Bug 1660739 - Fix tabs.printPreview API with the new print preview design. r=mstriemer

Beta/Release Uplift Approval Request

  • User impact if declined: Some addons, including recommended ones, will be broken.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: see comment 0
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Relatively straight-forward change to the print stuff to support the WebExtensions API.
  • String changes made/needed: none
Flags: needinfo?(emilio)
Attachment #9171663 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Comment on attachment 9171663 [details]
Bug 1660739 - Fix tabs.printPreview API with the new print preview design. r=mstriemer

approved for 81.0b3

Attachment #9171663 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9171663 [details]
Bug 1660739 - Fix tabs.printPreview API with the new print preview design. r=mstriemer

Scratch that, this depends on bug 1636728 which isn't in beta (and has open regressions).

Attachment #9171663 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?

Is the intention to make the Print Preview Redesign the default print/preview behaviour in Firefox 81.0?

If so, this means that the tabs.printPreview() API would not work in Firefox 81.0, which is unacceptable.

Flags: needinfo?(emilio)

I don't think there's a plan to make it on by default, but it is intended to be on for a percentage of the population, so I agree. I can make a patch that doesn't depend on bug 1636728.

Flags: needinfo?(emilio)
Attached file Rebased patch for beta. (obsolete) (deleted) —

See comment 22 for the uplift request..

Attachment #9172427 - Flags: approval-mozilla-beta?
Whiteboard: [print2020_v81]

(In reply to dw-dev from comment #25)

Is the intention to make the Print Preview Redesign the default print/preview behaviour in Firefox 81.0?

If so, this means that the tabs.printPreview() API would not work in Firefox 81.0, which is unacceptable.

Why do you say it wouldn't work? The redesign will replace the old print preview.

Flags: needinfo?(dw-dev)

I think comment 25 is just about uplifting to beta afaict.

Mark,

My point was that, with the Print Preview Redesign experiment enabled in Firefox 81.0b3, the tabs.printPreview() API shows the new print preview panel, but all of the pages are blank. If the original patch to fix this problem wasn't going to be uplifted to Firefox 81 beta, then I felt that this regression was unacceptable.

Emilio has now rebased the patch and if this is uplifted to Firefox 81 beta then my concern goes away.

Attachment #9171663 - Flags: approval-mozilla-beta?
Attached patch rebased again. (deleted) — Splinter Review
Attachment #9172427 - Attachment is obsolete: true
Attachment #9172427 - Flags: approval-mozilla-beta?
Attachment #9172671 - Flags: approval-mozilla-beta?

Comment on attachment 9172671 [details] [diff] [review]
rebased again.

Approved for 81.0b4.

Attachment #9172671 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Contact: emil.ghitta

Reproduced the issue mentioned in comment 0 using Firefox 82.0a1 (BuildId:20200824215021).

This issue is verified fixed using Firefox 82.0a1 (BuildId:20200831091558) and Firefox 81.0b4 (BuildId:20200829200810) on Windows 10 64bit, macOS 10.14 & Ubuntu 20.04

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Comment on attachment 9171663 [details]
Bug 1660739 - Fix tabs.printPreview API with the new print preview design. r=mstriemer

Beta/Release Uplift Approval Request

  • User impact if declined: Extensions breakage.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Been on Nightly for a while and verified by reporter
  • String changes made/needed:
Attachment #9171663 - Flags: approval-mozilla-beta?

Comment on attachment 9171663 [details]
Bug 1660739 - Fix tabs.printPreview API with the new print preview design. r=mstriemer

Approved for 81.0b6 (no functional change, but we had to re-do this on account of bug 1660739 being uplifted to Beta).

Attachment #9171663 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(dw-dev)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: