tabs.saveAsPDF() doesn't work on Mac OS X

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: General
P1
normal
RESOLVED FIXED
16 days ago
2 days ago

People

(Reporter: wbamberg, Assigned: dw-dev, Mentored)

Tracking

({dev-doc-needed})

unspecified
mozilla57
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

16 days ago
Created attachment 8893080 [details]
saveaspdf.zip

See bug 1269300, comment 73.

I've attached a sample add-on: it just adds a browser action, and has a background script like this:

function handleClick() {
  browser.tabs.saveAsPDF({});
}

browser.browserAction.onClicked.addListener(handleClick);

In Firefox 56.0a1, when I click the browser action (say in http://example.com/), I get the "Save As" dialog. I select a name and location and choose "Save". A file does appear with the right name and location, but it's empty. 

If I access the same feature using the browser's built-in UI: Ctrl-P/select "Save As PDF..." then the PDF is saved as expected.

Apparently it does work in Windows.
(Assignee)

Comment 1

14 days ago
Bob Owen's Comment 87 in Bug 1269300 supports my recollection in Comment 82 in Bug 1269300.

I think it is therefore likely that saveAsPDF() cannot easily be made to work on Mac OS X.

It would be good if we could get a definitive view on whether the print to PDF functionality was ever implemented for Mac OS X.
We'll need to figure out a resolution on this soon.
Priority: -- → P1
(Assignee)

Comment 3

11 days ago
I think we need a definitive view on whether the print to PDF functionality was ever implemented for Mac OS X.

Who would have this definitive view?

When we have the answer, then:

1. If the functionality was never implemented, we need to document that saveAsPDF() does not work on Mac OS X. This has already been actioned. We may also want to return an error in this case.

2. If the functionality was implemented, then presumably there is a bug that needs fixing, which I suspect may not be a priority.  So the immediate action would probably be the same as for (1) above.
(Assignee)

Comment 4

10 days ago
Created attachment 8894959 [details]
ext-tabs.js - with changes to printPreview() and saveAsPDF()
(Assignee)

Comment 5

10 days ago
Created attachment 8894960 [details]
tabs.json - with changes to saveAsPDF() return values
(Assignee)

Comment 6

10 days ago
Shane,

I think I've made quite a lot of progress:

1. I've released the latest version (19.0) of my Print Edit WE add-on and on Window 10 this works perfectly with the new print(), printPreview() and saveAsPDF() API's.

2. I've searched on DXR and like Bob Owen I cannot find any mention of the 'kOutputFormatPDF' setting in conjunction with Mac OS X.

3. I've looked back through my development notes for my original (XUL/XPCOM) Print Edit add-on. The 'Save As PDF' feature was introduced in Print Edit 12.1 in April 2014 and my development notes (based on extensive testing) state:

        /*  kOutputFormatPDF print setting:                                         */
        /*                                                                          */
        /*  - works on Windows Vista/7/8 with Firefox 3.6 & Firefox 28.0+           */
        /*                                                                          */
        /*  - works on Linux with Firefox 3.6+                                      */
        /*                                                                          */
        /*  - does not work on Mac OS X                                             */

4. I think we should proceed on the basis that the print to PDF functionality was never implemented for Mac OS X and that it is not likely to be in the near future. Therefore, saveAsPDF() should return an error message in lastError if called on Mac OS X.

5. I've discovered why I had problems with "throw new ExtensionError(...)" not returning an error message in lastError if 
printPreview() fails. A "throw" doesn't result in a rejected promise if it is thrown from within an asynchronous callback, such as the "onEntered" function inside the promise in printPreview().

There is a discussion here: https://stackoverflow.com/questions/33445415/javascript-promises-reject-vs-throw

6. I've produced a new version of ext-tabs.js (see attachment) with the following changes:

- Added lines 800-803 to return error message in lastError if saveAsPDF() called on Mac OS X.

- Changed lines 774 & 784 to return error message in lastError if printPreview() fails.

7. I've produced a new version of tabs.json (see attachment) that amends the documented return status values to correctly reflect those in the code: "Not saved" and "Not replaced".


If you are okay with these changes, I will go ahead and submit a formal patch for review. Please let me know.
Flags: needinfo?(mixedpuppy)
Lets go forward with the patch.

We should ni someone who worked on pdf.js and see if there is any api there we could use (could be a followup).
Flags: needinfo?(mixedpuppy) → needinfo?(dw-dev)
(Assignee)

Comment 8

10 days ago
Okay, will do.
Flags: needinfo?(dw-dev)
Assignee: nobody → dw-dev
Mentor: mixedpuppy@gmail.com
Comment hidden (mozreview-request)
Lets be sure to update docs on this.
Keywords: dev-doc-needed

Comment 11

7 days ago
mozreview-review
Comment on attachment 8896384 [details]
Bug 1386805 - changes to saveAsPDF() and printPreview();

https://reviewboard.mozilla.org/r/167634/#review172848
Attachment #8896384 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 12

7 days ago
You beat me to it.  Anyway, here is a quick commentary on this patch:

1. The changes made in this patch are the same as those in the previous attachments, except that the status values returned by saveAsPDF() have been changed to: saved, replaced, canceled, not_saved, not_replaced.  I changed to these values after looking at a lot of the other WebExtensions API's, which in almost all cases return lower case values with underscores instead of spaces.

2. eslint has been run on the changed files with no errors.

2. The printPreview test file passes all tests.

3. A build with this patch works correctly with Print Edit WE 19.0.

4. Please could you submit this patch to the Try Server?  And if everything goes well, then add the 'checkin-needed' flag.
(Assignee)

Comment 13

7 days ago
Will,

Please could you update the documentation for saveAsPDF() to show the new return status values: 

    "saved", "replaced", "canceled", "not_saved", "not_replaced"

I think it would also be worth updating the documentation for printPreview() to mention that an add-on can detect exit from print preview by listening for the "afterprint" event, for example:

    window.addEventListener("afterprint",resumeFunction,false);
Flags: needinfo?(wbamberg)
(Assignee)

Updated

7 days ago
Flags: needinfo?(mixedpuppy)
(Reporter)

Comment 14

7 days ago
Sure thing, thanks dw-dev. Do you know if these changes will be uplifted to Firefox 56, or will they take effect from 57?
Flags: needinfo?(wbamberg)
(Assignee)

Comment 15

7 days ago
That question did occur to me, but I don't know the answer.

My feeling is that it would be low risk to uplift to Firefox 56, but if that is difficult to do I think it could wait until Firefox 57.

Probably best to get Shane's view.
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

7 days ago
Flags: needinfo?(mixedpuppy)
Yeah, request an uplift.
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 17

7 days ago
Who requests's the uplift?

If it's me, how is that done?
Flags: needinfo?(mixedpuppy)
After it lands on central, go into "details" for the attachment (not reviewboard) and do approval‑mozilla‑beta? You'll need to fill out the questions.  If you need help on that let me know.
Flags: needinfo?(mixedpuppy)
Keywords: checkin-needed

Comment 19

7 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5279a1edf8ae
changes to saveAsPDF() and printPreview(); r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5279a1edf8ae
Status: NEW → RESOLVED
Last Resolved: 6 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
dw-dev, can you verify on nightly then request uplift?
Flags: needinfo?(dw-dev)
(Assignee)

Comment 22

4 days ago
Comment on attachment 8896384 [details]
Bug 1386805 - changes to saveAsPDF() and printPreview();

Approval Request Comment
[Feature/Bug causing the regression]: Print-to-PDF print setting (kOutputFormatPDF) is not implemented for Mac OS X.
[User impact if declined]: Calls to browser.tabs.saveAsPDF() on Mac OS X will fail silently.
[Is this code covered by automated tests?]: printPreview() changes - yes.  saveAsPDF() changes - no.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just a few simple changes that are well understood.
[String changes made/needed]: None.
Flags: needinfo?(dw-dev)
Attachment #8896384 - Flags: approval-mozilla-beta?
status-firefox56: --- → affected
Comment on attachment 8896384 [details]
Bug 1386805 - changes to saveAsPDF() and printPreview();

Fix browser.tabs.saveAsPDF issue on Mac. Beta56+. Should be in 56.0b4.
Attachment #8896384 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 24

2 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/1a169abe9d0a
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.