1292566 - The "open" button in the subview for temporarily blocked downloads should not ask for confirmation

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P2)

Description

[:Paolo Amadini]

The Downloads Panel subview for blocked downloads is designed as a replacement and not an addition to the previous modal confirmation dialog, so the "open" button should not ask for confirmation.

Malware _may_ be a special case where an additional confirmation dialog can be wanted, we can clarify this with the designers. For uncommon and potentially unwanted downloads a modal dialog is surely not needed.

The presence of this additional dialog is a regression in user experience, because we show the same message too many times.

Comment 1

[:Paolo Amadini]

[Tracking Requested - why for this release]:
User interface regression with a new feature developed for Firefox 50 in bug 1252509.

Comment 2

[Drew Willcoxon :adw]

How is this a regression?  The feature didn't exist before 50.

Comment 3

[:Paolo Amadini]

The subview didn't exist, but malware download blocking was an existing feature. I've marked this explicitly as a regression so we don't lose track of it. It should be interpreted in the sense that the change to the existing feature inadvertently introduced a problem that didn't exist before, the repetition of the warning. Hope this clarifies!

Comment 4

[Drew Willcoxon :adw]

Attachment: Bug 1292566 - The "open" button in the subview for uncommon and potentially unwanted downloads should not ask for confirmation.

Review commit: https://reviewboard.mozilla.org/r/71352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71352/

Comment 5

[:Paolo Amadini]

(In reply to Drew Willcoxon :adw from comment #4)
> Review commit: https://reviewboard.mozilla.org/r/71352/diff/#index_header

Drew, sorry for the delay on this review. I still have to look at the code in detail, but the overall comment is to make sure that the same modal confirmation dialogs are still shown when the blocked download is handled from the Library window, where the subview is not present, and from the context menu. You may need to add a different command to handle this.

Comment 6

[:Paolo Amadini]

Comment on attachment 8780712 [details]
Bug 1292566 - The "open" button in the subview for uncommon and potentially unwanted downloads should not ask for confirmation.

https://reviewboard.mozilla.org/r/71352/#review71018

Sorry again for the late review, just a couple of fixes needed. I'll take a look at the next iteration but I don't expect any special difficulty.

::: browser/components/downloads/DownloadsViewUI.jsm:298
(Diff revision 1)
>        verdict: this.download.error.reputationCheckVerdict,
>        window,
>        dialogType,
>      }).then(action => {
>        if (action == "open") {
> -        return this.download.unblock().then(() => this.downloadsCmd_open());
> +        return this.openBlockedDownload();

"this.unblockAndOpenDownload()". Apparently we're not testing the dialog yet.

::: browser/components/downloads/content/downloads.js:1116
(Diff revision 1)
>    downloadsCmd_chooseUnblock() {
>      DownloadsPanel.hidePanel();
>      this.confirmUnblock(window, "chooseUnblock");
>    },
>  
>    downloadsCmd_chooseOpen() {

Rename to downloadsCmd_unblockAndOpen(), here and in downloadsOverlay.xul. The Library window will continue to use downloadsCmd_chooseOpen instead.

::: browser/components/downloads/content/downloads.js:1118
(Diff revision 1)
>      this.confirmUnblock(window, "chooseUnblock");
>    },
>  
>    downloadsCmd_chooseOpen() {
>      DownloadsPanel.hidePanel();
> -    this.confirmUnblock(window, "chooseOpen");
> +    this.unblockAndOpenDownload();

this.unblockAndOpenDownload().catch(Cu.reportError);

::: browser/components/downloads/test/browser/browser_downloads_panel_block.js:172
(Diff revision 1)
> +function promiseUnblockAndOpenDownloadCalled(item) {
> +  return new Promise(resolve => {
> +    let realFn = item._shell.unblockAndOpenDownload;
> +    item._shell.unblockAndOpenDownload = () => {
> +      item._shell.unblockAndOpenDownload = realFn;
> +      resolve();
> +      // unblockAndOpenDownload returns a promise (that's resolved when the file
> +      // is opened).
> +      return new Promise(r => r());
> +    };
> +  });
> +}

When bug 1269962 lands you'll be able to use "sinon.stub", however this alternative is fine for now because we'll need to uplift the patch to Aurora where it will not be available.

Note that you can return Promise.resolve() instead of new Promise(r => r()).

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8780712 [details]
Bug 1292566 - The "open" button in the subview for uncommon and potentially unwanted downloads should not ask for confirmation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71352/diff/1-2/

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8780712 [details]
Bug 1292566 - The "open" button in the subview for uncommon and potentially unwanted downloads should not ask for confirmation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71352/diff/2-3/

Comment 9

[:Paolo Amadini]

Comment on attachment 8780712 [details]
Bug 1292566 - The "open" button in the subview for uncommon and potentially unwanted downloads should not ask for confirmation.

https://reviewboard.mozilla.org/r/71352/#review71986

Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04efdec5c0cf
The "open" button in the subview for uncommon and potentially unwanted downloads should not ask for confirmation. r=Paolo

Comment 11

[[Back July 11] Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/04efdec5c0cf

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8780712 [details]
Bug 1292566 - The "open" button in the subview for uncommon and potentially unwanted downloads should not ask for confirmation.

Approval Request Comment
[Feature/regressing bug #]: Sliding subview for blocked download info in the downloads panel, bug 1252509
[User impact if declined]: The "open" button in the subview for blocked downloads will ask for confirmation
[Describe test coverage new/current, TreeHerder]: mochitest
[Risks and why]: Low risk, has automated and manual testing
[String/UUID change made/needed]: None

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Paolo, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8780712 [details]
Bug 1292566 - The "open" button in the subview for uncommon and potentially unwanted downloads should not ask for confirmation.

Fixes a regression in a new feature planned for Fx50.

Comment 15

[:Paolo Amadini]

Tested that the issue is solved on the latest Nightly on Windows 7.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bac8c738419e

Comment 17

[Paul Silaghi, QA [:pauly]]

(In reply to :Paolo Amadini from comment #0)
> The Downloads Panel subview for blocked downloads is designed as a
> replacement and not an addition to the previous modal confirmation dialog,
> so the "open" button should not ask for confirmation.
The title of the bug specifies "uncommon and potentially unwanted" only.
Now the "open" button doesn't ask for confirmation for any type of bad download (malicious, dangerous etc). Is this correct?

Comment 18

[Drew Willcoxon :adw]

(In reply to Paul Silaghi, QA [:pauly] from comment #17)
> The title of the bug specifies "uncommon and potentially unwanted" only.
> Now the "open" button doesn't ask for confirmation for any type of bad
> download (malicious, dangerous etc). Is this correct?

Yes, that's my understanding, but to be safe let's ask Paolo.

Comment 19

[:Paolo Amadini]

Yeah, malware also should not ask for confirmation with the new design, because more information about the choice is already provided in the subview. I've updated the subject to match.

Comment 20

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 50b2, 51.0a2 (2016-09-28) Win 7 using http://testsafebrowsing.appspot.com/