Downloads open up blank tab when middle-clicking a link that points to a download
Categories
(Core :: DOM: Navigation, defect, P3)
Tracking
()
People
(Reporter: b66id0hw1, Assigned: Gijs)
References
(Regression)
Details
(Keywords: regression, Whiteboard: [fidefe-mr11-downloads])
Attachments
(2 files)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:83.0) Gecko/20100101 Firefox/83.0
Steps to reproduce:
Right click on a download link on a page, Firefox will open up a blank tab and then add that link to the Download queue but will not automatically close the tab.
Actual results:
Firefox does not automatically close blank tabs that it opens when downloading files. This was the normal behavior for years but it changed somewhere around v60? and then was fixed a few releases ago (late 70s) but it's back again. Chrome and etc. do this correctly so I'm not sure what has happened in the back end to make this change.
Expected results:
The download link should have opened the tab, added the link to the queue, and then Firefox should have automatically closed the 'empty' tab.
Updated•4 years ago
|
Comment 1•4 years ago
|
||
The severity field is not set for this bug.
:dao, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
(In reply to jeremiah from comment #0)
Right click on a download link on a page, Firefox will open up a blank tab
Pretty sure this is missing a step - are you using "save link as..." or "open link in new tab" or something else?
Can you provide an example link where this is broken for you? Can you test in a new Firefox profile ( https://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-profiles ) to clarify if the brokenness depends on any preferences you may have set or add-ons you may have installed?
Comment 3•4 years ago
|
||
Nika fixed a similar bug in Fx83 (bug 1656753). Perhaps this bug is a regression from that code change?
Sorry, I meant middle click. I'm not sure how I missed that, I reviewed my post before submitting it -_-
What i meant was that if you open up a page with a list of links, it's easier to middle click on the ones you want and have them added to the queue instead of left clicking on them in series.
For ex., in Chrome, doing this (middle clicking on links) will open/close a new tab and add the links to your queue, most times Chrome won't even open up a new tab when you do this and just add the links to your queue. On the latest versions of FF, it will open new tabs for each link you click and then add them to your queue, without closing the empty tabs.
The way Chrome behaves was the way Firefox used to work on earlier versions but it changed around v60 or so, was fixed, and then was reverted again.
I am not sure if I have been clear enough so please let me know if there is any way for me to clarify this further.
Thank you for working on figuring out this bug!
To try this, open up this page on both Chrome and in Firefox: https://windowsloop.com/direct-download-office-2016-office-2019-office-365-iso/
Now middle click on those links to the img/iso files and you will see how Chrome handles those links and how Firefox does. Chrome will open and close the page, and then add the link to your download queue, whereas Firefox will open a blank tab, will keep that tab open, and then will add the link to your queue.
That's just the only page that I could think of quickly but it illustrates my point well.
Assignee | ||
Comment 7•4 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #3)
Nika fixed a similar bug in Fx83 (bug 1656753). Perhaps this bug is a regression from that code change?
mozregression says yes, that's the regressor. Note that that bug was about the download not happening, because the tab closed too soon - now the tab doesn't close too soon, but ends up not closing at all. :-(
Nika, can you take a look?
I'm kind of astonished this didn't break tests - I'm 90% sure we do have some tests, because I remember it took some work to make defaulting to no-opener not break them (bug 1531289). The ISO links on the page in comment #5 have target=_blank
as well as noopener/nofollow/noreferrer, in case that matters - but also, the STR involve middle-clicking (or cmd-click on macOS / ctrl-click on windows) those links.
Updated•4 years ago
|
Comment 8•4 years ago
|
||
Hmm, it looks like this was actually something semi-accidentally fixed by the changes in bug 1531289. The code to open a document with a middle-click doesn't actually go through the usual window opening machinery, instead it bounces out to chrome JS, which calls through tabbrowser to open the link in a specific location: (https://searchfox.org/mozilla-central/rev/8d722de75886d6bffc116772a1db8854e34ee6a7/browser/actors/ClickHandlerParent.jsm#117)
Because this goes through the parent process, not native window.open code, it never establishes either an opener or cross-group opener relationship between the original tab and the newly created one, so when the window closing logic looks for an opener window to parent the dialog to, it fails.
In the original approach we took in bug 1531289, the "fix" to the noopener window situation was to try to re-parent the dialog to the parent chrome window when the tab is going to be closed if no opener could be found. This unfortunately caused bug 1656753, as we'd immediately close that chrome window if a pop-up was created, and no dialog would be shown. To fix that I partially reverted back to the old opener
based behaviour, and added a new CrossGroupOpener
which would track the "opener" even in the case where a noopener
flag prevents one from existing. This avoided the issue where we would target the dialog to a doomed parent window.
Unfortunately the context menu and click handler code doesn't currently communicate this relationship to platform code, so we do not attempt to close the newly created tab, as we don't know who to parent the potential download dialog to. If we want to also close downloads started with "right click -> open in new window" (or middle-click with opening in a pop-up supported), we'll probably need frontend to identify the cross-group opener window for us, but we can perhaps "fix" it in the new-tab case by asking nsIDocShellTreeOwner for the current tab count, and reparenting to the chrome window if it is over 1, sorta like we did before bug 1656753.
How tricky would it be to start tracking that context through frontend? It seems like it might be good to be explicit about these relationships so we don't show download dialogs in front of the wrong window if the tab is opened some other way (e.g. from extensions?).
Assignee | ||
Comment 9•4 years ago
|
||
(In reply to Nika Layzell [:nika] (ni? for response) from comment #8)
How tricky would it be to start tracking that context through frontend?
It's not super tricky to pass an extra arg (I guess canonical browsingcontext ref?) on the already huge params
dict. What I'm less sure about is how we'd communicate the opener to the docshell/browsingcontext code, ie at what point do we pass a browsing context - do we need to set it as an attribute on the browser
when creating that? Pass it to loadURI
? Have we even got a mechanism for this directly in the parent, or do we need to create one? I'm also not sure how we'd avoid exposing an opener relationship to web content (which I'm fairly sure we don't want to do here).
Comment 10•4 years ago
|
||
We definitely wouldn't want to start exposing this relationship to web content, the idea here would be that we only preserve this information for internal bookkeeping. There's currently no mechanism for communicating the exact value to the internal code, so we'd need to add one, but it wouldn't be too difficult to add. The easiest option is probably to expose a setCrossGroupOpener()
method on CanonicalBrowsingContext
, and then have frontend call it after inserting the <browser>
element, which should be fairly straightforward to do. We could also do an attribute or similar, but it seems unnecessary as we don't need to configure this value super early during context construction.
Assignee | ||
Updated•4 years ago
|
Comment 11•4 years ago
|
||
Comment 12•4 years ago
|
||
I've attached an example patch to expose the method which would be needed from JS, which hopefully can be built upon to fix this from frontend's side.
Assignee | ||
Updated•4 years ago
|
Comment 13•4 years ago
|
||
Nika's patch implements the platform changes. Rest of the fix is in frontend code.
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Assignee | ||
Comment 16•3 years ago
|
||
We're working on some related stuff atm, so making a note here that the tests for the existing functionality for _blank
etc. are in https://searchfox.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/browser_auto_close_window.js . They don't include middle clicks, but it shouldn't be too hard to add that. I'll try and get back to this "soon".
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 17•3 years ago
|
||
Depends on D100152
Assignee | ||
Comment 18•3 years ago
|
||
The patches here only fix the "new tab" (middle-click / ctrl-click / cmd-click) case, not the new window (shift-click) case. This is because passing the relevant information in that case is much less trivial (see also: bug 1485961), and because I think that case is much less frequent I didn't want to hold fixing the new tab case up for the "perfect" fix.
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 19•3 years ago
|
||
Comment 20•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5367cd1cdd71
https://hg.mozilla.org/mozilla-central/rev/3228d5319afc
Updated•3 years ago
|
Updated•3 years ago
|
Description
•