From 4e0c51a78ef1ea304bdd2deb13e70aff5a7f5e23 Mon Sep 17 00:00:00 2001
From: DJ <dj@walker.dev>
Date: Wed, 7 May 2025 13:17:01 +0000
Subject: [PATCH] Bug 1956458 - Unpin tabs when they are added to a group.
 r=dao,tabbrowser-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D247597
---
 .../tabbrowser/content/tabbrowser.js          | 10 +++----
 .../components/tabbrowser/content/tabgroup.js |  5 ++++
 .../test/browser/tabs/browser_tab_groups.js   | 27 ++++++++++++-------
 3 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/browser/components/tabbrowser/content/tabbrowser.js b/browser/components/tabbrowser/content/tabbrowser.js
index c669c0617674..f4f9a90a2d58 100644
--- a/browser/components/tabbrowser/content/tabbrowser.js
+++ b/browser/components/tabbrowser/content/tabbrowser.js
@@ -8864,10 +8864,9 @@ var TabContextMenu = {
     let contextUngroupTab = document.getElementById("context_ungroupTab");
 
     if (gBrowser._tabGroupsEnabled) {
-      let groupableTabs = this.contextTabs.filter(t => !t.pinned);
       let selectedGroupCount = new Set(
         // The filter removes the "null" group for ungrouped tabs.
-        groupableTabs.map(t => t.group).filter(g => g)
+        this.contextTabs.map(t => t.group).filter(g => g)
       ).size;
 
       let availableGroupsToMoveTo = gBrowser
@@ -8879,16 +8878,13 @@ var TabContextMenu = {
       // Determine whether or not the "current" tab group should appear in the
       // "move tab to group" context menu.
       if (selectedGroupCount == 1) {
-        let groupToFilter = groupableTabs[0].group;
-        if (groupToFilter && groupableTabs.every(t => t.group)) {
+        let groupToFilter = this.contextTabs[0].group;
+        if (groupToFilter && this.contextTabs.every(t => t.group)) {
           availableGroupsToMoveTo = availableGroupsToMoveTo.filter(
             group => group !== groupToFilter
           );
         }
       }
-
-      contextMoveTabToGroup.disabled = !groupableTabs.length;
-      contextMoveTabToNewGroup.disabled = !groupableTabs.length;
       if (!availableGroupsToMoveTo.length) {
         contextMoveTabToGroup.hidden = true;
         contextMoveTabToNewGroup.hidden = false;
diff --git a/browser/components/tabbrowser/content/tabgroup.js b/browser/components/tabbrowser/content/tabgroup.js
index aad8fbe3bb0d..e4317083eb9f 100644
--- a/browser/components/tabbrowser/content/tabgroup.js
+++ b/browser/components/tabbrowser/content/tabgroup.js
@@ -305,6 +305,11 @@
      *   Optional context to record for metrics purposes.
      */
     addTabs(tabs, metricsContext) {
+      for (let tab of tabs) {
+        if (tab.pinned) {
+          tab.ownerGlobal.gBrowser.unpinTab(tab);
+        }
+      }
       for (let tab of tabs) {
         let tabToMove =
           this.ownerGlobal === tab.ownerGlobal
diff --git a/browser/components/tabbrowser/test/browser/tabs/browser_tab_groups.js b/browser/components/tabbrowser/test/browser/tabs/browser_tab_groups.js
index eed33652fd56..df11a12aa585 100644
--- a/browser/components/tabbrowser/test/browser/tabs/browser_tab_groups.js
+++ b/browser/components/tabbrowser/test/browser/tabs/browser_tab_groups.js
@@ -67,22 +67,31 @@ add_task(async function test_tabGroupCreateAndAddTabAtPosition() {
 
 add_task(async function test_pinned() {
   let tab1 = BrowserTestUtils.addTab(gBrowser, "about:blank");
-  gBrowser.pinTab(tab1);
-  let group = gBrowser.addTabGroup([tab1]);
-  Assert.ok(
-    !group,
-    "addTabGroup shouldn't create a group when only supplied with pinned tabs"
-  );
-
   let tab2 = BrowserTestUtils.addTab(gBrowser, "about:blank");
+  gBrowser.pinTab(tab1);
+  gBrowser.pinTab(tab2);
+  Assert.ok(tab1.pinned, "Tab1 is pinned");
+  Assert.ok(tab2.pinned, "Tab2 is pinned");
+  let group = gBrowser.addTabGroup([tab1, tab2]);
+  Assert.equal(
+    group.tabs.length,
+    2,
+    "addTabGroup creates a group when only supplied with pinned tabs"
+  );
+  Assert.ok(!tab1.pinned, "Tab1 is no longer pinned");
+  Assert.ok(!tab2.pinned, "Tab2 is no longer pinned");
+  group.ungroupTabs();
+  gBrowser.pinTab(tab1);
+  Assert.ok(tab1.pinned, "Tab1 is pinned again");
+
   group = gBrowser.addTabGroup([tab1, tab2]);
   Assert.ok(
     group,
     "addTabGroup should create a group when supplied with both pinned and non-pinned tabs"
   );
 
-  Assert.equal(group.tabs.length, 1, "group has only the non-pinned tab");
-  Assert.equal(group.tabs[0], tab2, "tab2 is in group");
+  Assert.equal(group.tabs.length, 2, "group contains both tabs");
+  Assert.ok(!tab1.pinned, "tab1 is no longer pinned");
 
   BrowserTestUtils.removeTab(tab1);
   await removeTabGroup(group);