1191591 - Opening PDFs from Firefox's download panel should open the PDF within Firefox

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Chris More [:cmore]]

Steps to reproduce.

1) Open Firefox

2) Download a PDF via Firefox

3) Review that the downloaded PDF is inside the Firefox download panel

4) Click the PDF in the Firefox download manager

Expected results:

PDF opens in Firefox using pdf.js just like it would if you were to click a PDF on a website.

Actual results:

PDF opens in whatever is the default application on the operating system.

======================

Here's a few OS/browser specific use cases.

== Windows 10 ==

* Microsoft Edge is the default PDF reader on Windows 10.
* Opening a downloaded PDF from Firefox's download panel opens the file Edge.

== Chrome browser ==

* If you open a downloaded PDF in Chrome's download manager, it opens in Chrome and *not* the operating system default.

== Firefox on OSX ==

* Preview is the default PDF viewer on the Mac
* Opening a downloaded PDF from Firefox's download panel opens Preview

From a UX perspective, opening downloaded PDFs within the Firefox user interface, should use Firefox's PDF renderer. If someone opens a PDF outside of Firefox, it should use the OS's default setting. 

We should consider taking the approach that Chrome does where clicking a downloaded PDF from *within* Chrome opens the PDF in the Chrome. 

Opening a PDF outside of Chrome/Firefox/Edge, should open with whatever is the OS default and that's not something we should mess with.

Comment 1

[Chris More [:cmore]]

Windows 10 is unique here, because in all other operating systems across Windows and Mac, the default PDF application on the operating system is an application to open PDFs. On Windows 10, Microsoft registered Microsoft Edge as the default PDF view. So, opening any PDF will open Chrome even if you are opening it from the Firefox download panel.

Comment 2

[Chris More [:cmore]]

:verdi: can you review this with the Firefox UX team to see what they all think about this and especially with the Windows 10 situation?

Comment 3

[Chris More [:cmore]]

:marcom: can you check out this bug and let us know how we can get someone from the Firefox team to review to evaluate the priority?

Comment 4

[Marco Mucci [:MarcoM]]

(In reply to Chris More [:cmore] from comment #3)
> :marcom: can you check out this bug and let us know how we can get someone
> from the Firefox team to review to evaluate the priority?

Directing to Dolske to address.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Adding philipp, let's review this one in our triage.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: chrome-pdf-options.png

Just for reference these are the options that Chrome offers for opening a PDF within Chrome or with the System Viewer.

Comment 7

[Justin Dolske [:Dolske]]

(In reply to Chris More [:cmore] from comment #0)
[...]
> We should consider taking the approach that Chrome does where clicking a
> downloaded PDF from *within* Chrome opens the PDF in the Chrome. 
> 
> Opening a PDF outside of Chrome/Firefox/Edge, should open with whatever is
> the OS default and that's not something we should mess with.

I think there are a couple of use-cases in play here...

1) From discussion with Bryan last week, it sounded like the request was just to enable using PDF.js as an option to view files when they'd normally be downloaded. In other words, this is basically a specific example of ancient bug 185618 -- when we prompt to ask the use how to handle a file (save to disk or open with app), we'd be adding a 3rd "View in Firefox" choice.

File handling is old and crusty code, so that might end up being more difficult to do than one would think.

2) It looks like the current download manager code just hands off files to the system when you click to launch them. We could special-case PDF files, but would presumably need to add options somewhere (such as the context menu, as with Chrome?) to allow using the OS choices instead?

3) Verdi filed bug 1184306 to have Firefox associate itself with .pdf files. That's a bigger change for users, and sounds like you don't want to deal with that here.

Comment 8

[Verdi [:verdi] Best to slack me]

Sorry - should have commented a while ago. I brought this up with the UX team and there wasn't a clear answer. Dolske pretty well captured the conversation in comment 7 above and in Bug 1184306 comment 1.

Comment 9

[James Teh [:Jamie]]

An accessibility concern here is that pdf.js is pretty bad in terms of accessibility. While it's true that PDFs that aren't exposed as attachments already open in pdf.js, some users might be relying on the fact that if they download the PDF (to work around pdf.js poor a11y), opening it will open it in the OS default viewer like it does now. Of course, users can change the Firefox default, but this isn't trivial for novice users.

Having an option in the context menu to use the OS viewer might help mitigate the a11y concern. It's probably simpler/more discoverable than changing the Firefox default PDF viewer.

Comment 10

[Jonathan Watt [:jwatt]]

(An example of a PDF with Content-Disposition: attachment, FWIW)

Comment 11

[Marco Zehe (:MarcoZ) [out sick]]

