Bug 1480529 - Changed 'Bookmark All Tabs' to 'Bookmark Tab' for single tab selections. r=Felipe
authorJared Wein <jwein@mozilla.com>
Tue, 02 Oct 2018 16:24:43 +0000
changeset 494942 de5c2979877f9f7fb8731a86ad52aacf0c5f1082
parent 494941 07184f24860bafda10d0eac583236ae2e4544fef
child 494943 6a7574d2dfb8f5f7bff7ae8aab3a60fbfca8fdbf
push id9984
push userffxbld-merge
push dateMon, 15 Oct 2018 21:07:35 +0000
treeherdermozilla-beta@183d27ea8570 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersFelipe
bugs1480529
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1480529 - Changed 'Bookmark All Tabs' to 'Bookmark Tab' for single tab selections. r=Felipe I changed the toolbar context menuitem from 'Bookmark All Tabs' to 'Bookmark Selected Tabs' because it is separated from a specific tab and thus it is not clear which tab would get bookmarked if only one is selected. This seemed much clearer to me in my testing. The wording of 'Bookmark Selected Tabs' is already used elsewhere where we have 'Reload Selected Tabs', 'Close Selected Tabs', etc. Differential Revision: https://phabricator.services.mozilla.com/D7217
browser/base/content/browser-places.js
browser/base/content/browser-sets.inc
browser/base/content/browser.js
browser/base/content/browser.xul
browser/base/content/tabbrowser.js
browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js
browser/base/content/test/tabs/browser_visibleTabs_bookmarkAllTabs.js
browser/components/customizableui/test/browser_customization_context_menus.js
browser/locales/en-US/chrome/browser/browser.dtd
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -485,37 +485,31 @@ var PlacesCommandHook = {
   get uniqueSelectedPages() {
     return this.getUniquePages(gBrowser.selectedTabs);
   },
 
   /**
    * Adds a folder with bookmarks to URIList given in param.
    */
   bookmarkPages(URIList) {
-    if (URIList.length > 1) {
-      PlacesUIUtils.showBookmarkDialog({ action: "add",
-                                         type: "folder",
-                                         URIList,
-                                       }, window);
+    if (!URIList.length) {
+      return;
     }
-  },
 
-  /**
-   * Updates disabled state for the "Bookmark All Tabs" command.
-   */
-  updateBookmarkAllTabsCommand:
-  function PCH_updateBookmarkAllTabsCommand() {
-    // There's nothing to do in non-browser windows.
-    if (window.location.href != AppConstants.BROWSER_CHROME_URL)
-      return;
+    let bookmarkDialogInfo = {action: "add"};
+    if (URIList.length > 1) {
+      bookmarkDialogInfo.type = "folder";
+      bookmarkDialogInfo.URIList = URIList;
+    } else {
+      bookmarkDialogInfo.type = "bookmark";
+      bookmarkDialogInfo.title = URIList[0].title;
+      bookmarkDialogInfo.uri = URIList[0].uri;
+    }
 
-    // Disable "Bookmark All Tabs" if there are less than two
-    // "unique current pages".
-    goSetCommandEnabled("Browser:BookmarkAllTabs",
-                        this.uniqueCurrentPages.length >= 2);
+    PlacesUIUtils.showBookmarkDialog(bookmarkDialogInfo, window);
   },
 
   /**
    * Adds a Live Bookmark to a feed associated with the current page.
    * @param     url
    *            The nsIURI of the page the feed was attached to
    * @title     title
    *            The title of the feed. Optional.
@@ -1564,17 +1558,16 @@ var BookmarkingUI = {
   },
 
   onMainMenuPopupShowing: function BUI_onMainMenuPopupShowing(event) {
     // Don't handle events for submenus.
     if (event.target != event.currentTarget)
       return;
 
     this.updateBookmarkPageMenuItem();
-    PlacesCommandHook.updateBookmarkAllTabsCommand();
     this._initMobileBookmarks(document.getElementById("menu_mobileBookmarks"));
   },
 
   showSubView(anchor) {
     this._showSubView(null, anchor);
   },
 
   _showSubView(event, anchor = document.getElementById(this.BOOKMARK_BUTTON_ID)) {
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -50,18 +50,16 @@
     <command id="cmd_findAgain" oncommand="gLazyFindCommand('onFindAgainCommand', false)"/>
     <command id="cmd_findPrevious" oncommand="gLazyFindCommand('onFindAgainCommand', true)"/>
 #ifdef XP_MACOSX
     <command id="cmd_findSelection" oncommand="gLazyFindCommand('onFindSelectionCommand')"/>
 #endif
     <!-- work-around bug 392512 -->
     <command id="Browser:AddBookmarkAs"
              oncommand="PlacesCommandHook.bookmarkPage();"/>
-    <!-- The command disabled state must be manually updated through
-         PlacesCommandHook.updateBookmarkAllTabsCommand() -->
     <command id="Browser:BookmarkAllTabs"
              oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueCurrentPages);"/>
     <command id="Browser:Back"    oncommand="BrowserBack();" disabled="true"/>
     <command id="Browser:BackOrBackDuplicate" oncommand="BrowserBack(event);" disabled="true">
       <observes element="Browser:Back" attribute="disabled"/>
     </command>
     <command id="Browser:Forward" oncommand="BrowserForward();" disabled="true"/>
     <command id="Browser:ForwardOrForwardDuplicate" oncommand="BrowserForward(event);" disabled="true">
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -5701,18 +5701,16 @@ function onViewToolbarsPopupShowing(aEve
     node.hidden = showTabStripItems;
   }
 
   for (let node of popup.querySelectorAll('menuitem[contexttype="tabbar"]')) {
     node.hidden = !showTabStripItems;
   }
 
   if (showTabStripItems) {
-    PlacesCommandHook.updateBookmarkAllTabsCommand();
-
     let haveMultipleTabs = gBrowser.visibleTabs.length > 1;
     document.getElementById("toolbar-context-reloadAllTabs").disabled = !haveMultipleTabs;
 
     document.getElementById("toolbar-context-selectAllTabs").disabled = gBrowser.allTabsSelected();
     document.getElementById("toolbar-context-undoCloseTab").disabled =
       SessionStore.getClosedTabCount(window) == 0;
     return;
   }
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -112,16 +112,25 @@ xmlns="http://www.w3.org/1999/xhtml"
                 accesskey="&reloadSelectedTabs.accesskey;"
                 oncommand="gBrowser.reloadMultiSelectedTabs();"/>
       <menuitem id="context_toggleMuteTab" oncommand="TabContextMenu.contextTab.toggleMuteAudio();"/>
       <menuitem id="context_toggleMuteSelectedTabs" hidden="true"
                 oncommand="gBrowser.toggleMuteAudioOnMultiSelectedTabs(TabContextMenu.contextTab);"/>
       <menuseparator/>
       <menuitem id="context_selectAllTabs" label="&selectAllTabs.label;" accesskey="&selectAllTabs.accesskey;"
                 oncommand="gBrowser.selectAllTabs();"/>
+      <menuitem id="context_bookmarkSelectedTabs"
+                hidden="true"
+                label="&bookmarkSelectedTabs.label;"
+                accesskey="&bookmarkSelectedTabs.accesskey;"
+                oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueSelectedPages);"/>
+      <menuitem id="context_bookmarkTab"
+                label="&bookmarkTab.label;"
+                accesskey="&bookmarkTab.accesskey;"
+                oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.getUniquePages([TabContextMenu.contextTab]));"/>
       <menuitem id="context_pinTab" label="&pinTab.label;"
                 accesskey="&pinTab.accesskey;"
                 oncommand="gBrowser.pinTab(TabContextMenu.contextTab);"/>
       <menuitem id="context_unpinTab" label="&unpinTab.label;" hidden="true"
                 accesskey="&unpinTab.accesskey;"
                 oncommand="gBrowser.unpinTab(TabContextMenu.contextTab);"/>
       <menuitem id="context_pinSelectedTabs" label="&pinSelectedTabs.label;" hidden="true"
                 accesskey="&pinSelectedTabs.accesskey;"
@@ -148,25 +157,16 @@ xmlns="http://www.w3.org/1999/xhtml"
             class="sync-ui-item">
         <menupopup id="context_sendTabToDevicePopupMenu"
                    onpopupshowing="gSync.populateSendTabToDevicesMenu(event.target, TabContextMenu.contextTab);"/>
       </menu>
       <menuseparator/>
       <menuitem id="context_reloadAllTabs" label="&reloadAllTabs.label;" accesskey="&reloadAllTabs.accesskey;"
                 tbattr="tabbrowser-multiple-visible"
                 oncommand="gBrowser.reloadAllTabs();"/>
-       <menuitem id="context_bookmarkSelectedTabs"
-                hidden="true"
-                label="&bookmarkSelectedTabs.label;"
-                accesskey="&bookmarkSelectedTabs.accesskey;"
-                oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueSelectedPages);"/>
-      <menuitem id="context_bookmarkAllTabs"
-                label="&bookmarkAllTabs.label;"
-                accesskey="&bookmarkAllTabs.accesskey;"
-                command="Browser:BookmarkAllTabs"/>
       <menuitem id="context_closeTabsToTheEnd" label="&closeTabsToTheEnd.label;" accesskey="&closeTabsToTheEnd.accesskey;"
                 oncommand="gBrowser.removeTabsToTheEndFrom(TabContextMenu.contextTab, {animate: true});"/>
       <menuitem id="context_closeOtherTabs" label="&closeOtherTabs.label;" accesskey="&closeOtherTabs.accesskey;"
                 oncommand="gBrowser.removeAllTabsBut(TabContextMenu.contextTab);"/>
       <menuseparator/>
       <menuitem id="context_undoCloseTab"
                 label="&undoCloseTab.label;"
                 accesskey="&undoCloseTab.accesskey;"
@@ -412,22 +412,22 @@ xmlns="http://www.w3.org/1999/xhtml"
                 contexttype="toolbaritem"
                 class="customize-context-removeFromToolbar"/>
       <menuitem id="toolbar-context-reloadAllTabs"
                 class="toolbaritem-tabsmenu"
                 contexttype="tabbar"
                 oncommand="gBrowser.reloadAllTabs();"
                 label="&toolbarContextMenu.reloadAllTabs.label;"
                 accesskey="&toolbarContextMenu.reloadAllTabs.accesskey;"/>
-      <menuitem id="toolbar-context-bookmarkAllTabs"
+      <menuitem id="toolbar-context-bookmarkSelectedTabs"
                 class="toolbaritem-tabsmenu"
                 contexttype="tabbar"
-                command="Browser:BookmarkAllTabs"
-                label="&toolbarContextMenu.bookmarkAllTabs.label;"
-                accesskey="&toolbarContextMenu.bookmarkAllTabs.accesskey;"/>
+                oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueSelectedPages);"
+                label="&toolbarContextMenu.bookmarkSelectedTabs.label;"
+                accesskey="&toolbarContextMenu.bookmarkSelectedTabs.accesskey;"/>
       <menuitem id="toolbar-context-selectAllTabs"
                 class="toolbaritem-tabsmenu"
                 contexttype="tabbar"
                 oncommand="gBrowser.selectAllTabs();"
                 label="&toolbarContextMenu.selectAllTabs.label;"
                 accesskey="&toolbarContextMenu.selectAllTabs.accesskey;"/>
       <menuitem id="toolbar-context-undoCloseTab"
                 class="toolbaritem-tabsmenu"
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -5338,23 +5338,20 @@ var TabContextMenu = {
       gBrowser.visibleTabs.filter(t => !t.multiselected && !t.pinned).length :
       gBrowser.visibleTabs.filter(t => t != this.contextTab && !t.pinned).length;
     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;
 
-    // Hide "Bookmark All Tabs" for a pinned tab or multiselection.
+    // Hide "Bookmark Tab" for multiselection.
     // Update its state if visible.
-    let bookmarkAllTabs = document.getElementById("context_bookmarkAllTabs");
-    bookmarkAllTabs.hidden = this.contextTab.pinned || multiselectionContext;
-    if (!bookmarkAllTabs.hidden) {
-      PlacesCommandHook.updateBookmarkAllTabsCommand();
-    }
+    let bookmarkTab = document.getElementById("context_bookmarkTab");
+    bookmarkTab.hidden = multiselectionContext;
 
     // Show "Bookmark Selected Tabs" in a multiselect context and hide it otherwise.
     let bookmarkMultiSelectedTabs = document.getElementById("context_bookmarkSelectedTabs");
     bookmarkMultiSelectedTabs.hidden = !multiselectionContext;
 
     let toggleMute = document.getElementById("context_toggleMuteTab");
     let toggleMultiSelectMute = document.getElementById("context_toggleMuteSelectedTabs");
 
--- a/browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js
+++ b/browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js
@@ -18,47 +18,47 @@ add_task(async function setPref() {
   });
 });
 
 add_task(async function test() {
   let tab1 = await addTab();
   let tab2 = await addTab();
   let tab3 = await addTab();
 
-  let menuItemBookmarkAllTabs = document.getElementById("context_bookmarkAllTabs");
+  let menuItemBookmarkTab = document.getElementById("context_bookmarkTab");
   let menuItemBookmarkSelectedTabs = document.getElementById("context_bookmarkSelectedTabs");
 
   is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs");
 
   await BrowserTestUtils.switchTab(gBrowser, tab1);
   await triggerClickOn(tab2, { ctrlKey: true });
 
   ok(tab1.multiselected, "Tab1 is multiselected");
   ok(tab2.multiselected, "Tab2 is multiselected");
   ok(!tab3.multiselected, "Tab3 is not multiselected");
 
   // Check the context menu with a non-multiselected tab
   updateTabContextMenu(tab3);
-  is(menuItemBookmarkAllTabs.hidden, false, "Bookmark All Tabs is visible");
+  is(menuItemBookmarkTab.hidden, false, "Bookmark Tab is visible");
   is(menuItemBookmarkSelectedTabs.hidden, true, "Bookmark Selected Tabs is hidden");
 
   // Check the context menu with a multiselected tab and one unique page in the selection.
   updateTabContextMenu(tab2);
-  is(menuItemBookmarkAllTabs.hidden, true, "Bookmark All Tabs is visible");
+  is(menuItemBookmarkTab.hidden, true, "Bookmark Tab is visible");
   is(menuItemBookmarkSelectedTabs.hidden, false, "Bookmark Selected Tabs is hidden");
   is(PlacesCommandHook.uniqueSelectedPages.length, 1, "No more than one unique selected page");
 
   info("Add a different page to selection");
   let tab4 = await addTab_example_com();
   await triggerClickOn(tab4, { ctrlKey: true });
 
   ok(tab4.multiselected, "Tab4 is multiselected");
   is(gBrowser.multiSelectedTabsCount, 3, "Three multiselected tabs");
 
   // Check the context menu with a multiselected tab and two unique pages in the selection.
   updateTabContextMenu(tab2);
-  is(menuItemBookmarkAllTabs.hidden, true, "Bookmark All Tabs is visible");
+  is(menuItemBookmarkTab.hidden, true, "Bookmark Tab is visible");
   is(menuItemBookmarkSelectedTabs.hidden, false, "Bookmark Selected Tabs is hidden");
   is(PlacesCommandHook.uniqueSelectedPages.length, 2, "More than one unique selected page");
 
   for (let tab of [tab1, tab2, tab3, tab4])
     BrowserTestUtils.removeTab(tab);
 });
--- a/browser/base/content/test/tabs/browser_visibleTabs_bookmarkAllTabs.js
+++ b/browser/base/content/test/tabs/browser_visibleTabs_bookmarkAllTabs.js
@@ -3,63 +3,50 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 function test() {
   waitForExplicitFinish();
 
   // There should be one tab when we start the test
   let [origTab] = gBrowser.visibleTabs;
   is(gBrowser.visibleTabs.length, 1, "1 tab should be open");
-  is(Disabled(), true, "Bookmark All Tabs should be disabled");
 
   // Add a tab
   let testTab1 = BrowserTestUtils.addTab(gBrowser);
   is(gBrowser.visibleTabs.length, 2, "2 tabs should be open");
-  is(Disabled(), true, "Bookmark All Tabs should be disabled since there are two tabs with the same address");
 
   let testTab2 = BrowserTestUtils.addTab(gBrowser, "about:mozilla");
   is(gBrowser.visibleTabs.length, 3, "3 tabs should be open");
   // Wait for tab load, the code checks for currentURI.
   testTab2.linkedBrowser.addEventListener("load", function() {
-    is(Disabled(), false, "Bookmark All Tabs should be enabled since there are two tabs with different addresses");
-
     // Hide the original tab
     gBrowser.selectedTab = testTab2;
     gBrowser.showOnlyTheseTabs([testTab2]);
     is(gBrowser.visibleTabs.length, 1, "1 tab should be visible");
-    is(Disabled(), true, "Bookmark All Tabs should be disabled as there is only one visible tab");
 
     // Add a tab that will get pinned
     let pinned = BrowserTestUtils.addTab(gBrowser);
     is(gBrowser.visibleTabs.length, 2, "2 tabs should be visible now");
-    is(Disabled(), false, "Bookmark All Tabs should be available as there are two visible tabs");
     gBrowser.pinTab(pinned);
-    is(Hidden(), false, "Bookmark All Tabs should be visible on a normal tab");
-    is(Disabled(), true, "Bookmark All Tabs should not be available since one tab is pinned");
+    is(BookmarkTabHidden(), false, "Bookmark Tab should be visible on a normal tab");
     gBrowser.selectedTab = pinned;
-    is(Hidden(), true, "Bookmark All Tabs should be hidden on a pinned tab");
+    is(BookmarkTabHidden(), false, "Bookmark Tab should be visible on a pinned tab");
 
     // Show all tabs
     let allTabs = Array.from(gBrowser.tabs);
     gBrowser.showOnlyTheseTabs(allTabs);
 
     // reset the environment
     gBrowser.removeTab(testTab2);
     gBrowser.removeTab(testTab1);
     gBrowser.removeTab(pinned);
     is(gBrowser.visibleTabs.length, 1, "only orig is left and visible");
     is(gBrowser.tabs.length, 1, "sanity check that it matches");
-    is(Disabled(), true, "Bookmark All Tabs should be hidden");
     is(gBrowser.selectedTab, origTab, "got the orig tab");
     is(origTab.hidden, false, "and it's not hidden -- visible!");
     finish();
   }, {capture: true, once: true});
 }
 
-function Disabled() {
+function BookmarkTabHidden() {
   updateTabContextMenu();
-  return document.getElementById("Browser:BookmarkAllTabs").getAttribute("disabled") == "true";
+  return document.getElementById("context_bookmarkTab").hidden;
 }
-
-function Hidden() {
-  updateTabContextMenu();
-  return document.getElementById("context_bookmarkAllTabs").hidden;
-}
--- a/browser/components/customizableui/test/browser_customization_context_menus.js
+++ b/browser/components/customizableui/test/browser_customization_context_menus.js
@@ -52,17 +52,17 @@ add_task(async function tabstrip_context
   let rect = tabstrip.getBoundingClientRect();
   EventUtils.synthesizeMouse(tabstrip, rect.width - 2, 2, {type: "contextmenu", button: 2 });
   await shownPromise;
 
   let closedTabsAvailable = SessionStore.getClosedTabCount(window) == 0;
   info("Closed tabs: " + closedTabsAvailable);
   let expectedEntries = [
     ["#toolbar-context-reloadAllTabs", true],
-    ["#toolbar-context-bookmarkAllTabs", true],
+    ["#toolbar-context-bookmarkSelectedTabs", true],
     ["#toolbar-context-selectAllTabs", true],
     ["#toolbar-context-undoCloseTab", !closedTabsAvailable],
     ["---"],
   ];
   if (!isOSX) {
     expectedEntries.push(["#toggle_toolbar-menubar", true]);
   }
   expectedEntries.push(
--- a/browser/locales/en-US/chrome/browser/browser.dtd
+++ b/browser/locales/en-US/chrome/browser/browser.dtd
@@ -66,18 +66,18 @@ can reach it easily. -->
 <!ENTITY  sendPageToDevice.label             "Send Page to Device">
 <!ENTITY  sendPageToDevice.accesskey         "n">
 <!ENTITY  sendLinkToDevice.label             "Send Link to Device">
 <!ENTITY  sendLinkToDevice.accesskey         "n">
 <!ENTITY  moveToNewWindow.label              "Move to New Window">
 <!ENTITY  moveToNewWindow.accesskey          "W">
 <!ENTITY  reopenInContainer.label            "Reopen in Container">
 <!ENTITY  reopenInContainer.accesskey        "e">
-<!ENTITY  bookmarkAllTabs.label              "Bookmark All Tabs…">
-<!ENTITY  bookmarkAllTabs.accesskey          "T">
+<!ENTITY  bookmarkTab.label                  "Bookmark Tab">
+<!ENTITY  bookmarkTab.accesskey              "T">
 <!ENTITY  undoCloseTab.label                 "Undo Close Tab">
 <!ENTITY  undoCloseTab.accesskey             "U">
 <!ENTITY  closeTab.label                     "Close Tab">
 <!ENTITY  closeTab.accesskey                 "c">
 <!ENTITY  hiddenTabs.label                   "Hidden Tabs">
 
 <!ENTITY  listAllTabs.label      "List all tabs">
 
@@ -106,18 +106,18 @@ when there are no windows but Firefox is
 <!ENTITY menubarCmd.accesskey "M">
 <!ENTITY navbarCmd.label "Navigation Toolbar">
 <!ENTITY personalbarCmd.label "Bookmarks Toolbar">
 <!ENTITY personalbarCmd.accesskey "B">
 <!ENTITY bookmarksToolbarItem.label "Bookmarks Toolbar Items">
 
 <!ENTITY toolbarContextMenu.reloadAllTabs.label "Reload All Tabs">
 <!ENTITY toolbarContextMenu.reloadAllTabs.accesskey "A">
-<!ENTITY toolbarContextMenu.bookmarkAllTabs.label "Bookmark All Tabs…">
-<!ENTITY toolbarContextMenu.bookmarkAllTabs.accesskey "T">
+<!ENTITY toolbarContextMenu.bookmarkSelectedTabs.label "Bookmark Selected Tabs…">
+<!ENTITY toolbarContextMenu.bookmarkSelectedTabs.accesskey "T">
 <!ENTITY toolbarContextMenu.selectAllTabs.label "Select All Tabs">
 <!ENTITY toolbarContextMenu.selectAllTabs.accesskey "S">
 <!ENTITY toolbarContextMenu.undoCloseTab.label "Undo Close Tab">
 <!ENTITY toolbarContextMenu.undoCloseTab.accesskey "U">
 
 <!ENTITY pageSourceCmd.label "Page Source">
 <!ENTITY pageSourceCmd.accesskey "o">
 <!ENTITY pageSourceCmd.commandkey "u">