Bug 1676221 - Add UX nits to unblock UI r=ckerschb,Gijs

Show save dialogue
***
Automaticly show Panel On Block
***
Keep Open/Save Dialogue with open panel
***
Keep User choise in downloadsView
***
Add Unblock option to Unblock panel
***
lint
***
add test

Differential Revision: https://phabricator.services.mozilla.com/D96448

browser/components/downloads/DownloadsCommon.jsm

@@ -982,6 +982,9 @@ DownloadsDataCtor.prototype = {
       download,
       DownloadsCommon.stateOfDownload(download)
     );
+    if (download.error?.becauseBlockedByReputationCheck) {
+      this._notifyDownloadEvent("error");
+    }
   },
 
   onDownloadChanged(download) {
@@ -1070,7 +1073,8 @@ DownloadsDataCtor.prototype = {
    * window, if one is currently available with the required privacy type.
    *
    * @param aType
-   *        Set to "start" for new downloads, "finish" for completed downloads.
+   *        Set to "start" for new downloads, "finish" for completed downloads,
+   *        "error" for downloads that failed and need attention
    */
   _notifyDownloadEvent(aType) {
     DownloadsCommon.log(
@@ -1085,7 +1089,7 @@ DownloadsDataCtor.prototype = {
       return;
     }
 
-    if (this.panelHasShownBefore) {
+    if (this.panelHasShownBefore && aType != "error") {
       // For new downloads after the first one, don't show the panel
       // automatically, but provide a visible notification in the topmost
       // browser window, if the status indicator is already visible.

browser/components/downloads/DownloadsViewUI.jsm

@@ -537,11 +537,14 @@ DownloadsViewUI.DownloadElementShell.prototype = {
             // Downloads View, the interface depends on the threat severity.
             switch (verdict) {
               case Downloads.Error.BLOCK_VERDICT_UNCOMMON:
-                this.showButton("askOpenOrRemoveFile");
-                break;
               case Downloads.Error.BLOCK_VERDICT_INSECURE:
               case Downloads.Error.BLOCK_VERDICT_POTENTIALLY_UNWANTED:
+                // Keep the option the user chose on the save dialogue
+                if (this.download.launchWhenSucceeded) {
+                  this.showButton("askOpenOrRemoveFile");
+                } else {
                   this.showButton("askRemoveFileOrAllow");
+                }
                 break;
               default:
                 // Assume Downloads.Error.BLOCK_VERDICT_MALWARE
@@ -684,6 +687,9 @@ DownloadsViewUI.DownloadElementShell.prototype = {
     return this.download.unblock().then(() => this.downloadsCmd_open());
   },
 
+  unblockAndSave() {
+    return this.download.unblock();
+  },
   /**
    * Returns the name of the default command to use for the current state of the
    * download, when there is a double click or another default interaction. If
@@ -731,6 +737,7 @@ DownloadsViewUI.DownloadElementShell.prototype = {
       case "downloadsCmd_chooseUnblock":
       case "downloadsCmd_chooseOpen":
       case "downloadsCmd_unblock":
+      case "downloadsCmd_unblockAndSave":
       case "downloadsCmd_unblockAndOpen":
         return this.download.hasBlockedData;
       case "downloadsCmd_cancel":

browser/components/downloads/content/allDownloadsView.js

@@ -103,6 +103,9 @@ HistoryDownloadElementShell.prototype = {
   downloadsCmd_unblock() {
     this.confirmUnblock(window, "unblock");
   },
+  downloadsCmd_unblockAndSave() {
+    this.confirmUnblock(window, "unblock");
+  },
 
   downloadsCmd_chooseUnblock() {
     this.confirmUnblock(window, "chooseUnblock");

browser/components/downloads/content/downloads.js

@@ -1109,6 +1109,10 @@ class DownloadsViewItem extends DownloadsViewUI.DownloadElementShell {
     DownloadsPanel.hidePanel();
     this.unblockAndOpenDownload().catch(Cu.reportError);
   }
+  downloadsCmd_unblockAndSave() {
+    DownloadsPanel.hidePanel();
+    this.unblockAndSave();
+  }
 
   downloadsCmd_open(openWhere) {
     super.downloadsCmd_open(openWhere);
@@ -1209,7 +1213,11 @@ var DownloadsViewController = {
     // The currently supported commands depend on whether the blocked subview is
     // showing.  If it is, then take the following path.
     if (DownloadsView.subViewOpen) {
-      let blockedSubviewCmds = ["downloadsCmd_unblockAndOpen", "cmd_delete"];
+      let blockedSubviewCmds = [
+        "downloadsCmd_unblockAndOpen",
+        "cmd_delete",
+        "downloadsCmd_unblockAndSave",
+      ];
       return blockedSubviewCmds.includes(aCommand);
     }
     // If the blocked subview is not showing, then determine if focus is on a
@@ -1535,7 +1543,7 @@ var DownloadsBlockedSubview = {
       "title",
       "details1",
       "details2",
-      "openButton",
+      "unblockButton",
       "deleteButton",
     ];
     let elements = idSuffixes.reduce((memo, s) => {
@@ -1565,13 +1573,22 @@ var DownloadsBlockedSubview = {
   toggle(element, title, details) {
     DownloadsView.subViewOpen = true;
     DownloadsViewController.updateCommands();
+    const { download } = DownloadsView.itemForElement(element);
 
     let e = this.elements;
     let s = DownloadsCommon.strings;
     e.title.textContent = title;
     e.details1.textContent = details[0];
     e.details2.textContent = details[1];
-    e.openButton.label = s.unblockButtonOpen;
+
+    if (download.launchWhenSucceeded) {
+      e.unblockButton.label = s.unblockButtonOpen;
+      e.unblockButton.command = "downloadsCmd_unblockAndOpen";
+    } else {
+      e.unblockButton.label = s.unblockButtonUnblock;
+      e.unblockButton.command = "downloadsCmd_unblockAndSave";
+    }
+
     e.deleteButton.label = s.unblockButtonConfirmBlock;
 
     let verdict = element.getAttribute("verdict");

browser/components/downloads/content/downloadsPanel.inc.xhtml

@@ -16,6 +16,8 @@
            oncommand="goDoCommand('downloadsCmd_chooseUnblock')"/>
   <command id="downloadsCmd_unblockAndOpen"
            oncommand="goDoCommand('downloadsCmd_unblockAndOpen')"/>
+  <command id="downloadsCmd_unblockAndSave"
+           oncommand="goDoCommand('downloadsCmd_unblockAndSave')"/>
   <command id="downloadsCmd_confirmBlock"
            oncommand="goDoCommand('downloadsCmd_confirmBlock')"/>
   <command id="downloadsCmd_open"
@@ -174,7 +176,7 @@
       <hbox id="downloadsPanel-blockedSubview-buttons"
             class="panel-footer"
             align="stretch">
-        <button id="downloadsPanel-blockedSubview-openButton"
+        <button id="downloadsPanel-blockedSubview-unblockButton"
                 class="downloadsPanelFooterButton"
                 command="downloadsCmd_unblockAndOpen"
                 flex="1"/>

browser/components/downloads/test/browser/browser_downloads_panel_block.js

@@ -48,7 +48,7 @@ add_task(async function mainTest() {
     let unblockOpenPromise = promiseUnblockAndOpenDownloadCalled(item);
     let hidePromise = promisePanelHidden();
     EventUtils.synthesizeMouse(
-      DownloadsBlockedSubview.elements.openButton,
+      DownloadsBlockedSubview.elements.unblockButton,
       10,
       10,
       {},

dom/security/test/mixedcontentblocker/browser_test_mixed_content_download.js

@@ -3,6 +3,11 @@ ChromeUtils.defineModuleGetter(
   "Downloads",
   "resource://gre/modules/Downloads.jsm"
 );
+ChromeUtils.defineModuleGetter(
+  this,
+  "DownloadsCommon",
+  "resource:///modules/DownloadsCommon.jsm"
+);
 
 let INSECURE_BASE_URL =
   getRootDirectory(gTestPath).replace(
@@ -15,6 +20,27 @@ let SECURE_BASE_URL =
     "https://example.com/"
   ) + "download_page.html";
 
+function promiseFocus() {
+  return new Promise(resolve => {
+    waitForFocus(resolve);
+  });
+}
+
+function promisePanelOpened() {
+  if (DownloadsPanel.panel && DownloadsPanel.panel.state == "open") {
+    return Promise.resolve();
+  }
+  return BrowserTestUtils.waitForEvent(DownloadsPanel.panel, "popupshown");
+}
+
+async function task_openPanel() {
+  await promiseFocus();
+
+  let promise = promisePanelOpened();
+  DownloadsPanel.showPanel();
+  await promise;
+}
+
 function shouldPromptDownload() {
   // Waits until the download Prompt is shown
   return new Promise((resolve, reject) => {
@@ -27,9 +53,13 @@ function shouldPromptDownload() {
             win.location ==
             "chrome://mozapps/content/downloads/unknownContentType.xhtml"
           ) {
+            let dialog = win.document.getElementById("unknownContentType");
+            let button = dialog.getButton("accept");
+            let saveRadio = win.document.getElementById("save");
+            saveRadio.click();
+            button.disabled = false;
+            dialog.acceptDialog();
             resolve();
-            info("Trying to close window");
-            win.close();
           } else {
             reject();
           }
@@ -113,6 +143,7 @@ async function runTest(url, link, checkFunction, decscription) {
 
   await SpecialPowers.popPrefEnv();
 }
+
 // Test Blocking
 add_task(async function() {
   await runTest(
@@ -130,7 +161,12 @@ add_task(async function() {
   await runTest(
     SECURE_BASE_URL,
     "insecure",
-    () => Promise.all([shouldNotifyDownloadUI(), shouldConsoleError()]),
+    () =>
+      Promise.all([
+        shouldPromptDownload(),
+        shouldNotifyDownloadUI(),
+        shouldConsoleError(),
+      ]),
     "Secure -> Insecure should Error"
   );
   await runTest(
@@ -140,13 +176,13 @@ add_task(async function() {
     "Secure -> Secure should Download"
   );
 });
-
 // Test Manual Unblocking
 add_task(async function() {
   await runTest(
     SECURE_BASE_URL,
     "insecure",
     async () => {
+      await shouldPromptDownload();
       let download = await shouldNotifyDownloadUI();
       await download.unblock();
       ok(download.error == null, "There should be no error after unblocking");
@@ -154,3 +190,26 @@ add_task(async function() {
     "A Blocked Download Should succeeded to Download after a Manual unblock"
   );
 });
+
+// Test Unblock Download Visible
+add_task(async function() {
+  // Focus, open and close the panel once
+  // to make sure the panel is loaded and ready
+  await promiseFocus();
+  await runTest(
+    SECURE_BASE_URL,
+    "insecure",
+    async () => {
+      let panelHasOpened = promisePanelOpened();
+      info("awaiting that the Download Prompt is shown");
+      await shouldPromptDownload();
+      info("awaiting that the Download list adds the new download");
+      await shouldNotifyDownloadUI();
+      info("awaiting that the Download list shows itself");
+      await panelHasOpened;
+      DownloadsPanel.hidePanel();
+      ok(true, "The Download Panel should have opened on blocked download");
+    },
+    "A Blocked Download Should open the Download Panel"
+  );
+});

dom/security/test/mixedcontentblocker/download_server.sjs

@@ -4,6 +4,6 @@ function handleRequest(request, response)
 {
   response.setHeader("Cache-Control", "no-cache", false);
   response.setHeader("Content-Disposition", "attachment");
-  response.setHeader("Content-Type", "application/octet-stream");
+  response.setHeader("Content-Type", "image/png");
   response.write('🙈🙊🐵🙊');
 }

toolkit/components/downloads/DownloadCore.jsm

@@ -1006,6 +1006,15 @@ Download.prototype = {
           if (!this.stopped || this._finalized) {
             return;
           }
+          // In case we've blocked the Download becasue its
+          // insecure, we should not set hasBlockedData to
+          // false as its required to show the Unblock option.
+          if (
+            this.error.reputationCheckVerdict ==
+            DownloadError.BLOCK_VERDICT_INSECURE
+          ) {
+            return;
+          }
 
           this.hasBlockedData = false;
           this.hasPartialData = false;

toolkit/components/downloads/DownloadLegacy.jsm

@@ -337,12 +337,7 @@ DownloadLegacyTransfer.prototype = {
     browsingContextId = 0,
     handleInternally = false
   ) {
-    if (aDownloadClassification == Ci.nsITransfer.DOWNLOAD_ACCEPTABLE) {
-      // Only keep a refrence if it acceptable, as the download was canceled
-      // already if it isn't.
     this._cancelable = aCancelable;
-    }
-
     let launchWhenSucceeded = false,
       contentType = null,
       launcherPath = null;
@@ -397,6 +392,12 @@ DownloadLegacyTransfer.prototype = {
       // is already closed. A copy saver would create a new channel once
       // start() is called.
       serialisedDownload.saver = "copy";
+
+      // Since we now have the new Download reported in the list, we can
+      // cancel the original request.
+      this._download = null;
+      this._cancelable.cancel(Cr.NS_ERROR_ABORT);
+      this._cancelable = null;
     }
 
     Downloads.createDownload(serialisedDownload)

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -1676,15 +1676,12 @@ NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest* request) {
 
   mDownloadClassification =
       nsContentSecurityUtils::ClassifyDownload(aChannel, MIMEType);
-  if (mDownloadClassification != nsITransfer::DOWNLOAD_ACCEPTABLE) {
+
+  if (mDownloadClassification == nsITransfer::DOWNLOAD_FORBIDDEN) {
     // If the download is rated as forbidden,
-    // we need to silently cancel the request to make sure
-    // it wont show up in the download ui.
+    // cancel the request so no ui knows about this.
     mCanceled = true;
     request->Cancel(NS_ERROR_ABORT);
-    if (mDownloadClassification != nsITransfer::DOWNLOAD_FORBIDDEN) {
-      CreateFailedTransfer();
-    }
     return NS_OK;
   }
 
@@ -1918,6 +1915,9 @@ NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest* request) {
     }
   }
 
+  if (mDownloadClassification != nsITransfer::DOWNLOAD_ACCEPTABLE) {
+    request->Suspend();
+  }
   return NS_OK;
 }