Note: There are a few cases of duplicates in user autocompletion which are being worked on.

"Save as..." File menu entry or Ctrl+S produces unexpected results when having a PDF file opened within PDF Viewer

VERIFIED FIXED in Firefox 20

Status

()

Firefox
PDF Viewer
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: antistress, Assigned: neil@parkwaycc.co.uk)

Tracking

({dataloss})

Trunk
Firefox 22
dataloss
Points:
---

Firefox Tracking Flags

(firefox19- wontfix, firefox20+ verified, firefox21+ verified, firefox22+ verified)

Details

(Whiteboard: [testday-20120622] [pdfjs-c-ff-integration]https://github.com/mozilla/pdf.js/pull/2861)

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120324 Firefox/14.0a1
Build ID: 20120324031100

Steps to reproduce:

Open a PDF file using new integrated PDF Viewer, then use the "Save as..." Firefox functionnality 


Actual results:

The saved file can not be used (not a pdf file)


Expected results:

The saved file should be the original PDF file = the same that user can get using the Download button from the PDF Viewer dedicated toolbar
(Reporter)

Updated

5 years ago
Blocks: 714712
Component: Untriaged → PDF Viewer
OS: Linux → All
Hardware: x86_64 → All

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

5 years ago
QA Contact: untriaged → pdf-viewer

Updated

5 years ago
Keywords: dataloss

Comment 1

5 years ago
The file is saved as HTML (with .pdf.html), and the Download Manager always says its size is 53,5 KB.
Whiteboard: [testday-20120622]

Comment 2

5 years ago
Related: bug 744379.

Updated

5 years ago
See Also: → bug 744379

Updated

5 years ago
Duplicate of this bug: 744379
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20120712 Firefox/15.0a2

In addition to comment #1: opening the pdf.html file, it contains only blank pages

Updated

5 years ago
Severity: normal → critical

Updated

5 years ago
Whiteboard: [testday-20120622] → [testday-20120622] [pdfjs-c-ff-integration]

Updated

5 years ago
Duplicate of this bug: 812755

Updated

5 years ago
Duplicate of this bug: 826939
Duplicate of this bug: 828231
Duplicate of this bug: 828681
Duplicate of this bug: 829931

Updated

5 years ago
Summary: "Save as..." File menu entry produces inexpected result when having a PDF file opened within PDF Viewer → "Save as..." File menu entry or Ctrl+S produces unexpected results when having a PDF file opened within PDF Viewer
Duplicate of this bug: 830577

Comment 11

5 years ago
The Download button doesn't work either.

Comment 12

5 years ago
My bug talks about the download button not working either maybe it should not be duplicate?

Comment 13

5 years ago
Ignore my previous comments… I was doing it wrong…
This works for me with Firefox 19.0b5 and the latest Nightly. Can someone please confirm?
It doesn't work for me on either of those builds (Mac 10.7.5, Ubuntu 12.10 x64, Win 7 x64): the saved document is html and has only blank pages
Created attachment 711405 [details]
Screenshot of the bug

Screenshot of what I see when I hit Command-S while viewing a PDF. Note the .pdf.html extension and the 'Format: Web Page, Complete'.
See previous comment for a screenshot.

STR:

Start firefox with new/clean profile
Open URL pointing to PDF file
Hit command-s

Expected:

PDF is saved

Actual:

A file called foo.pdf.html and directory called foo.pdf_files are created. The foo.pdf_files directory contain things like pdf.js
Same here, except that on Windows it shows only the name.pdf (without .html) in the save as dialog, but the result is the same
I'm not clear on why this has dataloss keyword and critical severity. Yes, the access key does not work as expected in the context of the PDF Viewer, but it's not like the document is lost. You can still click on the download button (I know it was stated a while back that the download button wasn't working but it does work in current builds -- at least on my end). 

We should probably be a bit smarter to trigger the download behaviour when Save Page As is triggered from a tab with a PDF loaded, but I don't think that was in scope for Firefox 19.

Would someone mind providing justification for the severity?

Comment 20

5 years ago
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #19)
> Would someone mind providing justification for the severity?

Dataloss can happen if you don’t make sure that the file was saved properly.  The file can become unavailable or disappear (even be deleted by yourself).
(In reply to Aleksej [:Aleksej] from comment #20)
> Dataloss can happen if you don’t make sure that the file was saved properly.
> The file can become unavailable or disappear (even be deleted by yourself).

I think that's a bit of a stretch to consider this a "dataloss" bug. I would agree if the download button wasn't working, as earlier reported. But this is no longer the case. I'm open to be convinced otherwise but I have yet to see a compelling argument.

Comment 22

5 years ago
This should be fixed before Fx 19 with PDF.js is released (regardless of severity), or PDF.js needs to be dealyed once again. The current behaviour is really confusing. The download button in the PDF.js bar may not be obvious for many users thus they may try to download the displayed PDF via the menu bar (many esp. not tech-savvy users use menu items even when there are buttons available because they learned it that way) – which doesn't download the PDF at all (but rather the PDF.js code). Plus, this bug is probably (AFAICS) not that easy to solve (i. e. requiring quite a few code changes, compared to just a few LOC). 

Considering Bug 806057 and Bug 761539 (both supposedly fixed upstream), I think we should delay PDF.js for release and let all the fixes be tested on Beta 20 (we're just too close to release to fix and test all those bugs/code changes). Otherwise, this will be a PR disaster. 

This should be blocker for Fx 19 (or rather PDF.js pref'd off for Fx 19).

Comment 23

5 years ago
(In reply to Florian Bender from comment #22)
> is really confusing. The download button in the PDF.js bar may not be obvious for many users thus they may try to download the displayed PDF via

When I want to click the download button, I always wonder if I am looking at the right button. bug 762371
See Also: → bug 808162

Updated

5 years ago
See Also: bug 744379

Comment 24

5 years ago
Yeah, the download button is not obvious at all. It should be a floppy disk symbol and preferably the word “Save” beside it in text, since there is *plenty* of space.

Updated

5 years ago
Duplicate of this bug: 842969

Comment 26

5 years ago
Just for the record, this bug also applies to right click -> save page as. 

Testimonial from my (tech-savvy) gf: "I hate the PDF viewer because it doesn't let me save my PDFs." She was trying to use the menubar -> save as thus saving the HTML (in a file with the .pdf extension) + ressources. 

This needs to be fixed, it's a huge UX issue for many many Fx users, and PDF.js needs to be disabled for Fx 19. Please!

Comment 27

5 years ago
(In reply to Florian Bender from comment #26)
> Testimonial from my (tech-savvy) gf: "I hate the PDF viewer because it
> doesn't let me save my PDFs." She was trying to use the menubar -> save as
> thus saving the HTML (in a file with the .pdf extension) + ressources. 
You can use the download button (down arrow) on the right of the PDF Viewer header.

> PDF.js needs to be disabled for Fx 19.
Too late.

Comment 28

5 years ago
(In reply to Scoobidiver from comment #27)
> You can use the download button (down arrow) on the right of the PDF Viewer
> header.
I do know that, but most users do not (or at least will need some time trying to figure this out). I showed it to her, but I ended up disabling the viewer for her. This stuff needs to be as intuitive as possible, and currently it's not. 

(In reply to Scoobidiver from comment #27)
> > PDF.js needs to be disabled for Fx 19.
> Too late.
Yes and no. There's already a hotfix available (Bug 839239) which just needs to ship.

Comment 29

5 years ago
Is there any way to disable the pdf viewer? It crashes the browser a lot. I want it the way it was. The way that worked. With the plugin.

Comment 30

5 years ago
(In reply to ranunculoid from comment #29)
> Is there any way to disable the pdf viewer? It crashes the browser a lot. I
> want it the way it was. The way that worked. With the plugin.
Please ask your question in the support forum to disable PDF Viewer or file a new bug to report crashes if there is none related (see the Related Bugs section in crash reports of about:crashes).

Comment 31

5 years ago
Some user stories: 
https://input.mozilla.org/opinion/3579955
https://input.mozilla.org/opinion/3579939 (probably due to PDF.js)
https://input.mozilla.org/opinion/3579945

(This is just the last 10 minutes.)

Comment 32

5 years ago
Asking for tracking as it confuses many users (see also duplicates).
tracking-firefox19: --- → ?
Given the volume of user complaints (and the fact that we should be able to resolve fairly easily), we'll track for upcoming releases.
status-firefox19: --- → wontfix
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
tracking-firefox19: ? → -
tracking-firefox20: --- → +
tracking-firefox21: --- → +
tracking-firefox22: --- → +
Brian - let us know if the PDF.js team needs any tips from the FF frontend team about how to accomplish this integration (CC Gavin).
Assignee: nobody → bdahl
We looked into to doing this awhile back and if I remember correctly bz said that it wasn't a trivial task.  It would probably be best if we could get someone from the FF team to work on this or at least give us some concrete pointers on implementing this.

(looking for the previous conversation now)

Comment 36

5 years ago
Actually, I think capturing the "Ctrl+s" short-cut would be trivial, and a fix like this could be easily uplifted to beta?
Yes, it would still be broken from the Firefox menu (which should be fixed later), but it's what web-apps are expected to do: make common shortcuts work as expected.

Comment 37

5 years ago
> but it's what web-apps are expected to do: make common shortcuts work as expected.

For the user, pdf.js is not a web-app - its integrated into the firefox, and therefore is expected to function exactly as a native app would do.

Comment 38

5 years ago
I like the idea of fixing at least the shortcut – for Release (if there will be a 19.0.1). However for Fx 20 this bug should be fixed thoroughly.
Created attachment 718191 [details] [diff] [review]
toolkit patch

This adds a simple JS module that allows pdf.js to easily override saveDocument on a per-document basis. The idea here is that pdf.js would set PDFJSSaveAs.callback to a callback that:
- checks whether the document in question is a pdf.js document (not sure how this should happen exactly)
- if so, return true, and then handle the document saving itself
- otherwise, returns false to let the toolkit code perform the default save

The callback in question is passed:
- the chrome window where saveDocument was invoked
- the content document that was intended to be saved (this could be a sub frame, in the case of "Save frame as...")

Brendan, is this sufficient for PDF.js to fix this properly?
Attachment #718191 - Flags: feedback?(neil)
Attachment #718191 - Flags: feedback?(bdahl)
Created attachment 719071 [details] [diff] [review]
toolkit patch v2

Fixes one minor bug with the missing 'this' PDFJSSaveAs.jsm line 33.

Gavin,
We're going to land the fixes on the pdf.js side then bring over a full update of pdf.js for mozilla central in another bug.  We'll then create cherry-picked patches for uplift in this bug.
Attachment #718191 - Attachment is obsolete: true
Attachment #718191 - Flags: feedback?(neil)
Attachment #718191 - Flags: feedback?(bdahl)
Attachment #719071 - Flags: review?(gavin.sharp)
Attachment #719071 - Flags: feedback?(neil)
Comment on attachment 719071 [details] [diff] [review]
toolkit patch v2

Given that I wrote most of this I don't think I should review it, but thanks for actually testing it :)
Attachment #719071 - Flags: review?(neil)
Attachment #719071 - Flags: review?(gavin.sharp)
Attachment #719071 - Flags: feedback?(neil)
Attachment #719071 - Flags: feedback+

Comment 42

5 years ago
[20130222-testday] edwardb   linux-32bit

A bandaid solution is found:

While executing https://moztrap.mozilla.org/runtests/run/847/env/150/ case #665, 

selected "All Files" filter, and forcing .pdf as the file ending to be saved.

As a result, the file IS saved as a .pdf and can be opened by a pdf reader / editor.

it's a step in the right direction, but the fact that the dialog box doesn't display ".pdf" as the default filter to choose means that work still is needed to eradicate this bug.
Comment on attachment 719071 [details] [diff] [review]
toolkit patch v2

>--- a/toolkit/content/contentAreaUtils.js
>+++ b/toolkit/content/contentAreaUtils.js
>@@ -1,15 +1,16 @@
> # -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- 
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> Components.utils.import("resource://gre/modules/Services.jsm");
> Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
>+Components.utils.import("resource://gre/modules/PDFJSSaveAs.jsm");
> 
> var ContentAreaUtils = {
> 
>   // this is for backwards compatibility.
>   get ioService() {
>     return Services.io;
>   },

contentAreaUtils.js is loaded in many chrome documents that don't need access to PDFJSSaveAs. As a private utility it should be tacked on the ContentAreaUtils object.
Yeah, good point. We should fix that.
Created attachment 719736 [details] [diff] [review]
toolkit patch v3
Attachment #719071 - Attachment is obsolete: true
Attachment #719071 - Flags: review?(neil)
Attachment #719736 - Flags: review?(neil)
(Assignee)

Comment 46

5 years ago
Comment on attachment 719736 [details] [diff] [review]
toolkit patch v3

After a discussion from bz I've found a better way forward on this. In particular, a full-page PDF plugin document has a contentType of application/pdf which saves as you would expect. So, we need to get pdfjs to store a custom property on the channel which we read back in StartDocumentLoad when we set the content type.
Attachment #719736 - Flags: review?(neil)
(Assignee)

Comment 47

5 years ago
Created attachment 719750 [details] [diff] [review]
Proposed patch
Assignee: bdahl → neil
Attachment #719736 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #719750 - Flags: review?(bzbarsky)
(Assignee)

Comment 48

5 years ago
Created attachment 719752 [details] [diff] [review]
PdfStreamConverter.js changes

I've never used github so I don't know how to submit this change.
To further complicate things in the near future we may not always be using the original stream to fetch the PDF.  We'll be doing a second request to see if the server supports range requests.  If it does we'll then be cancelling the original request and then downloading chunks of the file with an XHR in the worker.  This is so we can render pages without downloading the whole file.

Comment 50

5 years ago
That's a good thing to keep in mind. On save, the original stream will have to be refetched if the second (range-supported) request is active.

Comment 51

5 years ago
Comment on attachment 719750 [details] [diff] [review]
Proposed patch

I can live with this, I think...
Attachment #719750 - Flags: review?(bzbarsky) → review+
(In reply to neil@parkwaycc.co.uk from comment #48)
> Created attachment 719752 [details] [diff] [review]
> PdfStreamConverter.js changes
> 
> I've never used github so I don't know how to submit this change.

I'll upstream the changes and they'll come to MC in a few days when we update the pdf component.  As for the uplift pieces I'll create separate patches and nominate in this bug.

Updated

5 years ago
Whiteboard: [testday-20120622] [pdfjs-c-ff-integration] → [testday-20120622] [pdfjs-c-ff-integration]https://github.com/mozilla/pdf.js/pull/2861
(Assignee)

Comment 53

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd1a9fdf8fb
https://hg.mozilla.org/mozilla-central/rev/1cd1a9fdf8fb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox22: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Keywords: verifyme
(In reply to neil@parkwaycc.co.uk from comment #53)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd1a9fdf8fb

Can you please nominate this for uplift already(asap) if we fine with the testing/bake-time it has had on m-c ? We would like to get it into Fx20 beta3 which is going to build tomorrow.
Created attachment 720979 [details] [diff] [review]
aurora save as support

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: save as doesn't work properly for pdfs
Testing completed (on m-c, etc.): only the changes to nsDocument have been tested on m-c, local testing of the full patch
Risk to taking this patch (and alternatives if risky): I can only speak for the pdf.js side of things which is very low risk, for the changes to nsDocument someone else will have to comment.
String or UUID changes made by this patch: none
Attachment #720979 - Flags: review?(bzbarsky)
Attachment #720979 - Flags: approval-mozilla-aurora?
Created attachment 720980 [details] [diff] [review]
beta save as support

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: save as doesn't work properly for pdfs
Testing completed (on m-c, etc.): only the changes to nsDocument have been tested on m-c, local testing of the full patch
Risk to taking this patch (and alternatives if risky): I can only speak for the pdf.js side of things which is very low risk, for the changes to nsDocument someone else will have to comment.
String or UUID changes made by this patch: none
Attachment #720980 - Flags: review?(bzbarsky)
Attachment #720980 - Flags: approval-mozilla-beta?
(In reply to Brendan Dahl from comment #57)
> Created attachment 720980 [details] [diff] [review]
> beta save as support

Looks like you attached the wrong patch for beta?

Comment 59

4 years ago
Comment on attachment 720979 [details] [diff] [review]
aurora save as support

r=me
Attachment #720979 - Flags: review?(bzbarsky) → review+

Comment 60

4 years ago
Comment on attachment 720980 [details] [diff] [review]
beta save as support

Definitely the wrong patch.
Attachment #720980 - Flags: review?(bzbarsky) → review-
Created attachment 721057 [details] [diff] [review]
beta save as support v2

Sorry about that.
Attachment #720980 - Attachment is obsolete: true
Attachment #720980 - Flags: approval-mozilla-beta?
Attachment #721057 - Flags: review?(bzbarsky)
Attachment #721057 - Flags: approval-mozilla-beta?

Comment 62

4 years ago
Comment on attachment 721057 [details] [diff] [review]
beta save as support v2

r=me
Attachment #721057 - Flags: review?(bzbarsky) → review+
Comment on attachment 721057 [details] [diff] [review]
beta save as support v2

Request to land by tomorrow(3/5) to make it into FX20 beta 3.
Attachment #721057 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

4 years ago
Attachment #720979 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/70dbd1a57c50
https://hg.mozilla.org/releases/mozilla-beta/rev/bbb75e4bd6ad
status-firefox20: affected → fixed
status-firefox21: affected → fixed
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0

Verified the fix on Firefox 20beta3: CTRL/CMD + S and "Save page" from file menu work as expected - the file is correctly saved as PDF.
status-firefox20: fixed → verified
QA Contact: mihaela.velimiroviciu
Duplicate of this bug: 849709

Updated

4 years ago
Duplicate of this bug: 846782

Comment 68

4 years ago
Verified as fixed on Friefox 21 Beta 1 (20130401192816):
Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:21.0) Gecko/20100101 Firefox/21.0
status-firefox21: fixed → verified

Comment 69

4 years ago
The problem seems related to the option chosen when saving the page containing the PDF.
If I choose "Save Web page as" --> "Web page, complete", then the saved PDF results corrupted (this behavior still persists in Firefox 21beta2). If I choose "Save Web pagee as" --> "All files", the saved PDF is OK both in Firefox 20 and Firefox 21beta2.
I'm on Mac OSX 10.8.2.
This bug is fixed. You shouldn't see the "Save Web pagee as" choice in the first place. If not, it is a different bug. Please file a new bug with a detailed information.
> You shouldn't see the "Save Web pagee as" choice
Correction: "Web page, complete" choice

Comment 72

4 years ago
Verified as fixed on Firefox 22 beta 1: 20130514181517.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20100101 Firefox/22.0
status-firefox22: fixed → verified
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.