corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1963825 - tabGroups.move index = desired final index r=zombie,tabbrowser-reviewers,dao

Browse Source 
To ensure that a group ends up at the position specified by `index`,
this patch adjusts the index when the tab group moves to the right.
Before this patch, the tab group appeared at a too low (left) index
because the original logic did not account for tabs shifting after a
repositioning to the right.

This also introduces stricter validation for moving tabs near pinned
tabs or other tab groups. Previously, the tabbrowser internals adjusted
the index as needed to fit adjacent to pinned tabs or groups. Now, the
extension API throws an error.

The new behavior matches developer expectations and Chrome's behavior:
https://bugzilla.mozilla.org/show_bug.cgi?id=1963825#c9

Differential Revision: https://phabricator.services.mozilla.com/D249493
This commit is contained in:
Rob Wu
2025-05-15 22:44:28 +00:00
committed by rob@robwu.nl
parent db7a4f65cc
commit c6223d7101
5 changed files with 386 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
browser/components/extensions/parent/ext-tabGroups.js
View File

@@ -11,6 +11,42 @@ var { ExtensionError } = ExtensionUtils;
const spellColour = color => (color === "grey" ? "gray" : color);
/**
* @param {MozTabbrowserTabGroup} group Group to move.
* @param {DOMWindow} window Browser window to move to.
* @param {integer} index The desired position of the group within the window
* @returns {integer} The tab index that the group should move to, such that
* after the move operation, the group's position is at the given index.
*/
function adjustIndexForMove(group, window, index) {
let tabIndex = index < 0 ? window.gBrowser.tabs.length : index;
if (group.ownerGlobal === window) {
let group_tabs = group.tabs;
if (tabIndex > group_tabs[0]._tPos) {
// When group is moving to a higher index, we need to increase the
// index to account for the fact that the act of moving tab groups
// causes all following tabs to have a decreased index.
tabIndex += group_tabs.length;
}
}
tabIndex = Math.min(tabIndex, window.gBrowser.tabs.length);
let prevTab = tabIndex > 0 ? window.gBrowser.tabs.at(tabIndex - 1) : null;
let nextTab = window.gBrowser.tabs.at(tabIndex);
if (nextTab?.pinned) {
throw new ExtensionError(
"Cannot move the group to an index that is in the middle of pinned tabs."
);
}
if (prevTab && nextTab?.group && prevTab.group === nextTab.group) {
throw new ExtensionError(
"Cannot move the group to an index that is in the middle of another group."
);
}
return tabIndex;
}
this.tabGroups = class extends ExtensionAPIPersistent {
queryGroups({ collapsed, color, title, windowId } = {}) {
color = spellColour(color);
@@ -197,10 +233,9 @@ this.tabGroups = class extends ExtensionAPIPersistent {
let last = win.gBrowser.tabContainer.ariaFocusableItems.length + 1;
let elementIndex = index === -1 ? last : Math.min(index, last);
group = win.gBrowser.adoptTabGroup(group, elementIndex);
} else if (index >= 0 && index < win.gBrowser.tabs.length) {
win.gBrowser.moveTabTo(group, { tabIndex: index });
} else if (win.gBrowser.tabs.at(-1) !== group.tabs.at(-1)) {
win.gBrowser.moveTabAfter(group, win.gBrowser.tabs.at(-1));
} else {
let tabIndex = adjustIndexForMove(group, win, index);
win.gBrowser.moveTabTo(group, { tabIndex });
}
return this.convert(group);
},

2
browser/components/extensions/test/browser/browser.toml
View File

@@ -483,6 +483,8 @@ https_first_disabled = true
["browser_ext_tabGroups.js"]
["browser_ext_tabGroups_move.js"]
["browser_ext_tabGroups_move_event_order.js"]
["browser_ext_tabGroups_move_onMoved.js"]

343
browser/components/extensions/test/browser/browser_ext_tabGroups_move.js Normal file
View File

@@ -0,0 +1,343 @@
/* Any copyright is dedicated to the Public Domain.
* http://creativecommons.org/publicdomain/zero/1.0/ */
"use strict";
add_task(async function test_tabGroups_move_index() {
let extension = ExtensionTestUtils.loadExtension({
manifest: {
permissions: ["tabGroups"],
},
async background() {
const firstTab = await browser.tabs.create({ url: "about:blank#0" });
// In this test we are going to reuse many tabs; to make tests easier to
// understand, the tests are going to reference tabs by their initial
// index in the tab. This array maps the tabs at the "initial index" to
// the actual tab ID.
let reusableTabIds = [firstTab.id];
// We are also going to reuse the window. When there are more tabs than
// we need, we'll put the excess tabs in the initial window.
const nonTestWindowId = firstTab.windowId;
const testWindow = await browser.windows.create({ tabId: firstTab.id });
const windowId = testWindow.id; // Used in the whole test!
let numberOfTabsInTestWindow = 1;
async function setTabCountInTestWindow(tabCount) {
await browser.tabs.ungroup(
reusableTabIds.slice(0, numberOfTabsInTestWindow)
);
for (let tabId of reusableTabIds) {
await browser.tabs.update(tabId, { pinned: false });
}
for (let i = 0; i < tabCount; ++i) {
if (i in reusableTabIds) {
await browser.tabs.move(reusableTabIds[i], { windowId, index: i });
} else {
const tab = await browser.tabs.create({
url: `about:blank#${i}`,
windowId,
index: i,
});
reusableTabIds[i] = tab.id;
}
}
// Move excess tabs to another window.
for (let i = tabCount; i < numberOfTabsInTestWindow; ++i) {
await browser.tabs.move(reusableTabIds[i], {
windowId: nonTestWindowId,
index: -1,
});
}
numberOfTabsInTestWindow = tabCount;
}
// Test the behavior of tabGroups.move, for a given initial tab strip,
// in the format [0, 1, 2, [3, 4, 5], 6, etc], where the numbers reflect
// the tabs (by their initial index), and the nested array marks a group.
async function testTabGroupMove(testCase) {
browser.test.log(`testTabGroupMove: ${JSON.stringify(testCase)}`);
const DUMMY_TABGROUPS_MOVE_NO_ERROR = "(tabGroups.move succeeded)";
const {
description,
starting_tabstrip,
pinned_count = 0,
group_at_tab,
index,
expected_tabstrip = starting_tabstrip,
expected_error = DUMMY_TABGROUPS_MOVE_NO_ERROR,
} = testCase;
let allTabIndexes = starting_tabstrip.flatMap(num => num);
// Sanity check: each number reflects the initial index.
if (allTabIndexes.some((num, i) => num !== i)) {
browser.test.fail(`Bad starting_tabstrip: ${starting_tabstrip}`);
}
// Setup: Prepare window as specified by starting_tabstrip, with a
// group for each array.
await setTabCountInTestWindow(allTabIndexes.length);
const groupIdAtIndex = new Map();
for (let v of starting_tabstrip) {
if (Array.isArray(v)) {
const groupId = await browser.tabs.group({
createProperties: { windowId },
tabIds: v.map(i => reusableTabIds[i]),
});
groupIdAtIndex.set(v[0], groupId);
}
}
for (let i = 0; i < pinned_count; ++i) {
let tabId = reusableTabIds[i];
await browser.tabs.update(tabId, { pinned: true });
}
if (!groupIdAtIndex.has(group_at_tab)) {
// Sanity check: group starts at the given index.
browser.test.fail("Bad group_at_tab: no group found at index");
}
const groupId = groupIdAtIndex.get(group_at_tab);
let actualError;
try {
await browser.tabGroups.move(groupId, { index });
actualError = DUMMY_TABGROUPS_MOVE_NO_ERROR;
} catch (e) {
actualError = e.message;
}
browser.test.assertEq(
expected_error,
actualError,
`expected_error matches - ${description}`
);
const actualTabstrip = [];
let lastGroupId;
for (const tab of await browser.tabs.query({ windowId })) {
let initialTabStripIndex = reusableTabIds.indexOf(tab.id);
if (tab.groupId == -1) {
actualTabstrip.push(initialTabStripIndex);
} else if (lastGroupId == tab.groupId) {
actualTabstrip.at(-1).push(initialTabStripIndex);
} else {
actualTabstrip.push([initialTabStripIndex]);
}
lastGroupId = tab.groupId;
}
browser.test.assertDeepEq(
expected_tabstrip,
actualTabstrip,
`Got expected tabstrip after move - ${description}`
);
}
browser.test.log("Initial test window created - running tests now.");
const ERROR_MOVE_IN_GROUP =
"Cannot move the group to an index that is in the middle of another group.";
const ERROR_MOVE_TO_PINNED =
"Cannot move the group to an index that is in the middle of pinned tabs.";
await testTabGroupMove({
description: "Move group that is alone in the window to same position",
starting_tabstrip: [[0]],
group_at_tab: 0,
index: 0,
expected_tabstrip: [[0]],
});
await testTabGroupMove({
description: "Move group that is alone in the window to last position",
starting_tabstrip: [[0]],
group_at_tab: 0,
index: -1,
expected_tabstrip: [[0]],
});
await testTabGroupMove({
description: "Move group to first position",
starting_tabstrip: [0, [1, 2, 3], 4],
group_at_tab: 1,
index: 0,
expected_tabstrip: [[1, 2, 3], 0, 4],
});
await testTabGroupMove({
description: "Move group to last position",
starting_tabstrip: [0, [1, 2, 3], 4],
group_at_tab: 1,
index: -1,
expected_tabstrip: [0, 4, [1, 2, 3]],
});
await testTabGroupMove({
description: "Move group to first position inside original group",
starting_tabstrip: [0, 1, [2, 3, 4, 5], 6, 7, 8, 9],
group_at_tab: 2,
index: 3,
expected_tabstrip: [0, 1, 6, [2, 3, 4, 5], 7, 8, 9],
});
await testTabGroupMove({
description: "Move group to last position inside original group",
starting_tabstrip: [0, 1, [2, 3, 4, 5], 6, 7, 8, 9],
group_at_tab: 2,
index: 5,
expected_tabstrip: [0, 1, 6, 7, 8, [2, 3, 4, 5], 9],
});
await testTabGroupMove({
description: "Move group to position of right neighbor",
starting_tabstrip: [0, 1, [2, 3, 4, 5], 6, 7, 8, 9],
group_at_tab: 2,
index: 6,
expected_tabstrip: [0, 1, 6, 7, 8, 9, [2, 3, 4, 5]],
});
await testTabGroupMove({
description: "Move group to position past right neighbor (clamped)",
starting_tabstrip: [0, 1, [2, 3, 4, 5], 6, 7, 8, 9],
group_at_tab: 2,
index: 7,
expected_tabstrip: [0, 1, 6, 7, 8, 9, [2, 3, 4, 5]],
// ^
// This is actually index 7.
// But we cannot leave a gap before the group,
// so the group ends up at index 6 instead.
});
await testTabGroupMove({
description: "Move group to position past end of tab strip (clamped)",
starting_tabstrip: [0, 1, [2, 3, 4, 5], 6, 7, 8, 9],
group_at_tab: 2,
index: 10,
expected_tabstrip: [0, 1, 6, 7, 8, 9, [2, 3, 4, 5]],
});
// Splits ////////////////////////////////////////////////////////////////
// Moving a group between other groups
await testTabGroupMove({
description: "Move group (size 1) left between 1-length groups",
starting_tabstrip: [0, [1], [2], [3, 4, 5], 6, 7, [8], 9],
group_at_tab: 8,
index: 2,
expected_tabstrip: [0, [1], [8], [2], [3, 4, 5], 6, 7, 9],
});
await testTabGroupMove({
description: "Move group (size 1) right between 1-length groups",
starting_tabstrip: [0, [1], [2], [3, 4, 5], [6], [7], 8, [9]],
group_at_tab: 1,
index: 6,
expected_tabstrip: [0, [2], [3, 4, 5], [6], [1], [7], 8, [9]],
});
await testTabGroupMove({
description: "Move group (size 1) left before a length-1 group",
starting_tabstrip: [0, [1], [2], [3, 4, 5], 6, 7, [8], 9],
group_at_tab: 8,
index: 1,
expected_tabstrip: [0, [8], [1], [2], [3, 4, 5], 6, 7, 9],
});
await testTabGroupMove({
description: "Move group (size 4) left between other groups",
starting_tabstrip: [[0], [1, 2], [3, 4, 5], [6, 7, 8, 9]],
group_at_tab: 6,
index: 1,
expected_tabstrip: [[0], [6, 7, 8, 9], [1, 2], [3, 4, 5]],
});
await testTabGroupMove({
description: "Move group (size 2) right between other groups",
starting_tabstrip: [[0], [1, 2], [3, 4, 5], [6, 7, 8, 9]],
group_at_tab: 1,
index: 4,
expected_tabstrip: [[0], [3, 4, 5], [1, 2], [6, 7, 8, 9]],
});
// Intersections /////////////////////////////////////////////////////////
// Moving tab groups into other tab groups
await testTabGroupMove({
description: "Move group (size 1) right inside another group (size 3)",
starting_tabstrip: [0, 1, [2], [3, 4, 5], 6, 7, 8, 9],
group_at_tab: 2,
index: 3,
expected_error: ERROR_MOVE_IN_GROUP,
});
await testTabGroupMove({
description: "Move group (size 1) left inside another group (size 3)",
starting_tabstrip: [0, 1, [2, 3, 4], 5, 6, [7], 8, 9],
group_at_tab: 7,
index: 3,
expected_error: ERROR_MOVE_IN_GROUP,
});
await testTabGroupMove({
description: "Move group (size 2) right inside another group (size 5)",
starting_tabstrip: [0, [1, 2], 3, [4, 5, 6, 7, 8], 9],
group_at_tab: 1,
index: 6,
expected_error: ERROR_MOVE_IN_GROUP,
});
await testTabGroupMove({
description: "Move group (size 5) left inside another group (size 3)",
starting_tabstrip: [[0, 1, 2], 3, [4, 5, 6, 7, 8], 9],
group_at_tab: 4,
index: 1,
expected_error: ERROR_MOVE_IN_GROUP,
});
// Pinned tabs ///////////////////////////////////////////////////////////
await testTabGroupMove({
description: "Move group before pinned tab",
starting_tabstrip: [0, 1, [2], 3],
pinned_count: 2,
group_at_tab: 2,
index: 0,
expected_error: ERROR_MOVE_TO_PINNED,
});
await testTabGroupMove({
description: "Move group between pinned tabs",
starting_tabstrip: [0, 1, [2], 3],
pinned_count: 2,
group_at_tab: 2,
index: 1,
expected_error: ERROR_MOVE_TO_PINNED,
});
await testTabGroupMove({
description: "Move group after pinned tab (position not changed)",
starting_tabstrip: [0, 1, [2], 3],
pinned_count: 2,
group_at_tab: 2,
index: 2,
expected_tabstrip: [0, 1, [2], 3],
});
await testTabGroupMove({
description: "Move group after pinned tab",
starting_tabstrip: [0, 1, 2, [3]],
pinned_count: 2,
group_at_tab: 3,
index: 2,
expected_tabstrip: [0, 1, [3], 2],
});
browser.test.log("Tests done - cleaning up.");
for (let tabId of reusableTabIds) {
// This also closes testWindow.
await browser.tabs.remove(tabId);
}
browser.test.sendMessage("done");
},
});
await extension.startup();
await extension.awaitMessage("done");
await extension.unload();
});

6
browser/components/extensions/test/browser/browser_ext_tabGroups_move_onMoved.js
View File

@@ -90,16 +90,12 @@ add_task(async function tabsGroups_move_onMoved() {
await ext.awaitMessage("done");
is(group.tabs[0], gBrowser.tabs[0], "Using same index 0 doesn't move.");
info("Create a large second group, and try to move in the middle of it.");
info("Create a large second group, and remove tabs in order.");
let group4 = gBrowser.addTabGroup(gBrowser.tabs.slice(5));
let created4 = await ext.awaitMessage("created");
let gid4 = getExtTabGroupIdForInternalTabGroupId(group4.id);
is(created4.id, gid4, "Correct group 4 created event.");
ext.sendMessage(gid, { index: 8 });
await Promise.all([ext.awaitMessage("done"), ext.awaitMessage("moved")]);
is(group.tabs[1], gBrowser.tabs[4], "Moved before the whole group instead.");
for (let tab of tabs) {
BrowserTestUtils.removeTab(tab);
}

2
browser/components/tabbrowser/content/tabbrowser.js
View File

@@ -5998,7 +5998,7 @@
element,
() => {
let neighbor = this.tabs[tabIndex];
if (forceUngrouped && neighbor.group) {
if (forceUngrouped && neighbor?.group) {
neighbor = neighbor.group;
}
if (neighbor && this.isTab(element) && tabIndex > element._tPos) {