Last Comment Bug 738952 - "Save as..." File menu entry or Ctrl+S produces unexpected results when having a PDF file opened within PDF Viewer
: "Save as..." File menu entry or Ctrl+S produces unexpected results when havin...
Status: VERIFIED FIXED
[testday-20120622] [pdfjs-c-ff-integr...
: dataloss
Product: Firefox
Classification: Client Software
Component: PDF Viewer (show other bugs)
: Trunk
: All All
: -- critical with 11 votes (vote)
: Firefox 22
Assigned To: neil@parkwaycc.co.uk
: Mihaela Velimiroviciu (:mihaelav)
Mentors:
: 744379 812755 826939 828231 828681 829931 830577 842969 846782 849709 (view as bug list)
Depends on:
Blocks: 714712
  Show dependency treegraph
 
Reported: 2012-03-24 10:51 PDT by antistress
Modified: 2013-09-17 07:18 PDT (History)
38 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
verified
+
verified
+
verified


Attachments
Screenshot of the bug (986.08 KB, image/png)
2013-02-07 10:10 PST, Stefan Arentz [:st3fan]
no flags Details
toolkit patch (3.43 KB, patch)
2013-02-25 17:51 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
toolkit patch v2 (3.44 KB, patch)
2013-02-27 09:58 PST, Brendan Dahl [:bdahl]
gavin.sharp: feedback+
Details | Diff | Review
toolkit patch v3 (3.53 KB, patch)
2013-02-28 15:52 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
Proposed patch (966 bytes, patch)
2013-02-28 16:22 PST, neil@parkwaycc.co.uk
bzbarsky: review+
Details | Diff | Review
PdfStreamConverter.js changes (1.37 KB, patch)
2013-02-28 16:25 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Review
aurora save as support (2.68 KB, patch)
2013-03-04 16:50 PST, Brendan Dahl [:bdahl]
bzbarsky: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Review
beta save as support (7.87 KB, patch)
2013-03-04 16:51 PST, Brendan Dahl [:bdahl]
bzbarsky: review-
Details | Diff | Review
beta save as support v2 (2.68 KB, patch)
2013-03-04 21:46 PST, Brendan Dahl [:bdahl]
bzbarsky: review+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Review

Description antistress 2012-03-24 10:51:00 PDT
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
Comment 1 [:Aleksej] 2012-06-22 03:30:23 PDT
The file is saved as HTML (with .pdf.html), and the Download Manager always says its size is 53,5 KB.
Comment 2 [:Aleksej] 2012-06-22 09:53:54 PDT
Related: bug 744379.
Comment 3 Stefan 2012-06-22 10:49:36 PDT
*** Bug 744379 has been marked as a duplicate of this bug. ***
Comment 4 Mihaela Velimiroviciu (:mihaelav) 2012-07-13 05:29:48 PDT
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
Comment 5 Brendan Dahl [:bdahl] 2012-11-19 13:04:10 PST
*** Bug 812755 has been marked as a duplicate of this bug. ***
Comment 6 Brendan Dahl [:bdahl] 2013-01-07 13:13:34 PST
*** Bug 826939 has been marked as a duplicate of this bug. ***
Comment 7 XtC4UaLL [:xtc4uall] 2013-01-09 14:02:44 PST
*** Bug 828231 has been marked as a duplicate of this bug. ***
Comment 8 Matthias Versen [:Matti] 2013-01-09 15:10:02 PST
*** Bug 828681 has been marked as a duplicate of this bug. ***
Comment 9 Matthias Versen [:Matti] 2013-01-12 06:12:57 PST
*** Bug 829931 has been marked as a duplicate of this bug. ***
Comment 10 Matthias Versen [:Matti] 2013-01-14 18:20:46 PST
*** Bug 830577 has been marked as a duplicate of this bug. ***
Comment 11 ranunculoid 2013-01-14 18:23:02 PST
The Download button doesn't work either.
Comment 12 ranunculoid 2013-01-14 18:26:13 PST
My bug talks about the download button not working either maybe it should not be duplicate?
Comment 13 ranunculoid 2013-01-14 19:25:53 PST
Ignore my previous comments… I was doing it wrong…
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-02-07 09:52:33 PST
This works for me with Firefox 19.0b5 and the latest Nightly. Can someone please confirm?
Comment 15 Mihaela Velimiroviciu (:mihaelav) 2013-02-07 10:03:15 PST
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
Comment 16 Stefan Arentz [:st3fan] 2013-02-07 10:10:26 PST
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'.
Comment 17 Stefan Arentz [:st3fan] 2013-02-07 10:13:15 PST
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
Comment 18 Mihaela Velimiroviciu (:mihaelav) 2013-02-07 10:15:28 PST
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
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-02-07 16:58:54 PST
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 [:Aleksej] 2013-02-07 22:19:41 PST
(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).
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-02-08 09:45:51 PST
(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 Florian Bender 2013-02-09 09:11:03 PST
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 [:Aleksej] 2013-02-09 09:19:54 PST
(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
Comment 24 ranunculoid 2013-02-10 06:33:38 PST
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.
Comment 25 Clemens Eisserer 2013-02-20 11:25:02 PST
*** Bug 842969 has been marked as a duplicate of this bug. ***
Comment 26 Florian Bender 2013-02-21 03:04:08 PST
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 Scoobidiver (away) 2013-02-21 03:09:44 PST
(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 Florian Bender 2013-02-21 03:29:50 PST
(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 ranunculoid 2013-02-21 06:15:51 PST
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 Scoobidiver (away) 2013-02-21 06:38:11 PST
(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 Florian Bender 2013-02-21 08:52:51 PST
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 Scoobidiver (away) 2013-02-21 08:54:48 PST
Asking for tracking as it confuses many users (see also duplicates).
Comment 33 Alex Keybl [:akeybl] 2013-02-22 12:11:08 PST
Given the volume of user complaints (and the fact that we should be able to resolve fairly easily), we'll track for upcoming releases.
Comment 34 Alex Keybl [:akeybl] 2013-02-22 12:12:46 PST
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).
Comment 35 Brendan Dahl [:bdahl] 2013-02-22 12:23:54 PST
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 Yann Brelière 2013-02-24 11:13:35 PST
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 Clemens Eisserer 2013-02-24 13:19:13 PST
> 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 Florian Bender 2013-02-24 14:03:50 PST
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.
Comment 39 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-25 17:51:19 PST
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?
Comment 40 Brendan Dahl [:bdahl] 2013-02-27 09:58:01 PST
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.
Comment 41 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-27 10:02:42 PST
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 :)
Comment 42 Edward B 2013-02-27 12:13:04 PST
[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 43 Dão Gottwald [:dao] 2013-02-28 03:31:16 PST
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.
Comment 44 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-28 15:19:26 PST
Yeah, good point. We should fix that.
Comment 45 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-28 15:52:32 PST
Created attachment 719736 [details] [diff] [review]
toolkit patch v3
Comment 46 neil@parkwaycc.co.uk 2013-02-28 16:20:14 PST
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.
Comment 47 neil@parkwaycc.co.uk 2013-02-28 16:22:59 PST
Created attachment 719750 [details] [diff] [review]
Proposed patch
Comment 48 neil@parkwaycc.co.uk 2013-02-28 16:25:10 PST
Created attachment 719752 [details] [diff] [review]
PdfStreamConverter.js changes

I've never used github so I don't know how to submit this change.
Comment 49 Brendan Dahl [:bdahl] 2013-02-28 16:43:30 PST
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 David H 2013-02-28 18:27:35 PST
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 Boris Zbarsky [:bz] 2013-02-28 20:48:50 PST
Comment on attachment 719750 [details] [diff] [review]
Proposed patch

I can live with this, I think...
Comment 52 Brendan Dahl [:bdahl] 2013-03-01 09:51:04 PST
(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.
Comment 54 Gregory Szorc [:gps] 2013-03-02 13:33:03 PST
https://hg.mozilla.org/mozilla-central/rev/1cd1a9fdf8fb
Comment 55 bhavana bajaj [:bajaj] 2013-03-04 15:43:03 PST
(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.
Comment 56 Brendan Dahl [:bdahl] 2013-03-04 16:50:33 PST
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
Comment 57 Brendan Dahl [:bdahl] 2013-03-04 16:51:33 PST
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
Comment 58 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-04 19:43:47 PST
(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 Boris Zbarsky [:bz] 2013-03-04 21:07:01 PST
Comment on attachment 720979 [details] [diff] [review]
aurora save as support

r=me
Comment 60 Boris Zbarsky [:bz] 2013-03-04 21:07:20 PST
Comment on attachment 720980 [details] [diff] [review]
beta save as support

Definitely the wrong patch.
Comment 61 Brendan Dahl [:bdahl] 2013-03-04 21:46:08 PST
Created attachment 721057 [details] [diff] [review]
beta save as support v2

Sorry about that.
Comment 62 Boris Zbarsky [:bz] 2013-03-04 22:09:51 PST
Comment on attachment 721057 [details] [diff] [review]
beta save as support v2

r=me
Comment 63 bhavana bajaj [:bajaj] 2013-03-04 22:52:31 PST
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.
Comment 65 Mihaela Velimiroviciu (:mihaelav) 2013-03-06 01:47:01 PST
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.
Comment 66 XtC4UaLL [:xtc4uall] 2013-03-11 09:02:57 PDT
*** Bug 849709 has been marked as a duplicate of this bug. ***
Comment 67 Brendan Dahl [:bdahl] 2013-03-11 14:51:44 PDT
*** Bug 846782 has been marked as a duplicate of this bug. ***
Comment 68 Ioana (away) 2013-04-03 04:53:27 PDT
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
Comment 69 plecto 2013-04-15 09:42:26 PDT
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.
Comment 70 Masatoshi Kimura [:emk] 2013-04-15 15:41:06 PDT
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.
Comment 71 Masatoshi Kimura [:emk] 2013-04-15 15:41:47 PDT
> You shouldn't see the "Save Web pagee as" choice
Correction: "Web page, complete" choice
Comment 72 Ioana (away) 2013-05-16 05:06:42 PDT
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

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