Bug 1276565 - Clamp out-of-range tab selection shortcuts and indexes instead of ignoring them. r=Gijs r=dao
authorChris Peterson <cpeterson@mozilla.com>
Thu, 02 Jun 2016 22:50:01 -0700
changeset 300427 2842eb7d2fb0de5d3edc4b18d857faf40df7db98
parent 300426 11f6b550511ffcb6d1e7f790049eca4fcd51a60a
child 300428 1b3eddbebb7ab27b07d3ff9d90eca37ccb6863a0
push id30313
push usercbook@mozilla.com
push dateMon, 06 Jun 2016 09:56:25 +0000
treeherdermozilla-central@0a3b6e2df656 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, dao
bugs1276565
milestone49.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 1276565 - Clamp out-of-range tab selection shortcuts and indexes instead of ignoring them. r=Gijs r=dao CMD + a number key is a keyboard shortcut to select that tab number. (CMD+9 will always select the right-most tab, regardless of the tab count.) Currently, if the number is greater than the tab count, then the keyboard shortcut is ignored. With this patch, the right-most tab will be selected instead. For example, if there are five tabs and the user accidentally enters CMD+6 instead of CMD+5, selecting tab #5 is probably what they wanted instead of ignoring the keyboard shortcut. Also expand the tab selection tests to cover more edge cases.
browser/base/content/tabbrowser.xml
browser/base/content/test/general/browser_selectTabAtIndex.js
browser/base/content/test/general/browser_visibleTabs.js
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -2886,21 +2886,27 @@
       <method name="selectTabAtIndex">
         <parameter name="aIndex"/>
         <parameter name="aEvent"/>
         <body>
         <![CDATA[
           let tabs = this.visibleTabs;
 
           // count backwards for aIndex < 0
-          if (aIndex < 0)
+          if (aIndex < 0) {
             aIndex += tabs.length;
-
-          if (aIndex >= 0 && aIndex < tabs.length)
-            this.selectedTab = tabs[aIndex];
+            // clamp at index 0 if still negative.
+            if (aIndex < 0)
+              aIndex = 0;
+          } else if (aIndex >= tabs.length) {
+            // clamp at right-most tab if out of range.
+            aIndex = tabs.length - 1;
+          }
+
+          this.selectedTab = tabs[aIndex];
 
           if (aEvent) {
             aEvent.preventDefault();
             aEvent.stopPropagation();
           }
         ]]>
         </body>
       </method>