FYI, the bug that tracks PDF.js accessibility is bug 727819, and its dependencies.

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1191591 - Open downloaded PDFs in pdf.js viewer. r?Gijs

Comment 13

[Sam Foster [:sfoster] (he/him)]

I'm looking into handling modifier keys to e.g. open download in a new window, or background tab. The problem is, all the event handling goes via the command infrastructure and doCommand, whose signature is an XPCOM interface so I can't simply pass the event along without changing that interface (even if that was a smart idea, which I suspect it isn't)

So I went looking at each of the entry points:
Here's the (reversed) stack from clicking an item in the download panel. Its in sequential order; we get the mousedown and the story begins...

onmousedown@chrome://browser/content/browser.xhtml:1:24
onCommand@chrome://browser/content/downloads/indicator.js:581:20
...
EventListener.handleEvent*connect@resource:///modules/DownloadsViewUI.jsm:211:18
connect/<@resource:///modules/DownloadsViewUI.jsm:212:43
onDownloadClick@chrome://browser/content/downloads/downloads.js:826:20
* the event with my modifier keys exists here, but is not forwarded to goDoCommand
* At this point, we know its a click on a download item, and we know we want to 'open' it
* is there a reason to go around the houses with goDoCommand, doCommand in this case?
goDoCommand@chrome://global/content/globalOverlay.js:101:18
doCommand@chrome://browser/content/downloads/downloads.js:1194:45
doCommand@chrome://browser/content/downloads/downloads.js:1052:21
downloadsCmd_open@chrome://browser/content/downloads/downloads.js:1076:11
downloadsCmd_open@resource:///modules/DownloadsViewUI.jsm:762:16

The stack from clicking an item in about:downloads:

EventListener.handleEvent*@chrome://browser/content/downloads/allDownloadsView.js:844:10
EventListener.handleEvent*@chrome://browser/content/downloads/allDownloadsView.js:852:15
@chrome://browser/content/downloads/allDownloadsView.js:853:29
onDoubleClick@chrome://browser/content/downloads/allDownloadsView.js:742:22
*  At this point we have the event. Are modifiers on double-clicks even a thing?
doDefaultCommand@chrome://browser/content/downloads/allDownloadsView.js:135:12
doCommand@resource:///modules/DownloadsViewUI.jsm:736:21
downloadsCmd_open@resource:///modules/DownloadsViewUI.jsm:762:16

The stack from clicking an item in the library > downloads menu

oncommand@chrome://browser/content/browser.xhtml:1:18
show@resource:///modules/DownloadsSubview.jsm:260:40
EventListener.handleEvent*DownloadsSubview@resource:///modules/DownloadsSubview.jsm:71:20
onClick@resource:///modules/DownloadsSubview.jsm:424:27
* This function tries to figure out what got clicked and the command name for it
* and checks the command is enabled before calling item._shell[command]()
downloadsCmd_open@resource:///modules/DownloadsViewUI.jsm:762:16

The stack from clicking an item in the all downloads dialog (view)

EventListener.handleEvent*@chrome://browser/content/downloads/allDownloadsView.js:844:10
EventListener.handleEvent*@chrome://browser/content/downloads/allDownloadsView.js:852:15
@chrome://browser/content/downloads/allDownloadsView.js:853:29
onDoubleClick@chrome://browser/content/downloads/allDownloadsView.js:742:22
* This is another double-click, so I'm not sure if it even makes sense to have modifiers here
* but if it did, they exist on the event available here (and don't get forwarded to doDefaultCommand)
doDefaultCommand@chrome://browser/content/downloads/allDownloadsView.js:135:12
doCommand@resource:///modules/DownloadsViewUI.jsm:736:21
downloadsCmd_open@resource:///modules/DownloadsViewUI.jsm:762:16

So, there's a couple of cases where we have the event we need, but lose it by the indirection of using doCommand. We certainly need to handle the cases where no event is available, but I wonder we would lose by bypassing doCommand and calling downloadsCmd_open directly with an event argument when possible?

Comment 14

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1191591 - Delegate to the parent class' code when opening downloads from the download panel. r?Gijs

Comment 15

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #13)

I wonder we would lose by bypassing doCommand and calling downloadsCmd_open directly with an event argument when possible?

I don't think there is anything, other than the command enabled/disabled state, which should already be known / acceptable (ie it looks like the download panel code, at least, picks a specific command and expects it to be enabled, so...).

Marco, do you know if I'm missing something here?

Comment 16

[Marco Bonardo [:mak]]

Going through goDoCommand is easier because it figures out by itself the right controller, checks that the command is effectively enabled and fires it. It should be possible to bypass it, I don't know by memory if there's some command that may be executed even when disabled, for the specific click event it seems not plausible, while it would be more likely for keyboard events. So until the bypass is limited to the click event, there should be no problem.
But what about keyboard? I can Enter, shift+enter and so on, on a download from these views, doesn't it go through the same commands, and then modifiers would only work for clicks, not for the keyboard?
Couldn't you introduce a different command for each action, like downloadsCmd_open, downloadsCmd_open:window, downloadsCmd_open:tab and then fire the appropriate command? there's a precedent here https://searchfox.org/mozilla-central/rev/7908ce29657cfd623993046bd8e38664e1c0b28e/browser/components/places/content/controller.js#167-170

Comment 17

[Sam Foster [:sfoster] (he/him)]

(In reply to Marco Bonardo [:mak] from comment #16)

But what about keyboard? I can Enter, shift+enter and so on, on a download from these views, doesn't it go through the same commands, and then modifiers would only work for clicks, not for the keyboard?

Ah, great point I hadn't thought about modifiers with keyboard events.

Couldn't you introduce a different command for each action, like downloadsCmd_open, downloadsCmd_open:window, downloadsCmd_open:tab and then fire the appropriate command? there's a precedent here https://searchfox.org/mozilla-central/rev/7908ce29657cfd623993046bd8e38664e1c0b28e/browser/components/places/content/controller.js#167-170

We don't know what the outcome of the command will be until we've first figured out if the selected download is a PDF which the user has elected to view in the browser. But it does give me some directions to explore, thanks for that.

Comment 18

[Marco Bonardo [:mak]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #17)

We don't know what the outcome of the command will be until we've first figured out if the selected download is a PDF which the user has elected to view in the browser. But it does give me some directions to explore, thanks for that.

Of course, but the command name is not a contract, it's a way to annotate that the command is different, when you actually manage the command you can pick different paths depending on the download, and with a few comments it should not be a problem.

Comment 19

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1191591 - Open downloaded PDFs in about:newtab with pdfs.js. r?Gijs

Depends on D73400

Comment 20

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1191591 - Provide a helper for getting a nsIMIMEInfo on DownloadsCommon. r?Gijs

Comment 21

[:Paolo Amadini]

Just leaving a fly-by comment after looking at one of the patches, as I don't have time for a full code review or fast replies at the moment. I know this code has loads of redundant paths and may be difficult to follow, but I encourage to try and see the consequences of seemingly small code changes. There are unfortunately few automated regression tests in this area.

One of the patches changes the code so it goes through "DownloadsCommon.openDownloadedFile" instead of "Download.launch". If I remember correctly, I think the former exists so we can pass around the reference to the window when we show the executable confirmation dialog box, in case it's the separate Library window. It's also a separate code path because it used to handle history downloads in the Library that were substantially different from session downloads, although I later consolidated the code so the underlying objects now look much more similar.

A major historical issue with "DownloadsCommon.openDownloadedFile", if I'm correct in looking at the latest code, is that it doesn't honor the "launcherPath", which is the application manually chosen by the user when the download started. There may be bugs on file related to this. This means that, probably, this patch breaks the dialog that allows the user to choose a custom application to open a certain file type, including an unknown file type. Bug 1306338 is on file to remove this ability eventually, but we're not there yet, see also bug 1306348 for telemetry.

Also, bear in mind that if the user chooses to open the file automatically at the end of the download, or if it's the default remembered setting, this never goes through "DownloadsCommon.openDownloadedFile" or the front-end UI anyways, but always through "Download.launch".

Here I see an opportunity to get rid of "DownloadsCommon.openDownloadedFile" entirely if it starts to be too complicated to handle two different paths. The reference to the window might not be that important. This patch series instead seems to go in the direction of adding new front-end code for every different piece of UI that can launch a download, like new tab page, that could instead be consolidated in the back-end.

Comment 22

[:Paolo Amadini]

I can see that we're using direct event helpers like openLinkIn, but the way the file should be opened, like in a new tab or window, may be something that we can encode and pass through the back-end calls in some way, which in some way connects to what Marco was suggesting with using different commands. Then the back-end might still have to find the most recent browser window, like it does now. The Download object is just one for the entire application, not one per window, so it shouldn't have window-specific state, but I believe it could be fine if they are passed around as arguments.

In the end it may be that, after a detailed analysis, this approach doesn't work or is too much work for the time you have, but it's worth taking a look to see if there is a chance to simplify the code and avoid accumulating too much technical debt.

By the way, it seems that the stack captures in comment 13 have been taken after applying the patches, and are not the current behavior on a fresh checkout, is that correct?

Again, a reminder that my replies may be slow and I won't have time for code review.

Comment 23

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #22)

I can see that we're using direct event helpers like openLinkIn, but the way the file should be opened, like in a new tab or window, may be something that we can encode and pass through the back-end calls in some way, which in some way connects to what Marco was suggesting with using different commands. Then the back-end might still have to find the most recent browser window, like it does now. The Download object is just one for the entire application, not one per window, so it shouldn't have window-specific state, but I believe it could be fine if they are passed around as arguments.

But we can't pass arguments to commands (not without touching the command dispatching interfaces, which I really don't want to do given nothing else seems to need this functionality), so if we don't store the state in the download, I don't see a way of threading extra information through. This is what we've been discussing, and why Marco suggested different commands in comment 16.

Comment 24

[Sam Foster [:sfoster] (he/him)]

Ok so what I'm hearing and the changes I'll investigate are:

• DownloadsCommon.openDownloadedFile (used by DownloadsViewUI which is the library UIs and about:newtab) is largely redundant with Download.prototype.launch. It has an existing bug where it fails to handle the .launcherPath property. It exists mostly to accept a DOMWindow argument which is possibly useful for cases where the launcher prompt needs to be anchored to the library window rather whatever the most recent window is. It may be a good trade-off to remove openDownloadedFile altogether and use Download.prototype.launch exclusively.

• Ideally, we would move the how-to-open-this-pdf-download logic to the shared backend, rather than duplicate it everywhere in the UI code. And that makes more sense as we already get the necessary mimeInfo object there. But, as there is no DOM context and no DOM event, we run the risk of opening the PDF in a tab in the wrong window, and we'll need a different way to pass along the user's intention to open in a background tab or new window. Some of this flow necessarily needs the context we have in the UI code.

The key modifiers issue actually isn't a new problem, we already run into this as we fallback to trying the file: protocol handler for some file types - which might be us. So, if you download e.g an html file, then shift+click the entry to open it in a new window, you just get a new tab in the current window. If we move this PDF-opening logic to the backend, we inherit the same problem. (and get 2 birds with one stone when we solve it)

Comment 25

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1191591 - Consolidate download opening to use download.launch() via a DownloadsCommon.openDownload helper. r?Gijs

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6b3783367f6
Consolidate download opening to use download.launch() via a DownloadsCommon.openDownload helper. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/3ff8cc8dd4b7
Delegate to the parent class' code when opening downloads from the download panel. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/bd59c10af492
Open downloaded PDFs in pdf.js viewer. r=Gijs

Comment 27

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1191591) for causing newtab failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=KgTbm0NRQm6B1DQk2udcHw-0&resultStatus=testfailed%2Cbusted%2Cexception&revision=bd59c10af492e987558712791ce41a347baf5bb0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302521007&repo=autoland&lineNumber=92
Also a TV failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302511400&repo=autoland&lineNumber=11968

Backout: https://hg.mozilla.org/integration/autoland/rev/1ded947f34354e5b37d6b2b03be5d52e9533ac06

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cead18119ae
Consolidate download opening to use download.launch() via a DownloadsCommon.openDownload helper. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d4eed4b66fab
Delegate to the parent class' code when opening downloads from the download panel. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/149db961c712
Open downloaded PDFs in pdf.js viewer. r=Gijs

Comment 29

[Sam Foster [:sfoster] (he/him)]

(In reply to Noemi Erli[:noemi_erli] from comment #27)

Backed out 3 changesets (Bug 1191591) for causing newtab failures

Thanks, I'd forgotten about this test. I just needed to update which method was stubbed. Passes now on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ea7762627cb7830c3eb9f1792f6851fbbc13530

Comment 30

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4cead18119ae
https://hg.mozilla.org/mozilla-central/rev/d4eed4b66fab
https://hg.mozilla.org/mozilla-central/rev/149db961c712

Comment 31

[Anca Soncutean [:Anca], Desktop Release QA]

Verified fixed with Fx 78.0a1 (2020-05-24) across platforms - Windows 10 64bit, macOS 10.15 and Ubuntu 18.04.

Comment 32

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1191591 - Provide a helper for getting a nsIMIMEInfo on DownloadsCommon, and confirm if a given download is a PDF. r?jaws

Comment 33

[Phabricator Automation]

Comment on attachment 9156129 [details]
Bug 1191591 - Provide a helper for getting a nsIMIMEInfo on DownloadsCommon, and confirm if a given download is a PDF. r?jaws

Revision D79395 was moved to bug 1639069. Setting attachment 9156129 [details] to obsolete.