Bug 1420601 - Let Accel+W in a pinned tab select the first unpinned tab. r=daleharvey
authorDão Gottwald <dao@mozilla.com>
Wed, 29 Nov 2017 16:17:37 +0100
changeset 394696 3575e010c493166a6c65d5ce470a98f65c49cd31
parent 394695 f909f09dd34630efb4a7ddbc15a19dbd955cdc9d
child 394697 37b4ae2a4693c7363115e60c61d70d475261c651
push id97969
push userbtara@mozilla.com
push dateMon, 04 Dec 2017 20:48:53 +0000
treeherdermozilla-inbound@b580f65bdb21 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdaleharvey
bugs1420601
milestone59.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 1420601 - Let Accel+W in a pinned tab select the first unpinned tab. r=daleharvey MozReview-Commit-ID: DNhOuW4BL3P
browser/base/content/browser-sets.inc
browser/base/content/browser.js
browser/base/content/tabbrowser.xml
browser/base/content/test/general/browser.ini
browser/base/content/test/general/browser_bug580638.js
browser/base/content/test/general/browser_pinnedTabs.js
browser/base/content/test/tabs/browser.ini
browser/base/content/test/tabs/browser_pinnedTabs.js
browser/base/content/test/tabs/browser_pinnedTabs_closeByKeyboard.js
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -26,17 +26,17 @@
     <command id="Browser:SavePage" oncommand="saveBrowser(gBrowser.selectedBrowser);"/>
 
     <command id="Browser:SendLink"
              oncommand="MailIntegration.sendLinkForBrowser(gBrowser.selectedBrowser);"/>
 
     <command id="cmd_pageSetup" oncommand="PrintUtils.showPageSetup();"/>
     <command id="cmd_print" oncommand="PrintUtils.printWindow(window.gBrowser.selectedBrowser.outerWindowID, window.gBrowser.selectedBrowser);"/>
     <command id="cmd_printPreview" oncommand="PrintUtils.printPreview(PrintPreviewListener);"/>
-    <command id="cmd_close" oncommand="BrowserCloseTabOrWindow()"/>
+    <command id="cmd_close" oncommand="BrowserCloseTabOrWindow(event);"/>
     <command id="cmd_closeWindow" oncommand="BrowserTryToCloseWindow()"/>
     <command id="cmd_toggleMute" oncommand="gBrowser.selectedTab.toggleMuteAudio()"/>
     <command id="cmd_CustomizeToolbars" oncommand="gCustomizeMode.enter()"/>
     <command id="cmd_quitApplication" oncommand="goQuitApplication()"/>
 
 
     <commandset id="editMenuCommands"/>
 
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -2367,23 +2367,34 @@ function BrowserOpenFileWindow() {
                      nsIFilePicker.filterImages | nsIFilePicker.filterXML |
                      nsIFilePicker.filterHTML);
     fp.displayDirectory = gLastOpenDirectory.path;
     fp.open(fpCallback);
   } catch (ex) {
   }
 }
 
-function BrowserCloseTabOrWindow() {
-  // If we're not a browser window, just close the window
+function BrowserCloseTabOrWindow(event) {
+  // If we're not a browser window, just close the window.
   if (window.location.href != getBrowserURL()) {
     closeWindow(true);
     return;
   }
 
+  // Keyboard shortcuts that would close a tab that is pinned select the first
+  // unpinned tab instead.
+  if (event &&
+      (event.ctrlKey || event.metaKey || event.altKey) &&
+      gBrowser.selectedTab.pinned) {
+    if (gBrowser.visibleTabs.length > gBrowser._numPinnedTabs) {
+      gBrowser.tabContainer.selectedIndex = gBrowser._numPinnedTabs;
+    }
+    return;
+  }
+
   // If the current tab is the last one, this will close the window.
   gBrowser.removeCurrentTab({animate: true});
 }
 
 function BrowserTryToCloseWindow() {
   if (WindowIsClosing())
     window.close(); // WindowIsClosing does all the necessary checks
 }
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -206,52 +206,16 @@
           if (this._statusPanel) {
             let browser = this.selectedBrowser;
             let browserContainer = this.getBrowserContainer(browser);
             browserContainer.insertBefore(this._statusPanel, browser.parentNode.nextSibling);
           }
         ]]></body>
       </method>
 
