1678965 - Downloads open up blank tab when middle-clicking a link that points to a download

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[jeremiah]

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.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dao, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[:Gijs (he/him)]

(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

[Chris Peterson [:cpeterson]]

Nika fixed a similar bug in Fx83 (bug 1656753). Perhaps this bug is a regression from that code change?

Comment 4

[jeremiah]

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!

Comment 5

[jeremiah]

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.

Comment 6

[jeremiah]

That's just the only page that I could think of quickly but it illustrates my point well.

Comment 7

[:Gijs (he/him)]

(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?

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b0c42fffcbc6a6e8ffb1fab4210b5f226295b945&tochange=737f0e3e2f948d46cc39b44185e26df3021ab6ee

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.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

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?).

Comment 9

[:Gijs (he/him)]

(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

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1678965 - Part 1: Expose SetCrossGroupOpener to chrome JS,

Comment 12

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 13

[Chris Peterson [:cpeterson]]

Nika's patch implements the platform changes. Rest of the fix is in frontend code.

Comment 14

[jeremiah]

Is this ever going to get fixed?

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

Hi Gijs, since you're worried about making FF "more compatible with other browsers," please fix this - it's an actual issue which should be addressed.

Thanks.

PS. please excuse me in advance for being snarky, I'm getting a bit tired of FF devs/UX people making short sighted decisions in order to make FF more like Chrome.

Comment 15

[:Gijs (he/him)]

(In reply to jeremiah from comment #14)

Is this ever going to get fixed?

Yes.

PS. please excuse me in advance for being snarky.

No, actually, you are not excused. Please re-read the participation guidelines. You not liking decisions we make doesn't mean we have to put up with you being snarky and abusive. Treating me and my colleagues poorly isn't going to achieve the result you want either way. The decisions you don't like also aren't directly related to this bug so complaining about them here is off-topic, so I'll hide both our comments.

Comment 16

[:Gijs (he/him)]

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".

Comment 17

[:Gijs (he/him)]

Attachment: Bug 1678965 - Part 2: set cross opener group for openLinkIn tabs opened as new tabs, r?dao

Depends on D100152

Comment 18

[:Gijs (he/him)]

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5367cd1cdd71
Part 1: Expose SetCrossGroupOpener to chrome JS, r=Gijs,farre
https://hg.mozilla.org/integration/autoland/rev/3228d5319afc
Part 2: set cross opener group for openLinkIn tabs opened as new tabs, r=dao

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5367cd1cdd71
https://hg.mozilla.org/mozilla-central/rev/3228d5319afc