Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab. r?jaws draft mutliselect_close_tabs_toTheRight
authorAbdoulaye O. Ly <ablayelyfondou@gmail.com>
Mon, 30 Jul 2018 15:04:32 +0000
branchmutliselect_close_tabs_toTheRight
changeset 826189 7e0b04ed8f49e57171639df4a5ae5f9374b16c58
parent 826132 b1c100e2b29a8e671678eb537ba69badc249c2fe
push id118259
push userbmo:ablayelyfondou@gmail.com
push dateFri, 03 Aug 2018 05:03:19 +0000
reviewersjaws
bugs1477780
milestone63.0a1
Bug 1477780 - In a multi-select context, 'Close Tabs To The Right' closes tabs located to right of the rightmost selected tab. r?jaws MozReview-Commit-ID: 6Kwnv8fvFLa
browser/base/content/tabbrowser.js
browser/base/content/tabbrowser.xml
browser/base/content/test/tabs/browser.ini
browser/base/content/test/tabs/browser_multiselect_tabs_close_other_tabs.js
browser/base/content/test/tabs/browser_multiselect_tabs_close_tabs_to_the_right.js
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -2527,27 +2527,41 @@ window._gBrowser = {
     // don't set the pref unless they press OK and it's false
     if (aCloseTabs == this.closingTabsEnum.ALL && reallyClose && !warnOnClose.value)
       Services.prefs.setBoolPref(pref, false);
 
     return reallyClose;
   },
 
   getTabsToTheEndFrom(aTab) {
+    let tab;
+    if (aTab.multiselected) {
+      // In a multi-select context, pick the rightmost
+      // selected tab as reference.
+      let selectedTabs = this.selectedTabs;
+      tab = selectedTabs[selectedTabs.length - 1];
+    } else {
+      tab = aTab;
+    }
+
     let tabsToEnd = [];
     let tabs = this.visibleTabs;
     for (let i = tabs.length - 1; i >= 0; --i) {
-      if (tabs[i] == aTab || tabs[i].pinned) {
+      if (tabs[i] == tab || tabs[i].pinned) {
         break;
       }
       tabsToEnd.push(tabs[i]);
     }
     return tabsToEnd;
   },
 
+  /**
+   * In a multi-select context, the tabs (except pinned tabs) that are located to the
+   * right of the rightmost selected tab will be removed.
+   */
   removeTabsToTheEndFrom(aTab) {
     let tabs = this.getTabsToTheEndFrom(aTab);
     if (!this.warnAboutClosingTabs(tabs.length, this.closingTabsEnum.TO_END)) {
       return;
     }
 
     this.removeTabs(tabs);
   },
@@ -2557,17 +2571,16 @@ window._gBrowser = {
    * Otherwise all unpinned tabs except aTab are removed.
    */
   removeAllTabsBut(aTab) {
     let tabsToRemove = [];
     if (aTab && aTab.multiselected) {
       tabsToRemove = this.visibleTabs.filter(tab => !tab.multiselected && !tab.pinned);
     } else {
       tabsToRemove = this.visibleTabs.filter(tab => tab != aTab && !tab.pinned);
-      this.selectedTab = aTab;
     }
 
     if (!this.warnAboutClosingTabs(tabsToRemove.length, this.closingTabsEnum.OTHER)) {
       return;
     }
 
     this.removeTabs(tabsToRemove);
   },
@@ -2577,37 +2590,46 @@ window._gBrowser = {
     if (!this.warnAboutClosingTabs(selectedTabs.length, this.closingTabsEnum.MULTI_SELECTED)) {
       return;
     }
 
     this.removeTabs(selectedTabs);
   },
 
   removeTabs(tabs) {
-    let tabsWithBeforeUnload = [];
-    let lastToClose;
-    let params = { animate: true };
-    for (let tab of tabs) {
-      if (tab.selected) {
-        lastToClose = tab;
-      } else if (this._hasBeforeUnload(tab)) {
-        tabsWithBeforeUnload.push(tab);
-      } else {
-        this.removeTab(tab, params);
+    this._clearMultiSelectionLocked = true;
+
+    // Guarantee that _clearMultiSelectionLocked lock gets released.
+    try {
+      let tabsWithBeforeUnload = [];
+      let lastToClose;
+      let aParams = { animate: true };
+      for (let tab of tabs) {
+        if (tab.selected)
+          lastToClose = tab;
+        else if (this._hasBeforeUnload(tab))
+          tabsWithBeforeUnload.push(tab);
+        else
+          this.removeTab(tab, aParams);
       }
-    }
-    for (let tab of tabsWithBeforeUnload) {
-      this.removeTab(tab, params);
-    }
-
-    // Avoid changing the selected browser several times by removing it,
-    // if appropriate, lastly.
-    if (lastToClose) {
-      this.removeTab(lastToClose, params);
-    }
+      for (let tab of tabsWithBeforeUnload) {
+        this.removeTab(tab, aParams);
+      }
+
+      // Avoid changing the selected browser several times by removing it,
+      // if appropriate, lastly.
+      if (lastToClose) {
+        this.removeTab(lastToClose, aParams);
+      }
+    } catch (e) {
+      Cu.reportError(e);
+    }
+
+    this._clearMultiSelectionLocked = false;
+    this.avoidSingleSelectedTab();
   },
 
   removeCurrentTab(aParams) {
     this.removeTab(this.selectedTab, aParams);
   },
 
   removeTab(aTab, {
     animate,
@@ -3688,60 +3710,85 @@ window._gBrowser = {
     if (this._clearMultiSelectionLocked) {
       if (this._clearMultiSelectionLockedOnce) {
         this._clearMultiSelectionLockedOnce = false;
         this._clearMultiSelectionLocked = false;
       }
       return;
     }
 
-    let selectedTabs = this.selectedTabs;
-    if (selectedTabs.length < 2) {
+    if (this.multiSelectedTabsCount < 1) {
       return;
     }
 
-    for (let tab of selectedTabs) {
+    for (let tab of this.selectedTabs) {
       tab.removeAttribute("multiselected");
     }
     this._multiSelectedTabsSet = new WeakSet();
     this._lastMultiSelectedTabRef = null;
     if (updatePositionalAttributes) {
       this.tabContainer._setPositionalAttributes();
     }
   },
 
   lockClearMultiSelectionOnce() {
     this._clearMultiSelectionLockedOnce = true;
     this._clearMultiSelectionLocked = true;
   },
 
   /**
-   * Remove the active tab from the multiselection if it's the only one left there.
+   * Remove a tab from the multiselection if it's the only one left there.
+   *
+   * In fact, some scenario may lead to only one single tab multi-selected,
+   * this is something to avoid (Chrome does the same)
+   * Consider 4 tabs A,B,C,D with A having the focus
+   * 1. select C with Ctrl
+   * 2. Right-click on B and "Close Tabs to The Right"
+   *
+   * Expected result
+   * C and D closing
+   * A being the only multi-selected tab, selection should be cleared
+   *
+   *
+   * Single selected tab could even happen with a none-focused tab.
+   * For exemple with the menu "Close other tabs", it could happen
+   * with a multi-selected pinned tab.
+   * For illustration, consider 4 tabs A,B,C,D with B active
+   * 1. pin A and Ctrl-select it
+   * 2. Ctrl-select C
+   * 3. right-click on D and click "Close Other Tabs"
+   *
+   * Expected result
+   * B and C closing
+   * A[pinned] being the only multi-selected tab, selection should be cleared.
    */
-  updateActiveTabMultiSelectState() {
-    if (this.selectedTabs.length == 1) {
+  avoidSingleSelectedTab() {
+    if (this.multiSelectedTabsCount == 1 ) {
       this.clearMultiSelectedTabs();
     }
   },
 
   switchToNextMultiSelectedTab() {
     this._clearMultiSelectionLocked = true;
+
+    // Guarantee that _clearMultiSelectionLocked lock gets released.
     try {
       let lastMultiSelectedTab = gBrowser.lastMultiSelectedTab;
       if (lastMultiSelectedTab != gBrowser.selectedTab) {
         gBrowser.selectedTab = lastMultiSelectedTab;
       } else {
         let selectedTabs = ChromeUtils.nondeterministicGetWeakSetKeys(this._multiSelectedTabsSet)
           .filter(tab => tab.isConnected && !tab.closing);
         let length = selectedTabs.length;
         gBrowser.selectedTab = selectedTabs[length - 1];
       }
     } catch (e) {
       Cu.reportError(e);
     }
+
     this._clearMultiSelectionLocked = false;
   },
 
   set selectedTabs(tabs) {
     this.clearMultiSelectedTabs(false);
     this.selectedTab = tabs[0];
     if (tabs.length > 1) {
       for (let tab of tabs) {
@@ -5093,20 +5140,19 @@ var TabContextMenu = {
     contextUnpinSelectedTabs.hidden = !this.contextTab.pinned || !multiselectionContext;
 
     // Disable "Close Tabs to the Right" if there are no tabs
     // following it.
     document.getElementById("context_closeTabsToTheEnd").disabled =
       gBrowser.getTabsToTheEndFrom(this.contextTab).length == 0;
 
     // Disable "Close other Tabs" if there are no unpinned tabs.
-    let unpinnedTabsToClose = gBrowser.visibleTabs.length - gBrowser._numPinnedTabs;
-    if (!this.contextTab.pinned) {
-      unpinnedTabsToClose--;
-    }
+    let unpinnedTabsToClose = multiselectionContext ?
+      gBrowser.visibleTabs.filter(t => !t.multiselected && !t.pinned).length :
+      gBrowser.visibleTabs.filter(t => t != this.contextTab && !t.pinned).length;
     document.getElementById("context_closeOtherTabs").disabled = unpinnedTabsToClose < 1;
 
     // Only one of close_tab/close_selected_tabs should be visible
     document.getElementById("context_closeTab").hidden = multiselectionContext;
     document.getElementById("context_closeSelectedTabs").hidden = !multiselectionContext;
 
     // Hide "Bookmark All Tabs" for a pinned tab or multiselection.
     // Update its state if visible.
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -2081,17 +2081,17 @@
           }
           if (accelKey) {
             // Ctrl (Cmd for mac) key is pressed
             if (this.multiselected) {
               gBrowser.removeFromMultiSelectedTabs(this);
               if (this == gBrowser.selectedTab) {
                 gBrowser.switchToNextMultiSelectedTab();
               }
-              gBrowser.updateActiveTabMultiSelectState();
+              gBrowser.avoidSingleSelectedTab();
             } else if (this != gBrowser.selectedTab) {
               for (let tab of [this, gBrowser.selectedTab]) {
                 gBrowser.addToMultiSelectedTabs(tab, true);
               }
               gBrowser.tabContainer._setPositionalAttributes();
               gBrowser.lastMultiSelectedTab = this;
             }
             return;
--- a/browser/base/content/test/tabs/browser.ini
+++ b/browser/base/content/test/tabs/browser.ini
@@ -18,16 +18,17 @@ tags = audiochannel
 skip-if = (verify && debug && (os == 'linux'))
 support-files =
   test_bug1358314.html
 [browser_isLocalAboutURI.js]
 [browser_multiselect_tabs_active_tab_selected_by_default.js]
 [browser_multiselect_tabs_bookmark.js]
 [browser_multiselect_tabs_clear_selection_when_tab_switch.js]
 [browser_multiselect_tabs_close_other_tabs.js]
+[browser_multiselect_tabs_close_tabs_to_the_right.js]
 [browser_multiselect_tabs_close_using_shortcuts.js]
 [browser_multiselect_tabs_close.js]
 [browser_multiselect_tabs_move_to_new_window_contextmenu.js]
 [browser_multiselect_tabs_mute_unmute.js]
 [browser_multiselect_tabs_pin_unpin.js]
 [browser_multiselect_tabs_positional_attrs.js]
 [browser_multiselect_tabs_reload.js]
 [browser_multiselect_tabs_reorder.js]
--- a/browser/base/content/test/tabs/browser_multiselect_tabs_close_other_tabs.js
+++ b/browser/base/content/test/tabs/browser_multiselect_tabs_close_other_tabs.js
@@ -60,33 +60,41 @@ add_task(async function withAMultiSelect
 });
 
 add_task(async function withNotAMultiSelectedTab() {
   let initialTab = gBrowser.selectedTab;
   let tab1 = await addTab();
   let tab2 = await addTab();
   let tab3 = await addTab();
   let tab4 = await addTab();
+  let tab5 = await addTab();
 
   is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs");
 
   await BrowserTestUtils.switchTab(gBrowser, tab1);
   await triggerClickOn(tab2, { ctrlKey: true });
+  await triggerClickOn(tab5, { ctrlKey: true });
 
   let tab4Pinned = BrowserTestUtils.waitForEvent(tab4, "TabPinned");
   gBrowser.pinTab(tab4);
   await tab4Pinned;
 
+  let tab5Pinned = BrowserTestUtils.waitForEvent(tab5, "TabPinned");
+  gBrowser.pinTab(tab5);
+  await tab5Pinned;
+
   ok(!initialTab.multiselected, "InitialTab is not multiselected");
   ok(tab1.multiselected, "Tab1 is multiselected");
   ok(tab2.multiselected, "Tab2 is multiselected");
   ok(!tab3.multiselected, "Tab3 is not multiselected");
   ok(!tab4.multiselected, "Tab4 is not multiselected");
   ok(tab4.pinned, "Tab4 is pinned");
-  is(gBrowser.multiSelectedTabsCount, 2, "Two multiselected tabs");
+  ok(tab5.multiselected, "Tab5 is multiselected");
+  ok(tab5.pinned, "Tab5 is pinned");
+  is(gBrowser.multiSelectedTabsCount, 3, "Three multiselected tabs");
   is(gBrowser.selectedTab, tab1, "Tab1 is the active tab");
 
   let closingTabs = [tab1, tab2, tab3];
   let tabClosingPromises = [];
   for (let tab of closingTabs) {
     tabClosingPromises.push(BrowserTestUtils.waitForTabClosing(tab));
   }
 
@@ -96,13 +104,15 @@ add_task(async function withNotAMultiSel
     await promise;
   }
 
   ok(!initialTab.closing, "InitialTab is not closing");
   ok(tab1.closing, "Tab1 is closing");
   ok(tab2.closing, "Tab2 is closing");
   ok(tab3.closing, "Tab3 is closing");
   ok(!tab4.closing, "Tab4 is not closing");
-  is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs");
+  ok(!tab5.closing, "Tab5 is not closing");
+  is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs, selection is cleared");
   is(gBrowser.selectedTab, initialTab, "InitialTab is the active tab now");
 
-  BrowserTestUtils.removeTab(tab4);
+  for (let tab of [tab4, tab5])
+    BrowserTestUtils.removeTab(tab);
 });
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/tabs/browser_multiselect_tabs_close_tabs_to_the_right.js
@@ -0,0 +1,114 @@
+const PREF_MULTISELECT_TABS = "browser.tabs.multiselect";
+const PREF_WARN_ON_CLOSE = "browser.tabs.warnOnCloseOtherTabs";
+
+add_task(async function setPref() {
+  await SpecialPowers.pushPrefEnv({
+    set: [
+      [PREF_MULTISELECT_TABS, true],
+      [PREF_WARN_ON_CLOSE, false]
+    ]
+  });
+});
+
+add_task(async function withAMultiSelectedTab() {
+  let tab1 = await addTab();
+  let tab2 = await addTab();
+  let tab3 = await addTab();
+  let tab4 = await addTab();
+  let tab5 = await addTab();
+
+  is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs");
+
+  await BrowserTestUtils.switchTab(gBrowser, tab1);
+  await triggerClickOn(tab3, { ctrlKey: true });
+
+  ok(tab1.multiselected, "Tab1 is multiselected");
+  ok(!tab2.multiselected, "Tab2 is not multiselected");
+  ok(tab3.multiselected, "Tab3 is multiselected");
+  ok(!tab4.multiselected, "Tab4 is not multiselected");
+  ok(!tab5.multiselected, "Tab5 is not multiselected");
+  is(gBrowser.multiSelectedTabsCount, 2, "Two multiselected tabs");
+
+  let closingTabs = [tab4, tab5];
+  let tabClosingPromises = [];
+  for (let tab of closingTabs) {
+    tabClosingPromises.push(BrowserTestUtils.waitForTabClosing(tab));
+  }
+
+  gBrowser.removeTabsToTheEndFrom(tab1);
+
+  for (let promise of tabClosingPromises) {
+    await promise;
+  }
+
+  ok(!tab1.closing, "Tab1 is not closing");
+  ok(!tab2.closing, "Tab2 is not closing");
+  ok(!tab3.closing, "Tab3 is not closing");
+  ok(tab4.closing, "Tab4 is closing");
+  ok(tab5.closing, "Tab5 is closing");
+  is(gBrowser.multiSelectedTabsCount, 2, "Two multiselected tabs");
+
+  for (let tab of [tab1, tab2, tab3])
+    BrowserTestUtils.removeTab(tab);
+});
+
+add_task(async function withNotAMultiSelectedTab() {
+  let tab1 = await addTab();
+  let tab2 = await addTab();
+  let tab3 = await addTab();
+  let tab4 = await addTab();
+  let tab5 = await addTab();
+
+  is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs");
+
+  await BrowserTestUtils.switchTab(gBrowser, tab1);
+  await triggerClickOn(tab3, { ctrlKey: true });
+  await triggerClickOn(tab5, { ctrlKey: true });
+
+  ok(tab1.multiselected, "Tab1 is multiselected");
+  ok(!tab2.multiselected, "Tab2 is not multiselected");
+  ok(tab3.multiselected, "Tab3 is multiselected");
+  ok(!tab4.multiselected, "Tab4 is not multiselected");
+  ok(tab5.multiselected, "Tab5 is multiselected");
+  is(gBrowser.multiSelectedTabsCount, 3, "Three multiselected tabs");
+
+  let closingTabs = [tab5];
+  let tabClosingPromises = [];
+  for (let tab of closingTabs) {
+    tabClosingPromises.push(BrowserTestUtils.waitForTabClosing(tab));
+  }
+
+  gBrowser.removeTabsToTheEndFrom(tab4);
+
+  for (let promise of tabClosingPromises) {
+    await promise;
+  }
+
+  ok(!tab1.closing, "Tab1 is not closing");
+  ok(!tab2.closing, "Tab2 is not closing");
+  ok(!tab3.closing, "Tab3 is not closing");
+  ok(!tab4.closing, "Tab4 is not closing");
+  ok(tab5.closing, "Tab5 is closing");
+  is(gBrowser.multiSelectedTabsCount, 2, "Selection is not cleared");
+
+  closingTabs = [tab3, tab4];
+  tabClosingPromises = [];
+  for (let tab of closingTabs) {
+    tabClosingPromises.push(BrowserTestUtils.waitForTabClosing(tab));
+  }
+
+  gBrowser.removeTabsToTheEndFrom(tab2);
+
+  for (let promise of tabClosingPromises) {
+    await promise;
+  }
+
+  ok(!tab1.closing, "Tab1 is not closing");
+  ok(!tab2.closing, "Tab2 is not closing");
+  ok(tab3.closing, "Tab3 is closing");
+  ok(tab4.closing, "Tab4 is closing");
+  is(gBrowser.multiSelectedTabsCount, 0, "Selection is cleared");
+
+  for (let tab of [tab1, tab2])
+    BrowserTestUtils.removeTab(tab);
+});