Bug 1933813 - "close other tabs" changes for tab groups r=dao,jswinarton,sessionstore-reviewers,tabbrowser-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D232430

browser/components/sessionstore/SessionStore.sys.mjs

@@ -4359,7 +4359,7 @@ var SessionStoreInternal = {
     );
     if (savedGroupIndex < 0) {
       throw Components.Exception(
-        "Closed tab group not found",
+        "Saved tab group not found",
         Cr.NS_ERROR_INVALID_ARG
       );
     }

browser/components/sessionstore/test/browser.toml

@@ -296,6 +296,8 @@ tags = "os_integration"
 
 ["browser_tab_groups_restore_simple.js"]
 
+["browser_tab_groups_save_on_removeAllTabsBut.js"]
+
 ["browser_tab_groups_save_on_window_close.js"]
 
 ["browser_tab_groups_saved.js"]

browser/components/sessionstore/test/browser_tab_groups_restore_simple.js

@@ -62,6 +62,7 @@ add_task(async function test_RestoreSingleGroup() {
     "tab group collapsed state should be restored"
   );
 
+  win.gBrowser.removeTabGroup(tabGroup);
   await BrowserTestUtils.closeWindow(win);
   forgetClosedWindows();
 });

browser/components/sessionstore/test/browser_tab_groups_save_on_removeAllTabsBut.js

