Bug 1652641 - use consistent access key for tab context menu's items …

…to close 1 tab or selected tabs, r=dao,fluent-reviewers,flod Differential Revision: https://phabricator.services.mozilla.com/D83628
dothq · Jul 20, 2020 · e21c0bc · e21c0bc
1 parent 56b100b
commit e21c0bc
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 37 deletions.
diff --git a/browser/base/content/browser.xhtml b/browser/base/content/browser.xhtml
@@ -165,7 +165,9 @@
         <menupopup oncommand="TabContextMenu.reopenInContainer(event);"
                    onpopupshowing="TabContextMenu.createReopenInContainerMenu(event);"/>
       </menu>
-      <menu id="context_moveTabOptions">
+      <menu id="context_moveTabOptions"
+            data-lazy-l10n-id="tab-context-move-tabs"
+            data-l10n-args='{"tabCount": 1}'>
         <menupopup id="moveTabOptionsMenu">
           <menuitem id="context_moveToStart"
                     data-lazy-l10n-id="move-to-start"
@@ -199,11 +201,10 @@
                 data-lazy-l10n-id="tab-context-undo-close-tabs"
                 data-l10n-args='{"tabCount": 1}'
                 observes="History:UndoCloseTab"/>
-      <menuitem id="context_closeTab" data-lazy-l10n-id="close-tab"
-                oncommand="gBrowser.removeTab(TabContextMenu.contextTab, { animate: true });"/>
-      <menuitem id="context_closeSelectedTabs" data-lazy-l10n-id="close-tabs"
-                hidden="true"
-                oncommand="gBrowser.removeMultiSelectedTabs();"/>
+      <menuitem id="context_closeTab"
+                data-lazy-l10n-id="tab-context-close-tabs"
+                data-l10n-args='{"tabCount": 1}'
+                oncommand="TabContextMenu.closeContextTabs();"/>
     </menupopup>
 
     <!-- bug 415444/582485: event.stopPropagation is here for the cloned version