-      <method name="_setCloseKeyState">
-        <parameter name="aEnabled"/>
-        <body><![CDATA[
-          let keyClose = document.getElementById("key_close");
-          let closeKeyEnabled = keyClose.getAttribute("disabled") != "true";
-          if (closeKeyEnabled == aEnabled)
-            return;
-
-          if (aEnabled)
-            keyClose.removeAttribute("disabled");
-          else
-            keyClose.setAttribute("disabled", "true");
-
-          // We also want to remove the keyboard shortcut from the file menu
-          // when the shortcut is disabled, and bring it back when it's
-          // renabled.
-          //
-          // Fixing bug 630826 could make that happen automatically.
-          // Fixing bug 630830 could avoid the ugly hack below.
-
-          let closeMenuItem = document.getElementById("menu_close");
-          let parentPopup = closeMenuItem.parentNode;
-          let nextItem = closeMenuItem.nextSibling;
-          let clonedItem = closeMenuItem.cloneNode(true);
-
-          parentPopup.removeChild(closeMenuItem);
-
-          if (aEnabled)
-            clonedItem.setAttribute("key", "key_close");
-          else
-            clonedItem.removeAttribute("key");
-
-          parentPopup.insertBefore(clonedItem, nextItem);
-        ]]></body>
-      </method>
-
       <method name="pinTab">
         <parameter name="aTab"/>
         <body><![CDATA[
           if (aTab.pinned)
             return;
 
           if (aTab.hidden)
             this.showTab(aTab);
@@ -259,19 +223,16 @@
           this.moveTabTo(aTab, this._numPinnedTabs);
           aTab.setAttribute("pinned", "true");
           this.tabContainer._unlockTabSizing();
           this.tabContainer._positionPinnedTabs();
           this.tabContainer._updateCloseButtons();
 
           this.getBrowserForTab(aTab).messageManager.sendAsyncMessage("Browser:AppTab", { isAppTab: true });
 
-          if (aTab.selected)
-            this._setCloseKeyState(false);
-
           let event = document.createEvent("Events");
           event.initEvent("TabPinned", true, false);
           aTab.dispatchEvent(event);
         ]]></body>
       </method>
 
       <method name="unpinTab">
         <parameter name="aTab"/>
@@ -283,19 +244,16 @@
           aTab.removeAttribute("pinned");
           aTab.style.marginInlineStart = "";
           this.tabContainer._unlockTabSizing();
           this.tabContainer._positionPinnedTabs();
           this.tabContainer._updateCloseButtons();
 
           this.getBrowserForTab(aTab).messageManager.sendAsyncMessage("Browser:AppTab", { isAppTab: false });
 
-          if (aTab.selected)
-            this._setCloseKeyState(true);
-
           let event = document.createEvent("Events");
           event.initEvent("TabUnpinned", true, false);
           aTab.dispatchEvent(event);
         ]]></body>
       </method>
 
       <method name="previewTab">
         <parameter name="aTab"/>
@@ -1366,18 +1324,16 @@
               this.mIsBusy = false;
               this._callProgressListeners(null, "onStateChange",
                                           [webProgress, null,
                                            nsIWebProgressListener.STATE_STOP |
                                            nsIWebProgressListener.STATE_IS_NETWORK, 0],
                                           true, false);
             }
 
