Closed Bug 1639069 Opened 6 months ago Closed 5 months ago

Add a context menu item in the Downloads Panel to "Open in system viewer" and "Always open in system viewer"

Categories

(Firefox :: Downloads Panel, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 79
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- wontfix
firefox77 --- unaffected
firefox78 --- wontfix
firefox79 + verified
firefox80 --- verified

People

(Reporter: sfoster, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxgrowth])

Attachments

(3 files, 1 obsolete file)

Each of the downloads views provides a context menu for the download list items to "Open Containing Folder" etc.
This menu should include options to "Open in system viewer" and "Always open in system viewer", when the download is PDF.

See Also: → 1639067
Severity: -- → S3
Priority: -- → P2
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed262a70a438
Strings for new download context menu items. r=jaws,fluent-reviewers
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 78

There's more work to do here, I just forget to add leave-open when I landed that initial strings patch.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1641947

[Tracking Requested - why for this release]: Making it easier to access PDF.js will be a large part of Firefox 78 release and this bug tracks some of the work that just missed landing before the uplift from Nightly -> Beta.

Attachment #9156130 - Attachment description: Bug 1639069 - WIP add download context menu items to use and always use system viewer. r?jaws → Bug 1639069 - Add download context menu items to 'Use' and 'Always use' the system viewer to open the download. r?jaws

Is this still targeting 78? We're now in the last week of beta.

Flags: needinfo?(sfoster)
Attachment #9154357 - Attachment is obsolete: true

I noticed that currently (with/without these patches) if you download a PDF with contentType: "application/octet-stream" and a filename of something.pdf, the mimeInfo we get from mimeInfo = gMIMEService.getFromTypeAndExtension in DownloadIntegration#launchDownload will be for 'application/octet-stream', not 'application/pdf'. This means that mimeInfo.alwaysAskBeforeHandling will be true, so in the logic that determines if we should call DownloadUIHelper.loadFileIn for this file, the answer is always "no."

This also means that the context menu items we add here appear not to work with this kind of download; toggling "Always use system.." has no effect as despite resetting the preferredAction in the command, we're retrieving the wrong nsIMimeInfo and alwaysAskBeforeHandling value. I think what we want to happen in launchDownload is to follow the same logic now implemented in DownloadsCommon.getMimeInfo. Maybe that could move to Download (DownloadCore.jsm) where we can reuse it from DownloadIntegration.jsm

I think we should take stock and make a new plan as we are down to only a couple of days to land and uplift this.

Flags: needinfo?(sfoster) → needinfo?(jaws)

Thanks for digging in to that. At this point I don't think we should uplift, we will want some extra time for this to bake and fix any regressions or missing support. It's okay if this isn't in 78. It would be great if we can do it "right" in 79.

Flags: needinfo?(jaws)

Are we eventually going to want to do something for ESR78?

Status: REOPENED → ASSIGNED
Flags: needinfo?(sfoster)
Target Milestone: Firefox 78 → ---

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)

Are we eventually going to want to do something for ESR78?

Ideally yes, perhaps if there is a 78.1? We'll want to at least land and verify in nightly first.

Flags: needinfo?(sfoster)

Status: I'm tracking down an issue where the context menu on PDF downloads in the all-downloads view alternates between having the system viewer menu items and not. This showed up as a test failure but is reproducable manually.

Attachment #9157203 - Attachment description: Bug 1639069 - Provide a helper for getting a nsIMIMEInfo on DownloadsCommon, and confirm if a download is a given mime-type. r?jaws → Bug 1639069 - Provide helpers for getting a nsIMIMEInfo on DownloadsCommon, and confirming if a download is a given mime-type. r?jaws
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b06131994e4a
Provide helpers for getting a nsIMIMEInfo on DownloadsCommon, and confirming if a download is a given mime-type. r=jaws
https://hg.mozilla.org/integration/autoland/rev/d37373cfa435
Add download context menu items to 'Use' and 'Always use' the system viewer to open the download. r=jaws
Status: ASSIGNED → RESOLVED
Closed: 6 months ago5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 79
Flags: qe-verify+

Verified on Firefox 79.0b3 and Firefox 80.0a1 (2020-07-03) that these two new options are successfully implemented and they work as expected.

Tests were performed on macOS 10.15.5, Ubuntu 20.04 and Windows 10, using a pdf sample on google.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Is this ready for an ESR78 approval request?

Flags: needinfo?(sfoster)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)

Is this ready for an ESR78 approval request?

I'm still weighing the risks vs. benefits. This hasn't had long to bake for a feature that is not front-and-center for most people, so seeing no follow-up issues doesn't necessarily mean there are none. However, it has been verified by QA in 79 and 80, so its probably fair to say it is low (but not zero) risk.
The flip side is what is the risk of shipping ESR 78 without it? The idea with this feature is to provide choices and mitigation for when the changes in how we handle opening downloaded PDFs by default clash with users' workflows. So there is some potential risk in shipping one without the other.

Flags: needinfo?(sfoster)

Should we consider disabling the feature on ESR78 instead?

Flags: needinfo?(sfoster)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)

Should we consider disabling the feature on ESR78 instead?

I think the consensus is that ESR78 is fine without this patch at this point.

Flags: needinfo?(sfoster)
You need to log in before you can comment on or make changes to this bug.