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

--HG--
extra : moz-landing-system : lando
This commit is contained in:
Jared Wein 2018-10-02 16:24:43 +00:00
Родитель ea6021b769
Коммит 435e84add2
9 изменённых файлов: 42 добавлений и 69 удалений

Просмотреть файл

@ -490,27 +490,21 @@ var PlacesCommandHook = {
* Adds a folder with bookmarks to URIList given in param.
*/
bookmarkPages(URIList) {
if (URIList.length > 1) {
PlacesUIUtils.showBookmarkDialog({ action: "add",
type: "folder",
URIList,
}, window);
}
},
/**
* 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)
if (!URIList.length) {
return;
}
// Disable "Bookmark All Tabs" if there are less than two
// "unique current pages".
goSetCommandEnabled("Browser:BookmarkAllTabs",
this.uniqueCurrentPages.length >= 2);
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;
}
PlacesUIUtils.showBookmarkDialog(bookmarkDialogInfo, window);
},
/**
@ -1569,7 +1563,6 @@ var BookmarkingUI = {
return;
this.updateBookmarkPageMenuItem();
PlacesCommandHook.updateBookmarkAllTabsCommand();
this._initMobileBookmarks(document.getElementById("menu_mobileBookmarks"));
},

Просмотреть файл

@ -55,8 +55,6 @@
<!-- 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"/>

Просмотреть файл

@ -5706,8 +5706,6 @@ function onViewToolbarsPopupShowing(aEvent, aInsertPoint) {
}
if (showTabStripItems) {
PlacesCommandHook.updateBookmarkAllTabsCommand();
let haveMultipleTabs = gBrowser.visibleTabs.length > 1;
document.getElementById("toolbar-context-reloadAllTabs").disabled = !haveMultipleTabs;

Просмотреть файл

@ -117,6 +117,15 @@ xmlns="http://www.w3.org/1999/xhtml"
<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);"/>
@ -153,15 +162,6 @@ xmlns="http://www.w3.org/1999/xhtml"
<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;"
@ -417,12 +417,12 @@ xmlns="http://www.w3.org/1999/xhtml"
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"

Просмотреть файл

@ -5343,13 +5343,10 @@ var TabContextMenu = {
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");

Просмотреть файл

@ -23,7 +23,7 @@ add_task(async function test() {
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");
@ -37,12 +37,12 @@ add_task(async function test() {
// 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");
@ -55,7 +55,7 @@ add_task(async function test() {
// 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");

Просмотреть файл

@ -8,34 +8,27 @@ function test() {
// 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);
@ -47,19 +40,13 @@ function test() {
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";
}
function Hidden() {
updateTabContextMenu();
return document.getElementById("context_bookmarkAllTabs").hidden;
return document.getElementById("context_bookmarkTab").hidden;
}

Просмотреть файл

@ -57,7 +57,7 @@ add_task(async function tabstrip_context() {
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],
["---"],

Просмотреть файл

@ -71,8 +71,8 @@ can reach it easily. -->
<!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">
@ -111,8 +111,8 @@ when there are no windows but Firefox is still running. -->
<!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">