From 12b833710e11d3ff512eeed08e1cb562f08949f6 Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Fri, 28 Jan 2011 17:46:49 +0100 Subject: [PATCH] Bug 588011 - "Bookmark All Tabs" should ignore App Tabs. Includes fix for Bug 607227 - Remove "Bookmark all Tabs" from the bookmarks menu. r=dao ui-r=faaborg a=blocker --- browser/base/content/browser-menubar.inc | 8 +- browser/base/content/browser-places.js | 55 +++++++------- browser/base/content/browser-sets.inc | 9 +-- browser/base/content/browser.css | 5 ++ browser/base/content/browser.js | 44 ++--------- .../browser_visibleTabs_bookmarkAllPages.js | 2 +- .../browser_visibleTabs_bookmarkAllTabs.js | 76 ++++++++++++------- 7 files changed, 102 insertions(+), 97 deletions(-) diff --git a/browser/base/content/browser-menubar.inc b/browser/base/content/browser-menubar.inc index 8bf93fa99e92..4a204262926b 100644 --- a/browser/base/content/browser-menubar.inc +++ b/browser/base/content/browser-menubar.inc @@ -38,6 +38,10 @@ # ***** END LICENSE BLOCK ***** @@ -431,7 +435,8 @@ openInTabs="children" oncommand="BookmarksEventHandler.onCommand(event);" onclick="BookmarksEventHandler.onClick(event);" - onpopupshowing="if (!this.parentNode._placesView) + onpopupshowing="PlacesCommandHook.updateBookmarkAllTabsCommand(); + if (!this.parentNode._placesView) new PlacesMenu(event, 'place:folder=BOOKMARKS_MENU');" tooltip="bhTooltip" popupsinherittooltip="true"> 1) { + PlacesUIUtils.showMinimalAddMultiBookmarkUI(pages); + } + }, + + /** + * Updates disabled state for the "Bookmark All Tabs" command. + */ + updateBookmarkAllTabsCommand: + function PCH_updateBookmarkAllTabsCommand() { + // Disable "Bookmark All Tabs" if there are less than two + // "unique current pages". + goSetCommandEnabled("Browser:BookmarkAllTabs", + this.uniqueCurrentPages.length >= 2); }, - /** * Adds a Live Bookmark to a feed associated with the current page. * @param url diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc index 53f4d1ebb5d5..d02b194908c6 100644 --- a/browser/base/content/browser-sets.inc +++ b/browser/base/content/browser-sets.inc @@ -91,11 +91,10 @@ - + + oncommand="PlacesCommandHook.bookmarkCurrentPages();"/> @@ -304,7 +303,7 @@ # Accel+Shift+A-F are reserved on GTK2 #ifndef MOZ_WIDGET_GTK2 - + #else diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css index 6b487a8f0e1a..de8e3e5dae03 100644 --- a/browser/base/content/browser.css +++ b/browser/base/content/browser.css @@ -192,6 +192,11 @@ splitmenu { } %endif +/* Hide menu elements intended for keyboard access support */ +#main-menubar[openedwithkey=false] .show-only-for-keyboard { + display: none; +} + /* ::::: location bar ::::: */ #urlbar { -moz-binding: url(chrome://browser/content/urlbarBindings.xml#urlbar); diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js index 396b6df78622..fd3fb3ade08e 100644 --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -1550,9 +1550,6 @@ function delayedStartup(isLoadingBlank, mustLoadSidebar) { PlacesToolbarHelper.init(); - // bookmark-all-tabs command - gBookmarkAllTabsHandler.init(); - ctrlTab.readPref(); gPrefService.addObserver(ctrlTab.prefName, ctrlTab, false); gPrefService.addObserver(allTabs.prefName, allTabs, false); @@ -7159,41 +7156,6 @@ function formatURL(aFormat, aIsPref) { return aIsPref ? formatter.formatURLPref(aFormat) : formatter.formatURL(aFormat); } -/** - * This also takes care of updating the command enabled-state when tabs are - * created or removed. - */ -var gBookmarkAllTabsHandler = { - init: function () { - this._command = document.getElementById("Browser:BookmarkAllTabs"); - gBrowser.tabContainer.addEventListener("TabOpen", this, true); - gBrowser.tabContainer.addEventListener("TabClose", this, true); - gBrowser.tabContainer.addEventListener("TabShow", this, true); - gBrowser.tabContainer.addEventListener("TabHide", this, true); - this._updateCommandState(); - }, - - _updateCommandState: function BATH__updateCommandState() { - let remainingTabs = gBrowser.visibleTabs.filter(function(tab) { - return gBrowser._removingTabs.indexOf(tab) == -1; - }); - - if (remainingTabs.length > 1) - this._command.removeAttribute("disabled"); - else - this._command.setAttribute("disabled", "true"); - }, - - doCommand: function BATH_doCommand() { - PlacesCommandHook.bookmarkCurrentPages(); - }, - - // nsIDOMEventListener - handleEvent: function(aEvent) { - this._updateCommandState(); - } -}; - /** * Utility object to handle manipulations of the identity indicators in the UI */ @@ -8303,6 +8265,12 @@ var TabContextMenu = { document.getElementById("context_closeOtherTabs").disabled = unpinnedTabs <= 1; document.getElementById("context_closeOtherTabs").hidden = this.contextTab.pinned; + // Hide "Bookmark All Tabs" for a pinned tab. Update its state if visible. + let bookmarkAllTabs = document.getElementById("context_bookmarkAllTabs"); + bookmarkAllTabs.hidden = this.contextTab.pinned; + if (!bookmarkAllTabs.hidden) + PlacesCommandHook.updateBookmarkAllTabsCommand(); + // Hide "Move to Group" if it's a pinned tab. document.getElementById("context_tabViewMenu").hidden = this.contextTab.pinned; } diff --git a/browser/base/content/test/browser_visibleTabs_bookmarkAllPages.js b/browser/base/content/test/browser_visibleTabs_bookmarkAllPages.js index 8c204c594b12..755baa6f41ce 100644 --- a/browser/base/content/test/browser_visibleTabs_bookmarkAllPages.js +++ b/browser/base/content/test/browser_visibleTabs_bookmarkAllPages.js @@ -50,7 +50,7 @@ function test() { is(gBrowser.visibleTabs.length, 1, "Only one tab is visible"); - let uris = PlacesCommandHook._getUniqueTabInfo(); + let uris = PlacesCommandHook.uniqueCurrentPages; is(uris.length, 1, "Only one uri is returned"); is(uris[0].spec, tabTwo.linkedBrowser.currentURI.spec, "It's the correct URI"); diff --git a/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js b/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js index 12a550ed59c3..a7b7c817cd65 100644 --- a/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js +++ b/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js @@ -36,43 +36,67 @@ * ***** END LICENSE BLOCK ***** */ 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 hidden"); + is(Disabled(), true, "Bookmark All Tabs should be disabled"); // Add a tab - let testTab = gBrowser.addTab(); + let testTab1 = gBrowser.addTab(); is(gBrowser.visibleTabs.length, 2, "2 tabs should be open"); - is(Disabled(), false, "Bookmark All Tabs should be available"); + is(Disabled(), true, "Bookmark All Tabs should be disabled since there are two tabs with the same address"); - // Hide the original tab - gBrowser.selectedTab = testTab; - gBrowser.showOnlyTheseTabs([testTab]); - is(gBrowser.visibleTabs.length, 1, "1 tab should be visible"); - is(Disabled(), true, "Bookmark All Tabs should be hidden as there is only one visible tab"); - - // Add a tab that will get pinned - let pinned = gBrowser.addTab(); - gBrowser.pinTab(pinned); - 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"); + let testTab2 = gBrowser.addTab("about:robots"); + is(gBrowser.visibleTabs.length, 3, "3 tabs should be open"); + // Wait for tab load, the code checks for currentURI. + testTab2.linkedBrowser.addEventListener("load", function () { + testTab2.linkedBrowser.removeEventListener("load", arguments.callee, true); + is(Disabled(), false, "Bookmark All Tabs should be enabled since there are two tabs with different addresses"); - // Show all tabs - let allTabs = [tab for each (tab in gBrowser.tabs)]; - gBrowser.showOnlyTheseTabs(allTabs); + // 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"); - // reset the environment - gBrowser.removeTab(testTab); - 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!"); + // Add a tab that will get pinned + let pinned = gBrowser.addTab(); + 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"); + gBrowser.selectedTab = pinned; + is(Hidden(), true, "Bookmark All Tabs should be hidden on a pinned tab"); + + // Show all tabs + let allTabs = [tab for each (tab in 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(); + }, true); } function Disabled() { + document.popupNode = gBrowser.selectedTab; + TabContextMenu.updateContextMenu(document.getElementById("tabContextMenu")); let command = document.getElementById("Browser:BookmarkAllTabs"); return command.hasAttribute("disabled") && command.getAttribute("disabled") === "true"; -} \ No newline at end of file +} + +function Hidden() { + document.popupNode = gBrowser.selectedTab; + TabContextMenu.updateContextMenu(document.getElementById("tabContextMenu")); + return document.getElementById("context_bookmarkAllTabs").hidden; +}