Closed Bug 1386805 Opened 7 years ago Closed 7 years ago

Document that tabs.saveAsPDF() does not work on macOS and make it throw an exception if called

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox56 verified, firefox57 verified, firefox58 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox56 --- verified
firefox57 --- verified
firefox58 --- verified

People

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

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

Attached file 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.
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
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.
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)
Okay, will do.
Flags: needinfo?(dw-dev)
Assignee: nobody → dw-dev
Mentor: mixedpuppy
Lets be sure to update docs on this.
Keywords: dev-doc-needed
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+
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.
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)
Flags: needinfo?(mixedpuppy)
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)
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)
Flags: needinfo?(mixedpuppy)
Yeah, request an uplift.
Flags: needinfo?(mixedpuppy)
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
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
dw-dev, can you verify on nightly then request uplift?
Flags: needinfo?(dw-dev)
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?
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+
(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> We should ni someone who worked on pdf.js and see if there is any api there
> we could use (could be a followup).

I'm one of the core contributors to PDF.js too, so I can answer this.
The answer is no, PDF.js cannot be used to save a page as PDF, because PDF.js is only for PDF->canvas/SVG, and not for HTML/XML -> PDF.

When I press Cmd+P (print) on macOS, I do see a print to PDF option, so perhaps it is feasible to implement saveAsPDF on macOS too.
Will,

Just one comment ...

I think the ordering of the paragraphs at the start of the 'tabs.printPreview()' documentation is a little confusing.

I think it would be clearer if the "This is an asynchronous function that returns a Promise." paragraph is moved up so it is immediately after the "Opens print preview for the active tab." paragraph.

And perhaps the "An extension can detect ..." could start with "Note: An extension can detect ...".

Otherwise, I'm happy.
Flags: needinfo?(wbamberg)
Thanks for checking!

> I think the ordering of the paragraphs at the start of the 'tabs.printPreview()' documentation is a little confusing.

"This is an asynchronous function that returns a Promise." is a bit of boilerplate that lives at the end of the intro for every asynch function, so I wanted to keep it consistent. But you are right, it reads better the other way, so I've changed it. I didn't add "Note:" because I don't think it really adds anything.
Flags: needinfo?(wbamberg)
Attached file Bug1386805.zip
I can reproduce this issue on Firefox 55.0.3 (20170824053838) under Mac OS X 10.13.

I can see the next error in the browser console:  browser.tabs.saveAsPDF is not a function     background.js:4.

This issue is verified as fixed on Firefox 58.0a1 (2017-11-06), Firefox 57.0b14 (20171102181127) and  Firefox 56.0.2 (20171024165158) under Mac OS X 10.13. 

The next error is displayed in the browser console: Error: Not supported on Mac OS X     undefined.

Please see the attached video.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions

Having a bug with "tabs.saveAsPDF() doesn't work on Mac OS X" mark FIXED gave me the impression the bug had been fixed. You have to carefully read through the comments or look at the patch to realize that's not the case. I'm changing the title to avoid others being misled.

Summary: tabs.saveAsPDF() doesn't work on Mac OS X → Document that tabs.saveAsPDF() does not work on macOS and make it throw an exception if called
See Also: → 1653354

The MacOS restriction is mentioned in the tabs.saveAsPDF() documentation:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/saveAsPDF

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