1639067 - Open downloaded files in common web formats as file: URIs in Firefox

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, enhancement, P1)

Description

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

There are a many file formats which Firefox is a more than adequate viewer for. When a user has downloaded a file and clicks to open that download from any of our download views, unless configured otherwise we should open compatible files in a new tab in their current browser window.

This list could grow, but as a starter maybe the following file types:
.html
.xml
.svg
.gif
.png
.jpg

See bug 1191591 where we have done this for downloaded PDF documents.
This is not the same as registering Firefox as a handler for these formats with the OS, and would not preclude the user from opening a downloaded file in their application of choice.

Comment 1

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

Adding bug 1639069 to the see-also list. 1639069 might arguably block this bug if there were concerns about breaking user work flows.

Comment 2

[Romain Testard [:RT]]

Related is https://bugzilla.mozilla.org/show_bug.cgi?id=1627676 that should help us understand extensions of downloaded files.
My top priorities would be to address file extensions that would point to other browsers where we can do as good of a job:

• .xml and .svg (currenlty IE sets itself as default)
• webp (currently CHrome sets itself as default)
• shtml (currently Brave sets itself as default)
• xht and xhtml (currently IE and Edge set themselves as default)

I summarized what I saw with other browsers on https://docs.google.com/document/d/1BFpd1mNdok0ZbhlKEgExEZKPEmlmQRath_s1AmPtVug/edit

Comment 3

[:Gijs (out for now; he/him)]

I think the right way to fix this is for the dialog to check the mimetype with the category manager. Specifically:

Services.catMan.getCategoryEntry("Gecko-Content-Viewers", mimetype)

will return a contract ID if we support viewing the mimetype, and an exception (non-NS_OK result) if not.

In theory, what with everything else in place, this should now be relatively straightforward to fix...

Comment 4

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

