From b52f88014f69b30d9bd85760382e1ba0e2c84e9e Mon Sep 17 00:00:00 2001 From: Chris Peterson Date: Thu, 2 Jun 2016 22:50:01 -0700 Subject: [PATCH] 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 | 12 ++- .../test/general/browser_selectTabAtIndex.js | 81 ++++++++++++++++--- .../test/general/browser_visibleTabs.js | 11 ++- 3 files changed, 86 insertions(+), 18 deletions(-) diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml index 8b019a45a4530..44ce75016e0fa 100644 --- a/browser/base/content/tabbrowser.xml +++ b/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(); diff --git a/browser/base/content/test/general/browser_selectTabAtIndex.js b/browser/base/content/test/general/browser_selectTabAtIndex.js index a1306afbc8384..b6578aec0ae5f 100644 --- 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; + + 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"); } diff --git a/browser/base/content/test/general/browser_visibleTabs.js b/browser/base/content/test/general/browser_visibleTabs.js index 1c9f497819832..e9130bc18f7ca 100644 --- a/browser/base/content/test/general/browser_visibleTabs.js +++ b/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!"); }); -