1798307 - Download prompt now focuses on the file type field by default

Status: NEW

(Firefox :: Downloads Panel, defect, P3)

Description

[aoia7rz7l]

Tested on 106.0.2.

STR:

1. Flip browser.download.always_ask_before_handling_new_types to true and potentially anything in the Applications section in about:preferences to Always Ask.
2. Click on any link on any page (e.g. https://ftp.mozilla.org/) that can trigger a download prompt.

Expected Behavior:

Download prompt focuses on the Cancel button by default.

Actual Behavior:

Download prompt focuses on the file type field by default.

Mozregression returned

Last good revision: 4f9407bedea97bd1849b92c2178fd40773c38e8d
First bad revision: 81580b85d04c46dd623d69cacf107375f84db3fc
Pushlog : https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4f9407bedea97bd1849b92c2178fd40773c38e8d&tochange=81580b85d04c46dd623d69cacf107375f84db3fc

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Firefox::Downloads Panel' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Oana Botisan, Desktop QA]

I can reproduce this issue on Firefox 106.0.3, Firefox 107.0b7 and Nightly 108.0a1 while using macOS 12.5 and Windows 10 x64.

Comment 3

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

Setting Regressed by field after analyzing regression range found by mozregression in comment #0.

Comment 4

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

Set release status flags based on info from the regressing bug 1740989

:emilio, since you are the author of the regressor, bug 1740989, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

That's due to this code change. Before my patch, we'd leave the non-default button (Cancel in this case) focused, which isn't the intention of that code at all.

I believe this is working as intended (focus the first focusable thing, otherwise the default button, otherwise nothing). But what happens when the default button is not focusable is kinda weird, maybe the default button of this dialog should be cancel?

Mike, Paul, does the above make sense? Should we switch this dialog to default to cancel? Who should take that decision? (Sorry, ni?ing based on blame in this dialog, which is ancient, STR is just going to here for example)

Comment 6

[Emma Zühlcke [:emz]]

I noticed that the external protocol handler permission dialog seems similar in that the accept button isn't focusable and it falls back to the first link label (choose application). Maybe we should address that too.
From a security perspective "cancel" seems like a sensible default (we would go to the previous behavior?). But I haven't worked on the download dialog before so I'll leave it to mconley, maybe gijs?

Comment 7

[:Gijs (he/him)]

(In reply to Paul Zühlcke [:pbz] from comment #6)

I noticed that the external protocol handler permission dialog seems similar in that the accept button isn't focusable and it falls back to the first link label (choose application). Maybe we should address that too.
From a security perspective "cancel" seems like a sensible default (we would go to the previous behavior?). But I haven't worked on the download dialog before so I'll leave it to mconley, maybe gijs?

bug 1749028 is relevant context here. So we should definitely not focus accept/OK.

The confusing thing is that if you change the default button in the dialog, that also impacts what [esc] and [return] do (at least on macOS, where that doesn't activate the focused button but the default button!!). You don't want [esc] to accept the dialog. But you do want the cancel button focused, or at least not the "OK" button.

Although this is a change, I'm not convinced it's really wrong to focus the file field? From a security pov, anything other than [OK] is fine. And [enter] should still accept the dialog once the OK button is not disabled, right?

Comment 8

[Emma Zühlcke [:emz]]

Shouldn't [esc] default to cancel and [return] to the default button for <dialog>?

Comment 9

[:Gijs (he/him)]

(In reply to Paul Zühlcke [:pbz] from comment #8)

Shouldn't [esc] default to cancel and [return] to the default button for <dialog>?

Yes, my point is if you made "cancel" the default button, then that's what [return] would do, and [esc] would hit the other button, potentially. IIRC this is why we now use the 'extra' button stuff in the external protocol dialog, but I don't have time to look that (and the sec bug where I changed it) up right now.

Comment 10

[Emma Zühlcke [:emz]]

Oh I see, yes this is a potential pitfall. The external protocol permission dialog sets the default button to "none" explicitly: https://searchfox.org/mozilla-central/rev/49011d374b626d5f0e7dc751a8a57365878e65f1/toolkit/mozapps/handling/content/permissionDialog.xhtml#13-14
So it probably behaves the same as this one.

Comment 11

[Pascal Chevrel:pascalc]

We are in RC week, wontfix 106. Could we get a severity set to this bug to help with regression triage? Thanks

Comment 12

[Donal Meehan [:dmeehan]]

Setting 107 to fix-optional since this is a relatively new bug. Though the specs are still in flux, so may not expect anything for 107.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Defaulting to Cancel certainly makes sense to me.

Comment 14

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

Set release status flags based on info from the regressing bug 1740989

Comment 15

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

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

For more information, please visit auto_nag documentation.

Comment 16

[Marco Bonardo [:mak]]

Based on comment 7 I don't think this is a critical problem, there is a workaround (Enter still works, can Tab or click on buttons) and the solution is still unclear. For sure we want to avoid focusing OK, as well as making Cancel the "default button" of the dialog.

Comment 17

[aoia7rz7l]

FWIW the download prompt is no longer focusing on the file type field by default since 107 release (at least visually), but it's still not focusing on the Cancel button by default.

mozregression --find-fix --good 107 --bad 106 returned

First good revision: 2ec569867b48dc6db1cdcf5fdfd6d24c4dd221b6
Last bad revision: 96677f0310d26cde83f0ed391b2630fc249c2372
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=96677f0310d26cde83f0ed391b2630fc249c2372&tochange=2ec569867b48dc6db1cdcf5fdfd6d24c4dd221b6