1657796 - PDFs downloaded with "Open with Nightly" don't respect "Open in system viewer" context menu item

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Given that a system PDF viewer besides Nightly is set:

1. Download a PDF served with content-disposition: attachment, example
2. Keep the default "Open with Nightly" option in the unknown content type dialog, the PDF will open from the filesystem in a new tab
3. Right click on the download in the downloads list and select "Open in System Viewer". It will open another tab with the PDF in Firefox.

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Menus should do what they say they do.

Sam, can you take a look?

Comment 2

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

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #0)

3. Right click on the download in the downloads list and select "Open in System Viewer". It will open another tab with the PDF in Firefox.

I can reproduce. Thanks.

Comment 3

[Julien Cristau [:jcristau] (back April 22)]

Sam, we're building the last beta for 80 tomorrow do you think you'll have an upliftable fix ready?

Comment 4

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

I'm putting a patch together. The problem is right here: https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadIntegration.jsm#799-800
aDownload.handleInternally is true in this case, so even though useSystemDefault is also true, it wins out and and open the download in a new tab. I think is true to say that useSystemDefault should always win in this case - the user has explicitly made that choice by clicking that context menu item . But that seems kinda obvious so I need to understand why it wasn't already that way!

Comment 5

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

Attachment: Bug 1657796 - Ensure downloads requested to open with system viewer always do. r?Gijs

Comment 6

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

There's a patch up for review, and try push at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e33862474c1b851472b733231bf3172a86842b17

We don't have great automated test coverage in this area - as it is interacting with the external system and installed applications.

The change just re-orders the logic so even if a download can be handled internally (i.e. the user chose to open in the browser at the "What should nightly do with this file prompt", or its a known application/pdf type of file) we still open in system viewer as the user requested by clicking on the "Open in system viewer" context menu item.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b027ec0b329
Ensure downloads requested to open with system viewer always do. r=jaws

Comment 8

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

Comment on attachment 9169888 [details]
Bug 1657796 - Ensure downloads requested to open with system viewer always do. r?Gijs

Beta/Release Uplift Approval Request

• User impact if declined: Clicking the "open in system viewer" on a downloaded PDF will actually open the PDF in a new tab.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment #0.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a minor tweak to the logic for how we handle opening a download. There is test coverage for the scenarios in which we expect the download to open in a new tab, but not for those cases where we expect to open in an external application.
• String changes made/needed: None

Comment 9

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I don't think the current patch works properly, useSystemDefault is now being ignored if any other option was selected from the dialog, or if the file was just saved. (cross-posted from phab, sorry for not getting to this sooner)

Comment 10

[Natalia Csoregi [:nataliaCs]]

Backed out as requested: https://hg.mozilla.org/integration/autoland/rev/2ef8b79d5af2d94e1ac94be7d6b33329c19cb112

Comment 11

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

Comment on attachment 9169888 [details]
Bug 1657796 - Ensure downloads requested to open with system viewer always do. r?Gijs

I backed that patch out as it broke a different case and needs more work.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2420dc816657
Ensure downloads requested to open with system viewer always do. r=Gijs

Comment 13

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

Comment on attachment 9169888 [details]
Bug 1657796 - Ensure downloads requested to open with system viewer always do. r?Gijs

Beta/Release Uplift Approval Request

• User impact if declined: Clicking the "open in system viewer" on a downloaded PDF will actually open the PDF in a new tab.
  Toggling off the "Always use system viewer" item won't actually revert to loading downloaded PDFs in a new pdf.js tab.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Download a PDF served with content-disposition: attachment

2. Keep the default "Open with Nightly" option in the unknown content type dialog, the PDF will open from the filesystem in a new tab
3. Right click on the download in the downloads list and select "Open in System Viewer". It will open another tab with the PDF in Firefox.

For the "Always use system viewer" behavior:
4. Right click on the download item in the downloads panel for the previously downloaded PDF
5. Click "Always open in system viewer". It should become checked, and open the PDF into your external PDF viewer.
6. Right click again on the PDF download item. Click "Always open in system viewer" to toggle it off. The PDF and any subsequent PDF downloads should open and load directly into a new tab via pdf.js

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a minor tweak to the logic for how we handle opening a download. There is test coverage for the scenarios in which we expect the download to open in a new tab, but not for those cases where we expect to open in an external application.
• String changes made/needed: None

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2420dc816657

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9169888 [details]
Bug 1657796 - Ensure downloads requested to open with system viewer always do. r?Gijs

Approved for 80.0rc1.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b4bf5a53730f

Comment 17

[Alexandru Trif, Desktop QA [:atrif]]

Reproduced the initial issue with Firefox 81.0a1 (20200806215439) on Windows 10x64.
The issue is verified fixed with Firefox 81.0a1 (20200816214203) and Firefox 80.0b9 (20200814201339) from comment 16 on Windows 10x64, macOS 10.12 and Ubuntu 18.04. Checking Always Open in System Viewer in downloads panel will open the pdf with the external PDF viewer and unchecking Always Open in System Viewer will open the pdf in a new tab.