Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer. r?gijs draft multiselect_fix_shift_key
authorAbdoulaye O. Ly <ablayelyfondou@gmail.com>
Sat, 30 Jun 2018 03:11:31 +0000
branchmultiselect_fix_shift_key
changeset 814767 f7d0e3fe9e9944aff7c3520e793b9bf046f9628b
parent 814704 afdeb0288690f0acde2ba6bd008f860e1acdd026
push id115333
push userbmo:ablayelyfondou@gmail.com
push dateFri, 06 Jul 2018 04:15:32 +0000
reviewersgijs
bugs1472074
milestone63.0a1
Bug 1472074 - Fix behavior for multiselection using shift key to match other popular apps such as Chrome or Windows Explorer. r?gijs MozReview-Commit-ID: 8vWJJcGMOwA
browser/base/content/tabbrowser.js
browser/base/content/tabbrowser.xml
browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -3607,20 +3607,17 @@ window._gBrowser = {
       this.tabContainer._setPositionalAttributes();
     }
   },
 
   /**
    * Adds two given tabs and all tabs between them into the (multi) selected tabs collection
    */
   addRangeToMultiSelectedTabs(aTab1, aTab2) {
-    // Let's avoid going through all the heavy process below when the same
-    // tab is given as params.
     if (aTab1 == aTab2) {
-      this.addToMultiSelectedTabs(aTab1);
       return;
     }
 
     const tabs = this._visibleTabs;
     const indexOfTab1 = tabs.indexOf(aTab1);
     const indexOfTab2 = tabs.indexOf(aTab2);
 
     const [lowerIndex, higherIndex] = indexOfTab1 < indexOfTab2 ?
@@ -3641,21 +3638,43 @@ window._gBrowser = {
     this._multiSelectedTabsSet.delete(aTab);
   },
 
   clearMultiSelectedTabs(updatePositionalAttributes) {
     for (let tab of this.selectedTabs) {
       tab.removeAttribute("multiselected");
     }
     this._multiSelectedTabsSet = new WeakSet();
+    this._lastMultiSelectedTabRef = null;
     if (updatePositionalAttributes) {
       this.tabContainer._setPositionalAttributes();
     }
   },
 
+  /**
+   * Remove the active tab from the multiselection if it's the only one left there.
+   */
+  updateActiveTabMultiSelectState() {
+    if (this.selectedTabs.length == 1) {
+      this.clearMultiSelectedTabs();
+    }
+  },
+
+  switchToNextMultiSelectedTab() {
+    let lastMultiSelectedTab = gBrowser.lastMultiSelectedTab;
+    if (lastMultiSelectedTab != gBrowser.selectedTab) {
+      gBrowser.selectedTab = lastMultiSelectedTab;
+      return;
+    }
+    let selectedTabs = ChromeUtils.nondeterministicGetWeakSetKeys(this._multiSelectedTabsSet)
+                                  .filter(tab => tab.isConnected && !tab.closing);
+    let length = selectedTabs.length;
+    gBrowser.selectedTab = selectedTabs[length - 1];
+  },
+
   set selectedTabs(tabs) {
     this.clearMultiSelectedTabs(false);
     this.selectedTab = tabs[0];
     if (tabs.length > 1) {
       for (let tab of tabs) {
         this.addToMultiSelectedTabs(tab, true);
       }
     }
@@ -3678,17 +3697,19 @@ window._gBrowser = {
       .length;
   },
 
   get lastMultiSelectedTab() {
     let tab = this._lastMultiSelectedTabRef ? this._lastMultiSelectedTabRef.get() : null;
     if (tab && tab.isConnected && this._multiSelectedTabsSet.has(tab)) {
       return tab;
     }
-    return gBrowser.selectedTab;
+    let selectedTab = gBrowser.selectedTab;
+    this.lastMultiSelectedTab = selectedTab;
+    return selectedTab;
   },
 
   set lastMultiSelectedTab(aTab) {
     this._lastMultiSelectedTabRef = Cu.getWeakReference(aTab);
   },
 
   toggleMuteAudioOnMultiSelectedTabs(aTab) {
     let tabsToToggle;
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -2003,32 +2003,35 @@
       </handler>
       <handler event="mouseup">
         this.style.MozUserFocus = "";
       </handler>
 
       <handler event="click" button="0"><![CDATA[
         if (Services.prefs.getBoolPref("browser.tabs.multiselect")) {
           if (event.shiftKey) {
-            const lastSelectedTab = gBrowser.lastMultiSelectedTab || gBrowser.selectedTab;
+            const lastSelectedTab = gBrowser.lastMultiSelectedTab;
+            gBrowser.clearMultiSelectedTabs(true);
             gBrowser.addRangeToMultiSelectedTabs(lastSelectedTab, this);
-            gBrowser.lastMultiSelectedTab = this;
+            gBrowser.selectedTab = lastSelectedTab;
             return;
           }
           if (event.getModifierState("Accel")) {
             // Ctrl (Cmd for mac) key is pressed
             if (this.multiselected) {
               gBrowser.removeFromMultiSelectedTabs(this);
               if (this == gBrowser.selectedTab) {
-                gBrowser.selectedTab = gBrowser.lastMultiSelectedTab;
+                gBrowser.switchToNextMultiSelectedTab();
               }
+              gBrowser.updateActiveTabMultiSelectState();
             } else if (this != gBrowser.selectedTab) {
               for (let tab of [this, gBrowser.selectedTab]) {
-                gBrowser.addToMultiSelectedTabs(tab);
+                gBrowser.addToMultiSelectedTabs(tab, true);
               }
+              gBrowser.tabContainer._setPositionalAttributes();
               gBrowser.lastMultiSelectedTab = this;
             }
             return;
           }
 
           const overCloseButton = event.originalTarget.getAttribute("anonid") == "close-button";
           if (gBrowser.multiSelectedTabsCount > 0 && !overCloseButton && !this._overPlayingIcon) {
             // Tabs were previously multi-selected and user clicks on a tab
--- a/browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js
+++ b/browser/base/content/test/tabs/browser_multiselect_tabs_using_Shift.js
@@ -49,16 +49,17 @@ add_task(async function noItemsInTheColl
     is(gBrowser.selectedTab, tab1, "Tab1 has focus now");
     is(gBrowser.multiSelectedTabsCount, 0, "No tab is multi-selected");
 
     gBrowser.hideTab(tab3);
     ok(tab3.hidden, "Tab3 is hidden");
 
     info("Click on tab4 while holding shift key");
     await triggerClickOn(tab4, { shiftKey: true });
+    mSelectedTabs = gBrowser._multiSelectedTabsSet;
 
     ok(tab1.multiselected && mSelectedTabs.has(tab1), "Tab1 is multi-selected");
     ok(tab2.multiselected && mSelectedTabs.has(tab2), "Tab2 is multi-selected");
     ok(!tab3.multiselected && !mSelectedTabs.has(tab3), "Hidden tab3 is not multi-selected");
     ok(tab4.multiselected && mSelectedTabs.has(tab4), "Tab4 is multi-selected");
     ok(!tab5.multiselected && !mSelectedTabs.has(tab5), "Tab5 is not multi-selected");
     is(gBrowser.multiSelectedTabsCount, 3, "three multi-selected tabs");
     is(gBrowser.selectedTab, tab1, "Tab1 still has focus");
@@ -87,24 +88,37 @@ add_task(async function itemsInTheCollec
     await triggerClickOn(tab3, { ctrlKey: true });
     is(gBrowser.selectedTab, tab1, "Tab1 still has focus");
     is(gBrowser.multiSelectedTabsCount, 2, "Two tabs are multi-selected");
     ok(tab1.multiselected && mSelectedTabs.has(tab1), "Tab1 is multi-selected");
     ok(tab3.multiselected && mSelectedTabs.has(tab3), "Tab3 is multi-selected");
 
     info("Click on tab5 while holding Shift key");
     await triggerClickOn(tab5, { shiftKey: true });
+    mSelectedTabs = gBrowser._multiSelectedTabsSet;
 
-    is(gBrowser.selectedTab, tab1, "Tab1 still has focus");
-    ok(tab1.multiselected && mSelectedTabs.has(tab1), "Tab1 is multi-selected");
+    is(gBrowser.selectedTab, tab3, "Tab3 has focus");
+    ok(!tab1.multiselected && !mSelectedTabs.has(tab1), "Tab1 is not multi-selected");
     ok(!tab2.multiselected && !mSelectedTabs.has(tab2), "Tab2 is not multi-selected ");
     ok(tab3.multiselected && mSelectedTabs.has(tab3), "Tab3 is multi-selected");
     ok(tab4.multiselected && mSelectedTabs.has(tab4), "Tab4 is multi-selected");
     ok(tab5.multiselected && mSelectedTabs.has(tab5), "Tab5 is multi-selected");
-    is(gBrowser.multiSelectedTabsCount, 4, "Four tabs are multi-selected");
+    is(gBrowser.multiSelectedTabsCount, 3, "Three tabs are multi-selected");
+
+    info("Click on tab1 while holding Shift key");
+    await triggerClickOn(tab1, { shiftKey: true });
+    mSelectedTabs = gBrowser._multiSelectedTabsSet;
+
+    is(gBrowser.selectedTab, tab3, "Tab3 has focus");
+    ok(tab1.multiselected && mSelectedTabs.has(tab1), "Tab1 is multi-selected");
+    ok(tab2.multiselected && mSelectedTabs.has(tab2), "Tab2 is multi-selected ");
+    ok(tab3.multiselected && mSelectedTabs.has(tab3), "Tab3 is multi-selected");
+    ok(!tab4.multiselected && !mSelectedTabs.has(tab4), "Tab4 is not multi-selected");
+    ok(!tab5.multiselected && !mSelectedTabs.has(tab5), "Tab5 is not multi-selected");
+    is(gBrowser.multiSelectedTabsCount, 3, "Three tabs are multi-selected");
 
     BrowserTestUtils.removeTab(tab1);
     BrowserTestUtils.removeTab(tab2);
     BrowserTestUtils.removeTab(tab3);
     BrowserTestUtils.removeTab(tab4);
     BrowserTestUtils.removeTab(tab5);
 });