diff --git a/browser/base/content/tabbrowser.js b/browser/base/content/tabbrowser.js
@@ -6500,6 +6500,9 @@ var TabContextMenu = {
 
     let disabled = gBrowser.tabs.length == 1;
     let multiselectionContext = this.contextTab.multiselected;
+    let tabCountInfo = JSON.stringify({
+      tabCount: (multiselectionContext && gBrowser.multiSelectedTabsCount) || 1,
+    });
 
     var menuItems = aPopupMenu.getElementsByAttribute(
       "tbattr",
@@ -6551,11 +6554,8 @@ var TabContextMenu = {
     let contextMoveTabOptions = document.getElementById(
       "context_moveTabOptions"
     );
+    contextMoveTabOptions.setAttribute("data-l10n-args", tabCountInfo);
     contextMoveTabOptions.disabled = gBrowser.allTabsSelected();
-    document.l10n.setAttributes(
-      contextMoveTabOptions,
-      multiselectionContext ? "move-tabs" : "move-tab"
-    );
     let selectedTabs = gBrowser.selectedTabs;
     let contextMoveTabToEnd = document.getElementById("context_moveToEnd");
     let allSelectedTabsAdjacent = selectedTabs.every(
@@ -6607,11 +6607,10 @@ var TabContextMenu = {
     document.getElementById("context_closeOtherTabs").disabled =
       unpinnedTabsToClose < 1;
 
-    // Only one of close_tab/close_selected_tabs should be visible
-    document.getElementById("context_closeTab").hidden = multiselectionContext;
-    document.getElementById(
-      "context_closeSelectedTabs"
-    ).hidden = !multiselectionContext;
+    // Update the close item with how many tabs will close.
+    document
+      .getElementById("context_closeTab")
+      .setAttribute("data-l10n-args", tabCountInfo);
 
     // Hide "Bookmark Tab" for multiselection.
     // Update its state if visible.
@@ -6776,4 +6775,12 @@ var TabContextMenu = {
       }
     }
   },
+
+  closeContextTabs(event) {
+    if (this.contextTab.multiselected) {
+      gBrowser.removeMultiSelectedTabs();
+    } else {
+      gBrowser.removeTab(this.contextTab, { animate: true });
+    }
+  },
 };
diff --git a/browser/base/content/test/tabs/browser_multiselect_tabs_close.js b/browser/base/content/test/tabs/browser_multiselect_tabs_close.js
@@ -1,5 +1,19 @@
 const PREF_WARN_ON_CLOSE = "browser.tabs.warnOnCloseOtherTabs";
 
+async function openTabMenuFor(tab) {
+  let tabMenu = tab.ownerDocument.getElementById("tabContextMenu");
+
+  let tabMenuShown = BrowserTestUtils.waitForEvent(tabMenu, "popupshown");
+  EventUtils.synthesizeMouseAtCenter(
+    tab,
+    { type: "contextmenu" },
+    tab.ownerGlobal
+  );
+  await tabMenuShown;
+
+  return tabMenu;
+}
+
 add_task(async function setPref() {
   await SpecialPowers.pushPrefEnv({
     set: [[PREF_WARN_ON_CLOSE, false]],
@@ -67,10 +81,6 @@ add_task(async function usingTabContextMenu() {
   let tab4 = await addTab();
 
   let menuItemCloseTab = document.getElementById("context_closeTab");
-  let menuItemCloseSelectedTabs = document.getElementById(
-    "context_closeSelectedTabs"
-  );
-
   is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs");
 
   await BrowserTestUtils.switchTab(gBrowser, tab1);
@@ -84,17 +94,20 @@ add_task(async function usingTabContextMenu() {
 
   // Check the context menu with a non-multiselected tab
   updateTabContextMenu(tab4);
-  is(menuItemCloseTab.hidden, false, "Close Tab is visible");
-  is(menuItemCloseSelectedTabs.hidden, true, "Close Selected Tabs is hidden");
+  let { args } = document.l10n.getAttributes(menuItemCloseTab);
+  is(args.tabCount, 1, "Close Tab item lists a single tab");
 
-  // Check the context menu with a multiselected tab
-  updateTabContextMenu(tab2);
-  is(menuItemCloseTab.hidden, true, "Close Tab is hidden");
-  is(menuItemCloseSelectedTabs.hidden, false, "Close Selected Tabs is visible");
+  // Check the context menu with a multiselected tab. We have to actually open
+  // it (not just call `updateTabContextMenu`) in order for
+  // `TabContextMenu.contextTab` to stay non-null when we click an item.
+  let menu = openTabMenuFor(tab2);
+  ({ args } = document.l10n.getAttributes(menuItemCloseTab));
+  is(args.tabCount, 2, "Close Tab item lists more than one tab");
 
   let tab1Closing = BrowserTestUtils.waitForTabClosing(tab1);
   let tab2Closing = BrowserTestUtils.waitForTabClosing(tab2);
-  menuItemCloseSelectedTabs.click();
+  menuItemCloseTab.click();
+  menu.hidePopup();
   await tab1Closing;
   await tab2Closing;
 

diff --git a/browser/locales/en-US/browser/tabContextMenu.ftl b/browser/locales/en-US/browser/tabContextMenu.ftl
@@ -56,22 +56,27 @@ move-to-new-window =
 tab-context-close-multiple-tabs =
     .label = Close Multiple Tabs
     .accesskey = M
+
+## Variables:
+##  $tabCount (Number): the number of tabs that are affected by the action.
 tab-context-undo-close-tabs =
     .label =
         { $tabCount ->
             [1] Undo Close Tab
            *[other] Undo Close Tabs
         }
     .accesskey = U
-close-tab =
-    .label = Close Tab
-    .accesskey = c
-close-tabs =
-    .label = Close Tabs
-    .accesskey = S
-move-tabs =
-    .label = Move Tabs
-    .accesskey = v
-move-tab =
-    .label = Move Tab
+tab-context-close-tabs =
+    .label =
+        { $tabCount ->
+            [1] Close Tab
+           *[other] Close Tabs
+        }
+    .accesskey = C
+tab-context-move-tabs =
+    .label =
+        { $tabCount ->
+            [1] Move Tab
+           *[other] Move Tabs
+        }
     .accesskey = v