-            this._setCloseKeyState(!this.mCurrentTab.pinned);
-
             // TabSelect events are suppressed during preview mode to avoid confusing extensions and other bits of code
             // that might rely upon the other changes suppressed.
             // Focus is suppressed in the event that the main browser window is minimized - focusing a tab would restore the window
             if (!this._previewMode) {
               // We've selected the new tab, so go ahead and notify listeners.
               let event = new CustomEvent("TabSelect", {
                 bubbles: true,
                 cancelable: false,
--- a/browser/base/content/test/general/browser.ini
+++ b/browser/base/content/test/general/browser.ini
@@ -211,18 +211,16 @@ skip-if = toolkit != "cocoa" # Because o
 [browser_bug575830.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug577121.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug578534.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug579872.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
-[browser_bug580638.js]
-# DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug580956.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug581242.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug581253.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug585785.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
@@ -377,18 +375,16 @@ support-files = feed_discovery.html
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_gZipOfflineChild.js]
 support-files = test_offline_gzip.html gZipOfflineChild.cacheManifest gZipOfflineChild.cacheManifest^headers^ gZipOfflineChild.html gZipOfflineChild.html^headers^
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_page_style_menu.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_page_style_menu_update.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
-[browser_pinnedTabs.js]
-# DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_plainTextLinks.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_printpreview.js]
 skip-if = os == 'win' # Bug 1384127
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_private_browsing_window.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_private_no_prompt.js]
--- a/browser/base/content/test/tabs/browser.ini
+++ b/browser/base/content/test/tabs/browser.ini
@@ -15,14 +15,16 @@ skip-if = !e10s # Tab spinner is e10s on
 skip-if = os == 'mac'
 [browser_navigatePinnedTab.js]
 [browser_new_file_whitelisted_http_tab.js]
 skip-if = !e10s # Test only relevant for e10s.
 [browser_new_web_tab_in_file_process_pref.js]
 skip-if = !e10s # Pref and test only relevant for e10s.
 [browser_opened_file_tab_navigated_to_web.js]
 [browser_overflowScroll.js]
+[browser_pinnedTabs.js]
+[browser_pinnedTabs_closeByKeyboard.js]
 [browser_positional_attributes.js]
 [browser_preloadedBrowser_zoom.js]
 [browser_reload_deleted_file.js]
 [browser_tabswitch_updatecommands.js]
 [browser_viewsource_of_data_URI_in_file_process.js]
 [browser_open_newtab_start_observer_notification.js]
rename from browser/base/content/test/general/browser_pinnedTabs.js
rename to browser/base/content/test/tabs/browser_pinnedTabs.js
rename from browser/base/content/test/general/browser_bug580638.js
rename to browser/base/content/test/tabs/browser_pinnedTabs_closeByKeyboard.js
--- a/browser/base/content/test/general/browser_bug580638.js
+++ b/browser/base/content/test/tabs/browser_pinnedTabs_closeByKeyboard.js
@@ -5,55 +5,58 @@
 function test() {
   waitForExplicitFinish();
 
   function testState(aPinned) {
     function elemAttr(id, attr) {
       return document.getElementById(id).getAttribute(attr);
     }
 
-    if (aPinned) {
-      is(elemAttr("key_close", "disabled"), "true",
-         "key_close should be disabled when a pinned-tab is selected");
-      is(elemAttr("menu_close", "key"), "",
-         "menu_close shouldn't have a key set when a pinned is selected");
-    } else {
-      is(elemAttr("key_close", "disabled"), "",
-         "key_closed shouldn't have disabled state set when a non-pinned tab is selected");
-      is(elemAttr("menu_close", "key"), "key_close",
-         "menu_close should have key_close set as its key when a non-pinned tab is selected");
-    }
+    is(elemAttr("key_close", "disabled"), "",
+       "key_closed should always be enabled");
+    is(elemAttr("menu_close", "key"), "key_close",
+       "menu_close should always have key_close set");
   }
 
-  let lastSelectedTab = gBrowser.selectedTab;
-  ok(!lastSelectedTab.pinned, "We should have started with a regular tab selected");
+  let unpinnedTab = gBrowser.selectedTab;
+  ok(!unpinnedTab.pinned, "We should have started with a regular tab selected");
 
   testState(false);
 
-  let pinnedTab = BrowserTestUtils.addTab(gBrowser, "about:blank");
+  let pinnedTab = gBrowser.addTab();
   gBrowser.pinTab(pinnedTab);
 
   // Just pinning the tab shouldn't change the key state.
   testState(false);
 
-  // Test updating key state after selecting a tab.
+  // Test key state after selecting a tab.
   gBrowser.selectedTab = pinnedTab;
   testState(true);
 
-  gBrowser.selectedTab = lastSelectedTab;
+  gBrowser.selectedTab = unpinnedTab;
   testState(false);
 
   gBrowser.selectedTab = pinnedTab;
   testState(true);
 
-  // Test updating the key state after un/pinning the tab.
+  // Test the key state after un/pinning the tab.
   gBrowser.unpinTab(pinnedTab);
   testState(false);
 
   gBrowser.pinTab(pinnedTab);
   testState(true);
 
-  // Test updating the key state after removing the tab.
+  // Test that accel+w in a pinned tab selects the next tab.
+  let pinnedTab2 = gBrowser.addTab();
+  gBrowser.pinTab(pinnedTab2);
+  gBrowser.selectedTab = pinnedTab;
+
+  EventUtils.synthesizeKey("w", { accelKey: true });
+  is(gBrowser.tabs.length, 3, "accel+w in a pinned tab didn't close it");
+  is(gBrowser.selectedTab, unpinnedTab, "accel+w in a pinned tab selected the first unpinned tab");
+
+  // Test the key state after removing the tab.
   gBrowser.removeTab(pinnedTab);
+  gBrowser.removeTab(pinnedTab2);
   testState(false);
 
   finish();
 }