1638156 - Support key modifiers to open downloaded PDFs in a new window, background tab

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P2)

Description

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

It should be possible to hold click/shift/meta keys when opening a downloaded PDF and if that results in opening the file in the browser, we should follow the normal conventions and open it as appropriate:

• (default): open in a new tab ("current") open in current tab
• ctrl/metaKey: open in new tab ("tab")
• shiftKey: open in new browser window ("window")
• ctrl/metaKey + shiftKey: open in background tab ("tabshifted")

Comment 1

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

Do you have suggestions for how to get at the event to check .shiftKey/.ctrlKey/.metaKey when handling the OPEN_DOWNLOAD_FILE action in DownloadManager.jsm for the download cards about:newtab? In the other downloads views for this bug I'm calling BrowserWindowTracker.getTopWindow().whereToOpenLink(event, false, true) to allow use of key modifiers to direct where to open the download if we load it into the browser as a file: URI (e.g. new window, background tab).

Comment 2

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

Attachment: Bug 1638156 - Allow key modifiers to determine how/'where' PDFs are opened when possible. r?mak

Comment 3

[Ed Lee :Mardak]

Probably similar pattern for OPEN_LINK passing a fake event along with the action to determine where to open the link:

https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/components/newtab/content-src/components/Card/Card.jsx#149,152-154
https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/components/newtab/lib/PlacesFeed.jsm#283,307

Comment 4

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

(In reply to Ed Lee :Mardak from comment #3)

Probably similar pattern for OPEN_LINK passing a fake event along with the action to determine where to open the link:

https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/components/newtab/content-src/components/Card/Card.jsx#149,152-154
https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/components/newtab/lib/PlacesFeed.jsm#283,307

That makes sense, but its not so clear how to accomplish the same for the options menu where the OpenFile option currently lives. I'm getting lost trying to figure out where the event exists and how to pass modifier key data down to the option's action at https://searchfox.org/mozilla-central/source/browser/components/newtab/content-src/lib/link-menu-options.js#153 (ReactJs noob here)

Before I spend more time on this, the other question that may be relevant here is why SHOW_DOWNLOAD_FILE (which opens the download directory in the OS' filesystem viewer) the default action for the download cards, when elsewhere in the UI we tend to open/launch the file itself - ie OPEN_DOWNLOAD_FILE. Was there a decision made the showing the containing directory is a better fit somehow for this context? If not, maybe I can change the default to OPEN_DOWNLOAD_FILE at which point the tips in comment #3 should be most of what I need.

Comment 5

[Ed Lee :Mardak]

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

Was there a decision made the showing the containing directory is a better fit somehow for this context?

It looks like the original UX desire was to open the file:
https://github.com/mozilla/activity-stream/issues/3535#issuecomment-344707481

I believe there was a security concern of having a (special) content page sending a message to the main process to "open <arbitrary file>" although I suppose that surface could be reduced to "open <downloaded file index 1>" or some other downloads-specific identifier to then figure out which file it should open.

Comment 6

[Ed Lee :Mardak]

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

I'm getting lost trying to figure out where the event exists and how to pass modifier key data down to the option's action

Ah there's a bunch of indirection going on here… The context menu is controlled by an array of strings where those strings key into the link-menu-options that then dynamically create onClick handlers on the rendered items:
https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/browser/components/newtab/content-src/components/LinkMenu/LinkMenu.jsx#41,53-54

It looks like no menu items have ever needed to use the event object or anything dynamic. I suppose maybe you can define the action of a link menu option to be a function that LinkMenu.jsx decides to call the function to get an action object or just plain pass on the statically declared action object.

Comment 7

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

(In reply to Ed Lee :Mardak from comment #5)

I believe there was a security concern of having a (special) content page sending a message to the main process to "open <arbitrary file>" although I suppose that surface could be reduced to "open <downloaded file index 1>" or some other downloads-specific identifier to then figure out which file it should open.

As we ended up implementing this (we send a OPEN_DOWNLOAD_FILE message to the parent process and DownloadManager.jsm calls into DownloadsCommon/DownloadIntegration etc to do the file opening) , but under the options menu, I assume this security concern is resolved, or at least orthogonal to this bug.

Comment 8

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

Attachment: Bug 1638156 - Make open downloaded file the default action in about:newtab download cards, and add key modifiers support. r?mardak

Depends on D75870

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da0e27f7f17a
Allow key modifiers to determine how/'where' PDFs are opened when possible. r=mak
https://hg.mozilla.org/integration/autoland/rev/c5c3da72080f
Make open downloaded file the default action in about:newtab download cards, and add key modifiers support. r=Mardak

Comment 10

[Razvan Maries]

Backed out for perma failures on browser_pdfjs_preview.js and browser_download_open_with_internal_handler.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&searchStr=linux%2C18.04%2Cx64%2Casan%2Copt%2Cmochitests%2Ctest-linux1804-64-asan%2Fopt-mochitest-browser-chrome-e10s-2%2Cm%28bc2%29&revision=c5c3da72080fe4504a941fbbbb0cba112ab387b9&selectedTaskRun=FUWEbONRTPq1ruselB2Hqw-0

Failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304050461&repo=autoland&lineNumber=2047
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304050458&repo=autoland&lineNumber=5164

Backout: https://hg.mozilla.org/integration/autoland/rev/6927efcab2958a77515d245a0dd88677b20775b2

Comment 11

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

To fix the browser_download_open_with_internal_handler.js failure, I set the default "openWhere" option to DownloadUIHelper.loadFileIn to "tab". All the views will pass "current" by default but it looks like for the URI-loader use-case, "tab" does the right thing.

I found at least part of the issue with the private download / about:downloads test: Bug 1329912. With a workaround in place for that, I can't reproduce this locally on linux anymore. But it looks like https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea1517cc36a34c3a52f36d72ea4be650ef65a62 still has some failures.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5e87e5b6f9f
Allow key modifiers to determine how/'where' PDFs are opened when possible. r=mak
https://hg.mozilla.org/integration/autoland/rev/8d8372333cb9
Make open downloaded file the default action in about:newtab download cards, and add key modifiers support. r=Mardak

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c5e87e5b6f9f
https://hg.mozilla.org/mozilla-central/rev/8d8372333cb9

Comment 14

[Anca Soncutean, Desktop QA]

The support key modifiers to open downloaded PDFs works as described in comment 0 on Fx 79.0a1 (2020-06-24) and Fx 78.0 across platforms (Windows 10, Ubuntu 18.04 and macOS 10.15), with the mention that the default behavior was verified in bug 1642431.
One question here, when attempting to open several times the downloaded file using a different key combination, for example I click the downloaded file 1 time and a new tab opens to the right, then using Ctrl + Shift +click the new tab is displayed to the left of the previously opened tab. Shouldn't it be opened to the right as well? Is this behavior expected?

Comment 15

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

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #14)

One question here, when attempting to open several times the downloaded file using a different key combination, for example I click the downloaded file 1 time and a new tab opens to the right, then using Ctrl + Shift +click the new tab is displayed to the left of the previously opened tab. Shouldn't it be opened to the right as well? Is this behavior expected?

I'm not able to reproduce this as described. Yes, the default behavior (equivalent to ctrl+click) should open the PDF at the end (right in LTR) of the tab strip and switch to it. And Ctrl+shift+click should open another tab right of that one. This is true for clicks on items representing downloads in the browser UI - not necessarily for clicking links in content. If you can confirm STR to get the behavior you describe in comment 14 can you open a new bug for that?