@@ -0,0 +1,127 @@
+/* Any copyright is dedicated to the Public Domain.
+   https://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const ORIG_STATE = SessionStore.getBrowserState();
+
+registerCleanupFunction(async () => {
+  await SessionStoreTestUtils.promiseBrowserState(ORIG_STATE);
+});
+
+add_task(async function test_removeAllTabsBut_default_save_tab_groups() {
+  let win = await promiseNewWindowLoaded();
+  let tab1 = BrowserTestUtils.addTab(win.gBrowser, "about:robots");
+  let tab2 = BrowserTestUtils.addTab(win.gBrowser, "about:robots");
+  let tab3 = BrowserTestUtils.addTab(win.gBrowser, "about:robots");
+
+  let tabGroup = win.gBrowser.addTabGroup([tab2, tab3]);
+  let tabGroupId = tabGroup.id;
+
+  await TabStateFlusher.flushWindow(win);
+
+  Assert.equal(
+    SessionStore.getSavedTabGroups().length,
+    0,
+    "should not be any saved tab groups to start"
+  );
+  Assert.equal(
+    SessionStore.getClosedTabGroups(win).length,
+    0,
+    "should not be any closed tab groups to start"
+  );
+  Assert.equal(
+    SessionStore.getClosedTabDataForWindow(win).length,
+    0,
+    "should not be any closed tabs"
+  );
+
+  win.gBrowser.removeAllTabsBut(tab1);
+
+  await TestUtils.waitForCondition(
+    () => win.gBrowser.tabs.length == 1,
+    "waiting for other tabs to close"
+  );
+
+  await TabStateFlusher.flushWindow(win);
+
+  Assert.equal(
+    SessionStore.getSavedTabGroups().length,
+    1,
+    "should have saved a tab group"
+  );
+  Assert.ok(
+    SessionStore.getSavedTabGroup(tabGroupId),
+    "should have saved the tab group that was closed"
+  );
+  Assert.equal(
+    SessionStore.getClosedTabGroups(win).length,
+    0,
+    "should only have saved the tab group, not deleted it"
+  );
+  Assert.equal(
+    SessionStore.getClosedTabDataForWindow(win).length,
+    0,
+    "should not be any closed tabs"
+  );
+
+  await BrowserTestUtils.closeWindow(win);
+  forgetClosedWindows();
+  forgetSavedTabGroups();
+});
+
+add_task(async function test_removeAllTabsBut_suppress_saving_tab_groups() {
+  let win = await promiseNewWindowLoaded();
+  let tab1 = BrowserTestUtils.addTab(win.gBrowser, "about:robots");
+  let tab2 = BrowserTestUtils.addTab(win.gBrowser, "about:robots");
+  let tab3 = BrowserTestUtils.addTab(win.gBrowser, "about:robots");
+
+  win.gBrowser.addTabGroup([tab2, tab3]);
+
+  await TabStateFlusher.flushWindow(win);
+
+  Assert.equal(
+    SessionStore.getSavedTabGroups().length,
+    0,
+    "should not be any saved tab groups to start"
+  );
+  Assert.equal(
+    SessionStore.getClosedTabGroups(win).length,
+    0,
+    "should not be any closed tab groups to start"
+  );
+  Assert.equal(
+    SessionStore.getClosedTabDataForWindow(win).length,
+    0,
+    "should not be any closed tabs"
+  );
+
+  win.gBrowser.removeAllTabsBut(tab1, { skipSessionStore: true });
+
+  await TestUtils.waitForCondition(
+    () => win.gBrowser.tabs.length == 1,
+    "waiting for other tabs to close"
+  );
+
+  await TabStateFlusher.flushWindow(win);
+
+  Assert.equal(
+    SessionStore.getSavedTabGroups().length,
+    0,
+    "should not have saved the tab group"
+  );
+  Assert.equal(
+    SessionStore.getClosedTabGroups(win),
+    0,
+    "should still not have any deleted tab groups"
+  );
+  Assert.equal(
+    SessionStore.getClosedTabDataForWindow(win).length,
+    0,
+    "should be 0 closed tabs"
+  );
+
+  await BrowserTestUtils.closeWindow(win);
+  forgetClosedWindows();
+  forgetSavedTabGroups();
+});

browser/components/tabbrowser/content/tabbrowser.js

@@ -2969,7 +2969,6 @@
      * Removes the tab group. This has the effect of closing all the tabs
      * in the group.
      *
-     *
      * @param {MozTabbrowserTabGroup} [group]
      *   The tab group to remove.
      * @param {object} [options]
@@ -4009,18 +4008,20 @@
     }
 
     /**
-     * Remove all tabs but aTab. By default, in a multi-select context, all
+     * Remove all tabs but `aTab`. By default, in a multi-select context, all
      * unpinned and unselected tabs are removed. Otherwise all unpinned tabs
      * except aTab are removed. This behavior can be changed using the the bool
      * flags below.
      *
-     * @param   aTab The tab we will skip removing
-     * @param   aParams An optional set of parameters that will be passed to the
-     *          removeTabs function.
-     * @param   {boolean} [aParams.skipWarnAboutClosingTabs=false] Skip showing
-     *          the tab close warning prompt.
-     * @param   {boolean} [aParams.skipPinnedOrSelectedTabs=true] Skip closing
-     *          tabs that are selected or pinned.
+     * @param {MozTabbrowserTab} aTab
+     *   The tab we will skip removing
+     * @param {object} [aParams]
+     *   An optional set of parameters that will be passed to the
+     *   `removeTabs` function.
+     * @param {boolean} [aParams.skipWarnAboutClosingTabs=false]
+     *   Skip showing the tab close warning prompt.
+     * @param {boolean} [aParams.skipPinnedOrSelectedTabs=true]
+     *   Skip closing tabs that are selected or pinned.
      */
     removeAllTabsBut(aTab, aParams = {}) {
       let {
@@ -4028,21 +4029,22 @@
         skipPinnedOrSelectedTabs = true,
       } = aParams;
 
+      /** @type {function(MozTabbrowserTab):boolean} */
       let filterFn;
 
       // If enabled also filter by selected or pinned state.
       if (skipPinnedOrSelectedTabs) {
         if (aTab?.multiselected) {
-          filterFn = tab => !tab.multiselected && !tab.pinned;
+          filterFn = tab => !tab.multiselected && !tab.pinned && !tab.hidden;
         } else {
-          filterFn = tab => tab != aTab && !tab.pinned;
+          filterFn = tab => tab != aTab && !tab.pinned && !tab.hidden;
         }
       } else {
         // Exclude just aTab from being removed.
         filterFn = tab => tab != aTab;
       }
 
-      let tabsToRemove = this.visibleTabs.filter(filterFn);
+      let tabsToRemove = this.openTabs.filter(filterFn);
 
       // If enabled show the tab close warning.
       if (
@@ -4074,7 +4076,7 @@
 
     /**
      * @typedef {object} _startRemoveTabsReturnValue
-     * @property {Promise} beforeUnloadComplete
+     * @property {Promise<void>} beforeUnloadComplete
      *   A promise that is resolved once all the beforeunload handlers have been
      *   called.
      * @property {object[]} tabsWithBeforeUnloadPrompt
@@ -4117,15 +4119,36 @@
     ) {
       // Note: if you change any of the unload algorithm, consider also
       // changing `runBeforeUnloadForTabs` above.
+      /** @type {MozTabbrowserTab[]} */
       let tabsWithBeforeUnloadPrompt = [];
+      /** @type {MozTabbrowserTab[]} */
       let tabsWithoutBeforeUnload = [];
+      /** @type {Promise<void>[]} */
       let beforeUnloadPromises = [];
+      /** @type {MozTabbrowserTab|undefined} */
       let lastToClose;
+      /**
+       * Map of tab group to surviving tabs in the group.
+       * If any of the `tabs` to be removed belong to a tab group, keep track
+       * of how many tabs in the tab group will be left after removing `tabs`.
+       * For any tab group with 0 surviving tabs, we can know that that tab
+       * group will be removed as a consequence of removing these `tabs`.
+       * @type {Map<MozTabbrowserTabGroup, Set<MozTabbrowserTab>>}
+       */
+      let tabGroupsSurvivingTabs = new Map();
 
       for (let tab of tabs) {
         if (!skipRemoves) {
           tab._closedInGroup = true;
         }
+        if (!skipRemoves && !skipSessionStore) {
+          if (tab.group) {
+            if (!tabGroupsSurvivingTabs.has(tab.group)) {
+              tabGroupsSurvivingTabs.set(tab.group, new Set(tab.group.tabs));
+            }
+            tabGroupsSurvivingTabs.get(tab.group).delete(tab);
+          }
+        }
         if (!skipRemoves && tab.selected) {
           lastToClose = tab;
           let toBlurTo = this._findTabToBlurTo(lastToClose, tabs);
@@ -4182,6 +4205,22 @@
         }
       }
 
+      if (!skipRemoves && !skipSessionStore) {
+        for (let [
+          tabGroup,
+          survivingTabs,
+        ] of tabGroupsSurvivingTabs.entries()) {
+          // Before removing any tabs, save tab groups that won't survive
+          // because all of their tabs are about to be removed. Then remove
+          // the tab group directly to prevent the closing tabs from being
+          // recorded by the session as individually closed tabs.
+          if (!survivingTabs.size) {
+            tabGroup.save();
+            this.removeTabGroup(tabGroup);
+          }
+        }
+      }
+
       // Now that all the beforeunload IPCs have been sent to content processes,
       // we can queue unload messages for all the tabs without beforeunload listeners.
       // Doing this first would cause content process main threads to be busy and delay
@@ -4252,7 +4291,7 @@
     /**
      * Removes multiple tabs from the tab browser.
      *
-     * @param {object[]} tabs
+     * @param {MozTabbrowserTab[]} tabs
      *   The set of tabs to remove.
      * @param {object} [options]
      * @param {boolean} [options.animate]
@@ -8427,9 +8466,12 @@ var TabContextMenu = {
 
     // Disable "Close other Tabs" if there are no unpinned tabs.
     let unpinnedTabsToClose = multiselectionContext
-      ? gBrowser.visibleTabs.filter(t => !t.multiselected && !t.pinned).length
-      : gBrowser.visibleTabs.filter(t => t != this.contextTab && !t.pinned)
-          .length;
+      ? gBrowser.openTabs.filter(
+          t => !t.multiselected && !t.pinned && !t.hidden
+        ).length
+      : gBrowser.openTabs.filter(
+          t => t != this.contextTab && !t.pinned && !t.hidden
+        ).length;
     let closeOtherTabsItem = document.getElementById("context_closeOtherTabs");
     closeOtherTabsItem.disabled = unpinnedTabsToClose < 1;
 

browser/components/tabbrowser/test/browser/tabs/browser.toml

@@ -384,6 +384,9 @@ tags = "vertical-tabs"
 tags = "vertical-tabs"
 skip-if = ["os == 'mac' && vertical_tab"] # Bug 1936168
 
+["browser_removeAllTabsBut.js"]
+tags = "vertical-tabs"
+
 ["browser_removeTabsToTheEnd.js"]
 tags = "vertical-tabs"
 

browser/components/tabbrowser/test/browser/tabs/browser_removeAllTabsBut.js

@@ -0,0 +1,134 @@
+/* Any copyright is dedicated to the Public Domain.
+   https://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+add_task(async function test_removeAllButSingleTab() {
+  let win = await BrowserTestUtils.openNewBrowserWindow();
+  let tab1 = await addTabTo(win.gBrowser);
+  let tab2 = await addTabTo(win.gBrowser);
+  let tab3 = await addTabTo(win.gBrowser);
+
+  Assert.equal(
+    win.gBrowser.tabs.length,
+    4,
+    "should be 1 new tab from window + 3 added tabs from test"
+  );
+
+  win.gBrowser.removeAllTabsBut(tab2);
+
+  await TestUtils.waitForCondition(
+    () => win.gBrowser.tabs.length == 1,
+    "waiting for other tabs to close"
+  );
+
+  Assert.ok(
+    !win.gBrowser.tabs.some(tab => tab == tab1),
+    "tab1 should have been closed"
+  );
+  Assert.ok(
+    win.gBrowser.tabs.some(tab => tab == tab2),
+    "tab2 should still be present"
+  );
+  Assert.ok(
+    !win.gBrowser.tabs.some(tab => tab == tab3),
+    "tab3 should have been closed"
+  );
+
+  await BrowserTestUtils.closeWindow(win);
+});
+
+add_task(async function test_removesAllButMultiselectedTabs() {
+  let win = await BrowserTestUtils.openNewBrowserWindow();
+  let tab1 = await addTabTo(win.gBrowser);
+  let tab2 = await addTabTo(win.gBrowser);
+  let tab3 = await addTabTo(win.gBrowser);
+  let tab4 = await addTabTo(win.gBrowser);
+  let tab5 = await addTabTo(win.gBrowser);
+
+  Assert.equal(
+    win.gBrowser.tabs.length,
+    6,
+    "should be 1 new tab from window + 5 added tabs from test"
+  );
+
+  win.gBrowser.selectedTabs = [tab2, tab4];
+
+  win.gBrowser.removeAllTabsBut(tab2);
+
+  await TestUtils.waitForCondition(
+    () => win.gBrowser.tabs.length == 2,
+    "waiting for other tabs to close"
+  );
+
+  Assert.ok(
+    !win.gBrowser.tabs.some(tab => tab == tab1),
+    "tab1 should have been closed"
+  );
+  Assert.ok(
+    win.gBrowser.tabs.some(tab => tab == tab2),
+    "tab2 should still be present"
+  );
+  Assert.ok(
+    !win.gBrowser.tabs.some(tab => tab == tab3),
+    "tab3 should have been closed"
+  );
+  Assert.ok(
+    win.gBrowser.tabs.some(tab => tab == tab4),
+    "tab4 should still be present"
+  );
+  Assert.ok(
+    !win.gBrowser.tabs.some(tab => tab == tab5),
+    "tab5 should have been closed"
+  );
+
+  await BrowserTestUtils.closeWindow(win);
+});
+
+add_task(async function test_removesAllIncludingTabGroups() {
+  let win = await BrowserTestUtils.openNewBrowserWindow();
+  let tab1 = await addTabTo(win.gBrowser);
+  let tab2 = await addTabTo(win.gBrowser);
+  let tab3 = await addTabTo(win.gBrowser);
+  let tab4 = await addTabTo(win.gBrowser);
+
+  win.gBrowser.addTabGroup([tab3, tab4]);
+
+  Assert.equal(
+    win.gBrowser.tabs.length,
+    5,
+    "should be 1 new tab from window + 5 added tabs from test"
+  );
+  Assert.equal(win.gBrowser.tabGroups.length, 1, "should be 1 tab group");
+
+  win.gBrowser.removeAllTabsBut(tab2);
+
+  await TestUtils.waitForCondition(
+    () => win.gBrowser.tabs.length == 1,
+    "waiting for other tabs to close"
+  );
+
+  Assert.ok(
+    !win.gBrowser.tabs.some(tab => tab == tab1),
+    "tab1 should have been closed"
+  );
+  Assert.ok(
+    win.gBrowser.tabs.some(tab => tab == tab2),
+    "tab2 should still be present"
+  );
+  Assert.ok(
+    !win.gBrowser.tabs.some(tab => tab == tab3),
+    "tab3 should have been closed"
+  );
+  Assert.ok(
+    !win.gBrowser.tabs.some(tab => tab == tab4),
+    "tab4 should have been closed"
+  );
+  Assert.equal(
+    win.gBrowser.tabGroups.length,
+    0,
+    "tab group should have been deleted"
+  );
+
+  await BrowserTestUtils.closeWindow(win);
+});