Merge pull request #2754 from apostrophest/issue-2747-reopen-in-container-tab-groups

Fix #2747: open/reopen container tabs in tab groups when appropriate

src/js/background/assignManager.js

@@ -326,7 +326,8 @@ window.assignManager = {
         options.url,
         tab.index + 1,
         tab.active,
-        openTabId
+        openTabId,
+        tab.groupId
       );
     } else {
       this.reloadPageInContainer(
@@ -336,7 +337,8 @@ window.assignManager = {
         tab.index + 1,
         tab.active,
         siteSettings.neverAsk,
-        openTabId
+        openTabId,
+        tab.groupId
       );
     }
     this.calculateContextMenu(tab);
@@ -727,7 +729,15 @@ window.assignManager = {
     });
   },
 
-  reloadPageInDefaultContainer(url, index, active, openerTabId) {
+  /**
+   * @param {string} url
+   * @param {number} index
+   * @param {boolean} active
+   * @param {number} [openerTabId]
+   * @param {number} [groupId]
+   * @returns {void}
+   */
+  reloadPageInDefaultContainer(url, index, active, openerTabId, groupId) {
     // To create a new tab in the default container, it is easiest just to omit the
     // cookieStoreId entirely.
     //
@@ -746,16 +756,58 @@ window.assignManager = {
     // does not automatically return to the original opener tab. To get this desired behaviour,
     // we MUST specify the openerTabId when creating the new tab.
     const cookieStoreId = "firefox-default";
-    browser.tabs.create({url, cookieStoreId, index, active, openerTabId});
+    this.createTabWrapper(url, cookieStoreId, index, active, openerTabId, groupId);
   },
 
-  reloadPageInContainer(url, currentUserContextId, userContextId, index, active, neverAsk = false, openerTabId = null) {
+
+  /**
+   * Wraps around `browser.tabs.create` and `browser.tabs.group` to create a
+   * tab and ensure that it ends up in the requested tab group, if applicable.
+   *
+   * @param {string} url
+   * @param {string} cookieStoreId
+   * @param {number} index
+   * @param {boolean} active
+   * @param {number} openerTabId
+   * @param {number} [groupId] Tab group ID
+   * @returns {Promise<Tab>}
+   */
+  async createTabWrapper(url, cookieStoreId, index, active, openerTabId, groupId) {
+    const newTab = await browser.tabs.create({
+      url,
+      cookieStoreId,
+      index,
+      active,
+      openerTabId,
+    });
+
+    if (groupId >= 0) {
+      // If the original tab was in a tab group, make sure that the reopened tab
+      // stays in the same tab group.
+      await browser.tabs.group({ groupId, tabIds: newTab.id });
+    }
+
+    return newTab;
+  },
+
+  /**
+   * @param {string} url
+   * @param {string} currentUserContextId
+   * @param {string} userContextId
+   * @param {number} index
+   * @param {boolean} active
+   * @param {boolean} [neverAsk=false]
+   * @param {number} [openerTabId=null]
+   * @param {number} [groupId]
+   * @returns {Promise<Tab>}
+   */
+  reloadPageInContainer(url, currentUserContextId, userContextId, index, active, neverAsk = false, openerTabId = null, groupId = undefined) {
     const cookieStoreId = backgroundLogic.cookieStoreId(userContextId);
     const loadPage = browser.runtime.getURL("confirm-page.html");
     // False represents assignment is not permitted
     // If the user has explicitly checked "Never Ask Again" on the warning page we will send them straight there
     if (neverAsk) {
-      return browser.tabs.create({url, cookieStoreId, index, active, openerTabId});
+      return this.createTabWrapper(url, cookieStoreId, index, active, openerTabId, groupId);
     } else {
       let confirmUrl = `${loadPage}?url=${this.encodeURLProperty(url)}&cookieStoreId=${cookieStoreId}`;
       let currentCookieStoreId;
@@ -763,13 +815,14 @@ window.assignManager = {
         currentCookieStoreId = backgroundLogic.cookieStoreId(currentUserContextId);
         confirmUrl += `&currentCookieStoreId=${currentCookieStoreId}`;
       }
-      return browser.tabs.create({
-        url: confirmUrl,
-        cookieStoreId: currentCookieStoreId,
-        openerTabId,
+      return this.createTabWrapper(
+        confirmUrl,
+        currentCookieStoreId,
         index,
-        active
-      }).then(() => {
+        active,
+        openerTabId,
+        groupId
+      ).then(() => {
         // We don't want to sync this URL ever nor clutter the users history
         browser.history.deleteUrl({url: confirmUrl});
       }).catch((e) => {

src/js/background/messageHandler.js

@@ -91,7 +91,9 @@ const messageHandler = {
           m.newUserContextId,
           m.tabIndex,
           m.active,
-          true
+          true,
+          null,
+          m.groupId
         );
         break;
       case "assignAndReloadInContainer":
@@ -101,7 +103,9 @@ const messageHandler = {
           m.newUserContextId,
           m.tabIndex,
           m.active,
-          true
+          true,
+          null,
+          m.groupId
         );
         // m.tabId is used for where to place the in content message
         // m.url is the assignment to be removed/added

src/js/confirm-page.js

@@ -69,11 +69,15 @@ function confirmSubmit(redirectUrl, cookieStoreId) {
   openInContainer(redirectUrl, cookieStoreId);
 }
 
-function getCurrentTab() {
-  return browser.tabs.query({
+/**
+ * @returns {Promise<Tab>}
+ */
+async function getCurrentTab() {
+  const tabs = await browser.tabs.query({
     active: true,
     windowId: browser.windows.WINDOW_ID_CURRENT
   });
+  return tabs[0];
 }
 
 async function denySubmit(redirectUrl, currentCookieStoreId) {
@@ -93,7 +97,7 @@ async function denySubmit(redirectUrl, currentCookieStoreId) {
 
   await browser.runtime.sendMessage({
     method: "exemptContainerAssignment",
-    tabId: tab[0].id,
+    tabId: tab.id,
     pageUrl: redirectUrl
   });
   document.location.replace(redirectUrl);
@@ -103,12 +107,15 @@ load();
 
 async function openInContainer(redirectUrl, cookieStoreId) {
   const tab = await getCurrentTab();
-  await browser.tabs.create({
-    index: tab[0].index + 1,
+  const reopenedTab = await browser.tabs.create({
+    index: tab.index + 1,
     cookieStoreId,
     url: redirectUrl
   });
-  if (tab.length > 0) {
-    browser.tabs.remove(tab[0].id);
+  if (tab.groupId >= 0) {
+    // If the original tab was in a tab group, make sure that the reopened tab
+    // stays in the same tab group.
+    await browser.tabs.group({ groupId: tab.groupId, tabIds: reopenedTab.id });
   }
+  await browser.tabs.remove(tab.id);
 }

src/js/popup.js

@@ -1306,7 +1306,8 @@ Logic.registerPanel(REOPEN_IN_CONTAINER_PICKER, {
         false,
         newUserContextId,
         currentTab.index + 1,
-        currentTab.active
+        currentTab.active,
+        currentTab.groupId
       );
       window.close();
     };
@@ -1336,7 +1337,8 @@ Logic.registerPanel(REOPEN_IN_CONTAINER_PICKER, {
           false,
           0,
           currentTab.index + 1,
-          currentTab.active
+          currentTab.active,
+          currentTab.groupId
         );
         window.close();
       });

src/js/utils.js

@@ -94,6 +94,9 @@ const Utils = {
     return result.join("");
   },
 
+  /**
+   * @returns {Promise<Tab|false>}
+   */
   async currentTab() {
     const activeTabs = await browser.tabs.query({ active: true, windowId: browser.windows.WINDOW_ID_CURRENT });
     if (activeTabs.length > 0) {
@@ -146,14 +149,24 @@ const Utils = {
     });
   },
 
-  async reloadInContainer(url, currentUserContextId, newUserContextId, tabIndex, active) {
+  /**
+   * @param {string} url
+   * @param {string} currentUserContextId
+   * @param {string} newUserContextId
+   * @param {number} tabIndex
+   * @param {boolean} active
+   * @param {number} [groupId]
+   * @returns {Promise<any>}
+   */
+  async reloadInContainer(url, currentUserContextId, newUserContextId, tabIndex, active, groupId = undefined) {
     return await browser.runtime.sendMessage({
       method: "reloadInContainer",
       url,
       currentUserContextId,
       newUserContextId,
       tabIndex,
-      active
+      active,
+      groupId
     });
   },
 
@@ -167,7 +180,8 @@ const Utils = {
         currentUserContextId: false,
         newUserContextId: assignedUserContextId,
         tabIndex: currentTab.index +1,
-        active:currentTab.active
+        active: currentTab.active,
+        groupId: currentTab.groupId
       });
     }
     await Utils.setOrRemoveAssignment(