Bug 1732446 - fix multiple tab warning for quit shortcut usage on Windows, r=NeilDeakin,mhowell

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

browser/components/BrowserGlue.jsm

@@ -3002,14 +3002,12 @@ BrowserGlue.prototype = {
     // The warning will appear even when only one window/tab is open. For other
     // methods of quitting, the warning only appears when there is more than one
     // window or tab open.
-    if (this._quitSource == "shortcut") {
-      if (!Services.prefs.getBoolPref("browser.warnOnQuitShortcut")) {
-        return;
-      }
-    } else if (
-      pagecount < 2 ||
-      !Services.prefs.getBoolPref("browser.tabs.warnOnClose")
-    ) {
+    let shouldWarnForShortcut =
+      this._quitSource == "shortcut" &&
+      Services.prefs.getBoolPref("browser.warnOnQuitShortcut");
+    let shouldWarnForTabs =
+      pagecount >= 2 && Services.prefs.getBoolPref("browser.tabs.warnOnClose");
+    if (!shouldWarnForTabs && !shouldWarnForShortcut) {
       return;
     }
 
@@ -3037,7 +3035,7 @@ BrowserGlue.prototype = {
       title = gTabbrowserBundle.GetStringFromName("tabs.closeTabsTitle");
       title = PluralForm.get(pagecount, title).replace("#1", pagecount);
 
-      if (this._quitSource == "shortcut") {
+      if (shouldWarnForShortcut) {
         let productName = gBrandBundle.GetStringFromName("brandShorterName");
         title = gTabbrowserBundle.formatStringFromName(
           "tabs.closeTabsWithKeyTitle",
@@ -3059,7 +3057,7 @@ BrowserGlue.prototype = {
     // The checkbox label is different depending on whether the shortcut
     // was used to quit or not.
     let checkboxLabel;
-    if (this._quitSource == "shortcut") {
+    if (shouldWarnForShortcut) {
       let quitKeyElement = win.document.getElementById("key_quitApplication");
       let quitKey = ShortcutUtils.prettifyShortcut(quitKeyElement);
 
@@ -3113,7 +3111,7 @@ BrowserGlue.prototype = {
     // If the user has unticked the box, and has confirmed closing, stop showing
     // the warning.
     if (buttonPressed == 0 && !warnOnClose.value) {
-      if (this._quitSource == "shortcut") {
+      if (shouldWarnForShortcut) {
         Services.prefs.setBoolPref("browser.warnOnQuitShortcut", false);
       } else {
         Services.prefs.setBoolPref("browser.tabs.warnOnClose", false);

browser/components/tests/browser/browser.ini

@@ -13,6 +13,7 @@ reason = test depends on update channel
 [browser_default_browser_prompt.js]
 [browser_initial_tab_remoteType.js]
 https_first_disabled = true
+[browser_quit_multiple_tabs.js]
 [browser_startup_homepage.js]
 [browser_quit_disabled.js]
 # On macOS we can't change browser.quitShortcut.disabled during runtime.

browser/components/tests/browser/browser_quit_multiple_tabs.js

@@ -0,0 +1,110 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Ensure that when different combinations of warnings are enabled,
+ * quitting produces the correct warning (if any), and the checkbox
+ * is also correct.
+ */
+add_task(async function test_check_right_prompt() {
+  let tests = [
+    {
+      warnOnQuitShortcut: true,
+      warnOnClose: false,
+      expectedDialog: "shortcut",
+      messageSuffix: "with shortcut but no tabs warning",
+    },
+    {
+      warnOnQuitShortcut: false,
+      warnOnClose: true,
+      expectedDialog: "tabs",
+      messageSuffix: "with tabs but no shortcut warning",
+    },
+    {
+      warnOnQuitShortcut: false,
+      warnOnClose: false,
+      messageSuffix: "with no warning",
+      expectedDialog: null,
+    },
+    {
+      warnOnQuitShortcut: true,
+      warnOnClose: true,
+      messageSuffix: "with both warnings",
+      // Note: this is somewhat arbitrary; I don't think there's a right/wrong
+      // here, so if this changes due to implementation details, updating the
+      // text expectation to be "tabs" should be OK.
+      expectedDialog: "shortcut",
+    },
+  ];
+  let tab = BrowserTestUtils.addTab(gBrowser);
+
+  function checkDialog(dialog, expectedDialog, messageSuffix) {
+    let dialogElement = dialog.document.getElementById("commonDialog");
+    let acceptLabel = dialogElement.getButton("accept").label;
+    is(
+      acceptLabel.startsWith("Quit"),
+      expectedDialog == "shortcut",
+      `dialog label ${
+        expectedDialog == "shortcut" ? "should" : "should not"
+      } start with Quit ${messageSuffix}`
+    );
+    let checkLabel = dialogElement.querySelector("checkbox").label;
+    is(
+      checkLabel.includes("before quitting with"),
+      expectedDialog == "shortcut",
+      `checkbox label ${
+        expectedDialog == "shortcut" ? "should" : "should not"
+      } be for quitting ${messageSuffix}`
+    );
+
+    dialogElement.getButton("cancel").click();
+  }
+
+  let dialogOpened = false;
+  function setDialogOpened() {
+    dialogOpened = true;
+  }
+  Services.obs.addObserver(setDialogOpened, "common-dialog-loaded");
+  for (let {
+    warnOnClose,
+    warnOnQuitShortcut,
+    expectedDialog,
+    messageSuffix,
+  } of tests) {
+    dialogOpened = false;
+    let promise = null;
+    await SpecialPowers.pushPrefEnv({
+      set: [
+        ["browser.tabs.warnOnClose", warnOnClose],
+        ["browser.warnOnQuitShortcut", warnOnQuitShortcut],
+        ["browser.warnOnQuit", true],
+      ],
+    });
+    if (expectedDialog) {
+      promise = BrowserTestUtils.promiseAlertDialogOpen("", undefined, {
+        callback(win) {
+          checkDialog(win, expectedDialog, messageSuffix);
+        },
+      });
+    }
+    is(
+      !canQuitApplication(undefined, "shortcut"),
+      !!expectedDialog,
+      `canQuitApplication ${
+        expectedDialog ? "should" : "should not"
+      } block ${messageSuffix}.`
+    );
+    await promise;
+    is(
+      dialogOpened,
+      !!expectedDialog,
+      `Should ${
+        expectedDialog ? "" : "not "
+      }have opened a dialog ${messageSuffix}.`
+    );
+  }
+  Services.obs.removeObserver(setDialogOpened, "common-dialog-loaded");
+  BrowserTestUtils.removeTab(tab);
+});