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

@@ -2891,11 +2891,17 @@
           let tabs = this.visibleTabs;
 
           // count backwards for aIndex < 0
-          if (aIndex < 0)
+          if (aIndex < 0) {
             aIndex += tabs.length;
+            // 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;
+          }
 
-          if (aIndex >= 0 && aIndex < tabs.length)
-            this.selectedTab = tabs[aIndex];
+          this.selectedTab = tabs[aIndex];
 
           if (aEvent) {
             aEvent.preventDefault();

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;
+
+  function assertTab(expectedTab) {
+    is(gBrowser.tabContainer.selectedIndex, expectedTab,
+       `tab index ${expectedTab} should be selected`);
+  }
 
-  var isLinux = navigator.platform.indexOf("Linux") == 0;
-  for (let i = 9; i >= 1; i--) {
+  function sendAccelKey(key) {
     // Make sure the keystroke goes to chrome.
     document.activeElement.blur();
+    EventUtils.synthesizeKey(key.toString(), { altKey: isLinux, accelKey: !isLinux });
+  }
+
+  function createTabs(count) {
+    for (let n = 0; n < count; n++)
+      gBrowser.addTab();
+  }
 
-    EventUtils.synthesizeKey(i.toString(), { altKey: isLinux, accelKey: !isLinux });
+  function testKey(key, expectedTab) {
+    sendAccelKey(key);
+    assertTab(expectedTab);
+  }
 
-    is(gBrowser.tabContainer.selectedIndex, (i == 9 ? gBrowser.tabs.length : i) - 1,
-       (isLinux ? "Alt" : "Accel") + "+" + i + " selects expected tab");
+  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);
+
+  // 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);
 
-  for (let i = 0; i < 9; i++)
+  // 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");
 }

browser/base/content/test/general/browser_visibleTabs.js

@@ -2,6 +2,8 @@
  * 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;
@@ -33,16 +35,18 @@ add_task(function* () {
   is(visible[1], testTab, "next is the test tab");
   is(gBrowser.tabs.length, 3, "3 tabs should still be open");
 
-  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(0);
+  is(gBrowser.selectedTab, pinned, "first tab is pinned");
   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");
 
@@ -91,4 +95,3 @@ add_task(function* () {
   is(gBrowser.selectedTab, origTab, "got the orig tab");
   is(origTab.hidden, false, "and it's not hidden -- visible!");
 });
-