1932594 - Creating a Tab Group by moving a Tab past a 2nd Tab and then grouping them together will only group one tab

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Rares Doghi, Desktop QA]

Attachment: 2024-11-21_13h47_42.mp4

Found in

• 134.0a1 (2024-11-21)

Affected versions

• 134.0a1 (2024-11-21)

Preconditions
Set browser.tabs.groups.enabled true

Affected platforms

• all

Steps to reproduce

1. Have a Few Tabs opened.
2. Grab a Left Tab, move it past a 2nd tab to the right and then hover over the 2nd tab in order to create a Tab group.

Expected result

• Both tabs should be inside the Tab Group.

Actual result

• Only the Moving tab is displayed in the Created Tab Group.

Regression range
Not a Regression.

Comment 1

[Dão Gottwald [:dao]]

If I understand correctly, this might be due you slightly moving the mouse in the opposite direction before releasing the mouse button. To address that, we need to keep the tab you're dragging over in the group even when moving the mouse back and forth.

Comment 2

[Rares Doghi, Desktop QA]

We can create a group from either way but if we move the first tab past the 2nd and they switch positions, trying to create a group after that without ever letting the first tab go it will just create a tab group with that tab alone.

Comment 3

[Dão Gottwald [:dao]]

Are you sure this isn't a regression? I mean, it's a new feature, so for sure it's not a regression as far as releases are concerned, but as far as mozilla-central is concerned, I suspect bug 1927540 may have broken this just a few days ago.

Comment 4

[Rares Doghi, Desktop QA]

Yes, thats correct Bug 1927540 is the culprit for this issue.

11:55.73 INFO: Last good revision: 8aa920f03974a85faf6ea3e7582d9df84890cdd6
11:55.73 INFO: First bad revision: 9a28415ddc286729136846743a6bc23ce9bd06ab
11:55.73 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8aa920f03974a85faf6ea3e7582d9df84890cdd6&tochange=9a28415ddc286729136846743a6bc23ce9bd06ab

@Stephen can you take a look at this ?

Comment 6

[tabmix.onemen]

The issue here is in on_drop.
When the action is GROUP_DROP_ACTION_CREATE and we have dropIndex we move the dragged tab before we call gBrowser.addTabGroup
in this case groupTab index is no longer valid.

The fix is simply to assign groupTab before we move any tab.

          this._finishAnimateTabMove();
+         let groupTab = this.allTabs[groupDropIndex];
          if (groupDropAction == GROUP_DROP_ACTION_APPEND) {
-           let groupTab = this.allTabs[groupDropIndex];
            groupTab.group.addTabs(movingTabs);
          } else if (dropIndex !== false) {
            for (let tab of movingTabs) {
              gBrowser.moveTabTo(tab, dropIndex);
              if (incrementDropIndex) {
                dropIndex++;
              }
            }
          }
          if (groupDropAction == GROUP_DROP_ACTION_CREATE) {
-           let groupTab = this.allTabs[groupDropIndex];
            gBrowser.addTabGroup([groupTab, ...movingTabs], {
              insertBefore: draggedTab,
              showCreateUI: true,
              color: draggedTab._dragData.tabGroupCreationColor,
            });
          }
        }

you can also apply gBrowser.addTabGroup([groupTabFixed, ...movingTabs].sort((a,b) => a._tPos - b._tPos) to fix bug 1933210

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1927540

Comment 8

[Stephen Thompson [:sthompson]]

To be fixed by https://phabricator.services.mozilla.com/D230283 as part of related bug 1933210

Comment 9

[Stephen Thompson [:sthompson]]

Resolved by bug 1933210

Comment 10

[Rares Doghi, Desktop QA]

Verified as fixed in our latest Nightly 135.0a1 (2024-12-04)

Comment 11

[Rares Doghi, Desktop QA]

Updating the main status flag.