(In reply to :Gijs (he/him) from comment #3)

I think the right way to fix this is for the dialog to check the mimetype with the category manager. Specifically:

Services.catMan.getCategoryEntry("Gecko-Content-Viewers", mimetype)

Which dialog are you refering to here? I ask because most of the work for bug 1639069 was in getting params to the launchDownload in DownloadIntegration.jsm and special-casing application/pdf to open directly into a tab (handleInternally).

I had thought of an explicit list of mimeTypes (perhaps as well as not instead of the category manager check) just to avoid surprises and make it easier to know what we expect to happen, and perhaps respond to user feedback on the download behavior specifically. Or is the PDF case substantially different?

Comment 5

[:Gijs (out for now; he/him)]

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

(In reply to :Gijs (he/him) from comment #3)

I think the right way to fix this is for the dialog to check the mimetype with the category manager. Specifically:

Services.catMan.getCategoryEntry("Gecko-Content-Viewers", mimetype)

Which dialog are you refering to here?

The unknown content type dialog (which offers "open" and "save" actions).

I ask because most of the work for bug 1639069 was in getting params to the launchDownload in DownloadIntegration.jsm and special-casing application/pdf to open directly into a tab (handleInternally).

Right, instead of hardcoding application/pdf, I think the above expression will return true for both application/pdf and for a bunch of other things.

I had thought of an explicit list of mimeTypes (perhaps as well as not instead of the category manager check) just to avoid surprises and make it easier to know what we expect to happen, and perhaps respond to user feedback on the download behavior specifically. Or is the PDF case substantially different?

I don't think PDF is substantially different, though I could imagine that there would be people who'd be upset if we did this for images or some types of text/html files that they expect to open in external viewers/editors, that offer functionality which Firefox doesn't. Still, as long as we continue honouring file choices, that should be OK.

Note that in addition to this though, we'd need to update the preferences so we represent the associations involved. Though it gets a bit messy between having both a "do we ask the user when downloading" and "what is the default action" property of each mimetype/extension combination to represent in the prefs... (right now, if we always ask then that's what we'll show, and it's not obvious you could alter the default action and then alter it back to "always ask" to get the desired behaviour from a download view while maintaining choice when initially downloading, if that's what you want (e.g. because you want to pick the file saving destination)).

Comment 6

[Rachel Tublitz [:rachel]]

Adam, can you confirm which file types we're targeting for FX 81?

Comment 7

[Rachel Tublitz [:rachel]]

Link to PI request

Comment 8

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

(In reply to Rachel Tublitz [:rachel] from comment #6)

Adam, can you confirm which file types we're targeting for FX 81?

To answer this immediate question, I'm planning on .xml (text/xml, maybe application/xml if I can get both working), .svg, (image/svg+xml) and .webp (image/webp). I don't plan on doing .shtml, .xht, .xhtml currently, as I don't have a good way of distinguishing their handler from that for .html, generally.

Comment 9

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

Attachment: Bug 1639067 part 1 - Open more downloaded file types internally. r=Gijs!

Also attempts some of the refactoring proposed in bug 1638913, moving
the type-specific checks out of toolkit, which is also also used
further down this patch stack.

Comment 10

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

Attachment: Bug 1639067 part 2 - Extend "Open with" and overwrite description to all types viewable internally. r=Gijs!

I didn't fold PDF into this part as it has different conditions for showing
the radio button vs. viewing internally from the downloads list.

Depends on D86650

Comment 11

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

Attachment: Bug 1639067 part 3 - Extend "Open In System Viewer" from PDF to all viewable internally. r?Gijs!

Depends on D86651

Comment 12

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

Attachment: Bug 1639067 part 4 - Generalize internal handlers in the Applications list. r?Gijs!

Removes the need for applications-file-ending-with-type, and avoids
replicating applications-type-*-with-type for every internally
handled file type.

Depends on D86652

Comment 13

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

Attachment: Bug 1639067 Include Viewable Internally types in fileExtension events. r?Gijs!

I also added types from the list of extensions we register to support on
Windows, in shared.nsh SetStartMenuInternet.

Depends on D86653

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82be6da5a371
Include Viewable Internally types in fileExtension events. r=Gijs,chutten

Comment 15

[:Gijs (out for now; he/him)]

Adding "leave open" here to avoid the bug closing because the other commits aren't landing yet, by the looks of it.

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/82be6da5a371

Comment 17

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

I was worried that this would also run into the issues in bug 1633790 (infinite tabs if Firefox is the default and set to open in default) and bug 1555637 (extra copy saved but no view if Firefox is the default and set to always save), but I wasn't able to reproduce those with XML, SVG, WebP, AVIF, or other types, with or without these patches. I suspect that there's a difference in running the document loader for these documents vs. the stream converter for pdfjs, but I haven't dug into it.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/951e10b7354f
part 1 - Open more downloaded file types internally. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/755fd5771648
part 2 - Extend "Open with" and overwrite description to all types viewable internally. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/7bb3c2ec5c7a
part 3 - Extend "Open In System Viewer" from PDF to all viewable internally. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/1e400fafd80b
part 4 - Generalize internal handlers in the Applications list. r=Gijs,preferences-reviewers,fluent-reviewers

Comment 19

[Dorel Luca [:dluca]]

Backed out 4 changesets (bug 1639067) for XPCshell failures in tests/unit/test_getMIMEInfo_pdf.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=313255439&repo=autoland&lineNumber=3972

TV failures:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=313256018&repo=autoland&lineNumber=2149

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=1e400fafd80b3071c118ece6645754aab3488f2c

Backout:
https://hg.mozilla.org/integration/autoland/rev/d9cde14e8ec0f383e9bd45407d8b3e68643115b9

Comment 20

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

Sorry, I should have caught that test_getMIMEInfo_pdf.js failure. I think the TV failure is bug 1641774, I'll include a fix for that as well.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b34389aab2a8
part 1 - Open more downloaded file types internally. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/9cf29f703679
part 2 - Extend "Open with" and overwrite description to all types viewable internally. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a9eafc98de77
part 3 - Extend "Open In System Viewer" from PDF to all viewable internally. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/70af71248a58
part 4 - Generalize internal handlers in the Applications list. r=Gijs,preferences-reviewers,fluent-reviewers

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b34389aab2a8
https://hg.mozilla.org/mozilla-central/rev/9cf29f703679
https://hg.mozilla.org/mozilla-central/rev/a9eafc98de77
https://hg.mozilla.org/mozilla-central/rev/70af71248a58

Comment 23

[James May [:fowl2]]

Does this regress session restore?

I "Open in Nightly"'d something and after a session restore I just have a "file not found" tab.

Seems to be saving to a file to a temp directory which is deleted when Firefox exits.

Comment 24

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

That's a good point. We have bug 1632274 to resolve this with PDFs, but I hadn't thought about it becoming an issue with these other types.

Comment 25

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

(In reply to James May [:fowl2] from comment #23)

Does this regress session restore?

I "Open in Nightly"'d something and after a session restore I just have a "file not found" tab.

Seems to be saving to a file to a temp directory which is deleted when Firefox exits.

It's possible to work around this somewhat by setting pref browser.helperApps.deleteTempFileOnExit false in about:config, but that isn't a solution in general.

I am worried about the potential for data loss by changing the default option in the unknown content type dialog, I opened bug 1659737 to remove the "Open in Nightly" option for these types until we decide what is intended here.