--- a/browser/base/content/test/general/browser_selectTabAtIndex.js
+++ b/browser/base/content/test/general/browser_selectTabAtIndex.js
@@ -1,22 +1,81 @@
+"use strict";
+
 function test() {
-  for (let i = 0; i < 9; i++)
-    gBrowser.addTab();
+  const isLinux = navigator.platform.indexOf("Linux") == 0;
 
-  var isLinux = navigator.platform.indexOf("Linux") == 0;
-  for (let i = 9; i >= 1; i--) {
+  function assertTab(expectedTab) {
+    is(gBrowser.tabContainer.selectedIndex, expectedTab,
+       `tab index ${expectedTab} should be selected`);
+  }
+
+  function sendAccelKey(key) {
     // Make sure the keystroke goes to chrome.
     document.activeElement.blur();
+    EventUtils.synthesizeKey(key.toString(), { altKey: isLinux, accelKey: !isLinux });
+  }
 
-    EventUtils.synthesizeKey(i.toString(), { altKey: isLinux, accelKey: !isLinux });
+  function createTabs(count) {
+    for (let n = 0; n < count; n++)
+      gBrowser.addTab();
+  }
 
-    is(gBrowser.tabContainer.selectedIndex, (i == 9 ? gBrowser.tabs.length : i) - 1,
-       (isLinux ? "Alt" : "Accel") + "+" + i + " selects expected tab");
+  function testKey(key, expectedTab) {
+    sendAccelKey(key);
+    assertTab(expectedTab);
+  }
+
+  function testIndex(index, expectedTab) {
+    gBrowser.selectTabAtIndex(index);
+    assertTab(expectedTab);
   }
 
-  gBrowser.selectTabAtIndex(-3);
-  is(gBrowser.tabContainer.selectedIndex, gBrowser.tabs.length - 3,
-     "gBrowser.selectTabAtIndex(-3) selects expected tab");
+  // Create fewer tabs than our 9 number keys.
+  is(gBrowser.tabs.length, 1, "should have 1 tab");
+  createTabs(4);
+  is(gBrowser.tabs.length, 5, "should have 5 tabs");
+
+  // Test keyboard shortcuts. Order tests so that no two test cases have the
+  // same expected tab in a row. This ensures that tab selection actually
+  // changed the selected tab.
+  testKey(8, 4);
+  testKey(1, 0);
+  testKey(2, 1);
+  testKey(4, 3);
+  testKey(9, 4);
+
+  // Test index selection.
+  testIndex(0, 0);
+  testIndex(4, 4);
+  testIndex(-5, 0);
+  testIndex(5, 4);
+  testIndex(-4, 1);
+  testIndex(1, 1);
+  testIndex(-1, 4);
+  testIndex(9, 4);
 
-  for (let i = 0; i < 9; i++)
+  // Create more tabs than our 9 number keys.
+  createTabs(10);
+  is(gBrowser.tabs.length, 15, "should have 15 tabs");
+
+  // Test keyboard shortcuts.
+  testKey(2, 1);
+  testKey(1, 0);
+  testKey(4, 3);
+  testKey(8, 7);
+  testKey(9, 14);
+
+  // Test index selection.
+  testIndex(-15, 0);
+  testIndex(14, 14);
+  testIndex(-14, 1);
+  testIndex(15, 14);
+  testIndex(-1, 14);
+  testIndex(0, 0);
+  testIndex(1, 1);
+  testIndex(9, 9);
+
+  // Clean up tabs.
+  for (let n = 15; n > 1; n--)
     gBrowser.removeTab(gBrowser.selectedTab, {skipPermitUnload: true});
+  is(gBrowser.tabs.length, 1, "should have 1 tab");
 }
--- a/browser/base/content/test/general/browser_visibleTabs.js
+++ b/browser/base/content/test/general/browser_visibleTabs.js
@@ -1,12 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+"use strict";
+
 add_task(function* () {
   // There should be one tab when we start the test
   let [origTab] = gBrowser.visibleTabs;
 
   // Add a tab that will get pinned
   let pinned = gBrowser.addTab();
   gBrowser.pinTab(pinned);
 
@@ -28,26 +30,28 @@ add_task(function* () {
   gBrowser.showOnlyTheseTabs([testTab]);
 
   visible = gBrowser.visibleTabs;
   is(visible.length, 2, "2 tabs should be visible including the pinned");
   is(visible[0], pinned, "first is pinned");
   is(visible[1], testTab, "next is the test tab");
   is(gBrowser.tabs.length, 3, "3 tabs should still be open");
 
+  gBrowser.selectTabAtIndex(1);
+  is(gBrowser.selectedTab, testTab, "second tab is the test tab");
   gBrowser.selectTabAtIndex(0);
   is(gBrowser.selectedTab, pinned, "first tab is pinned");
-  gBrowser.selectTabAtIndex(1);
-  is(gBrowser.selectedTab, testTab, "second tab is the test tab");
   gBrowser.selectTabAtIndex(2);
   is(gBrowser.selectedTab, testTab, "no third tab, so no change");
   gBrowser.selectTabAtIndex(0);
   is(gBrowser.selectedTab, pinned, "switch back to the pinned");
   gBrowser.selectTabAtIndex(2);
-  is(gBrowser.selectedTab, pinned, "no third tab, so no change");
+  is(gBrowser.selectedTab, testTab, "no third tab, so select last tab");
+  gBrowser.selectTabAtIndex(-2);
+  is(gBrowser.selectedTab, pinned, "pinned tab is second from left (when orig tab is hidden)");
   gBrowser.selectTabAtIndex(-1);
   is(gBrowser.selectedTab, testTab, "last tab is the test tab");
 
   gBrowser.tabContainer.advanceSelectedTab(1, true);
   is(gBrowser.selectedTab, pinned, "wrapped around the end to pinned");
   gBrowser.tabContainer.advanceSelectedTab(1, true);
   is(gBrowser.selectedTab, testTab, "next to test tab");
   gBrowser.tabContainer.advanceSelectedTab(1, true);
@@ -86,9 +90,8 @@ add_task(function* () {
 
   // Close the last visible tab and make sure we still get a visible tab
   gBrowser.removeTab(testTab);
   is(gBrowser.visibleTabs.length, 1, "only orig is left and visible");
   is(gBrowser.tabs.length, 1, "sanity check that it matches");
   is(gBrowser.selectedTab, origTab, "got the orig tab");
   is(origTab.hidden, false, "and it's not hidden -- visible!");
 });
-