Closed Bug 1963825 Opened 1 year ago Closed 1 year ago

tabGroups.move() requires index different to Google Chrome

Categories

(WebExtensions :: Compatibility, defect, P1)

defect

Tracking

(firefox139+ fixed, firefox140 fixed)

RESOLVED FIXED
140 Branch
Tracking Status
firefox139 + fixed
firefox140 --- fixed

People

(Reporter: yuki, Assigned: robwu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [addons-jira])

Attachments

(4 files)

The method tabGroups.move() accepts index via the object given as the second argument, but we need to specify an index different from Chromium.

Steps to reproduce

Load the attached testcase as a temporary extension.
It prepares four tabs with two groups like:

  • group0
    • about:blank#0
    • about:blank#1
  • group1
    • about:blank#2
    • about:blank#3

and tries to move the group0 after group1 with the code: chrome.tabGroups.move(group0Id, { index: 2 }).

Expected result

On Chrome this works as expected and rearrange groups as:

  • group1
    • about:blank#2
    • about:blank#3
  • group0
    • about:blank#0
    • about:blank#1

Actual result

On Nightly 140.0a1 nothing changed.
We need to run chrome.tabGroups.move(group0Id, { index: 4 }) to rearrange groups like on Chrome.

Environment

I've confirmed this incompatibility with:

  • Nightly 140.0a1 Build ID: 20250430092515
  • Google Chrome 135.0.7049.115

I've tried more with two groups on Chrome:

  • group0
    • tab0
    • tab1
  • group1
    • tab2
    • tab3
    • tab4

To move the group0 next to the group1, I had to run chrome.tabGroups.move(group0, { index: 3 }) on Chrome. index:2 reports an error Cannot move the group to an index that is in the middle of another group. Thus the index parameter looks to mean the final index of the first tab of the moved group.

On the other hand, Firefox requires chrome.tabGroups.move(group0, { index: 5 }) on this case. The required index looks odd, because I just have to run chrome.tabs.move(tab0, { index: 4 }) when I hope to move the tab0 after the tab4. I can't understand why I have to specify index: 5 for tab groups...

One more: Firefox does not report error when unavailable index is given to tabGroups.move(). This is also an incompatibility but I think it is not critical.

I've understand that Chrome compatibility does not have highest priority on Firefox's WebExtensions API as described at the bug 1965057, and indeed some differences of APIs like tabGroups.onCreated/onRemoved/onMoved between Firefox and Chrome looks reasonable for extension authors, but I think the difference described here looks unreasonable. Until this bug is fixed, we extension authors look to need to do to move tab groups safely:

  1. Call tabGoups.get() to get the label and color of the group.
  2. Call tabs.ungroup() for the group member tabs.
  3. Move member tabs to the destination location.
  4. Call tabs.group() for the member tabs.

Chrome treats index as the desired index after moving., after the group is moved. This requires extensions to know where the tabs within the group came from.

Firefox treats index as the current position before which the tab group should be inserted. The idea behind this is that if an extension queries a tab, and wants to put a group before that tab, then they can just specify its tab.index as the index:

             index 0 - before group0
group0
  tab0
             index 1 - inside group0
  tab1
             index 2 - after group0 / before group1
group1
  tab2
             index 3 - inside group1
  tab3
             index 4 - inside group1
  tab4
             index 5 - after group1 (no tab at index 5, but need value for "after last tab", is -1 enough?)

Given this, it is clear why Firefox does not change anything when you request a move to index 2.

Chrome's interpretation is undocumented but the source shines some light on it: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/api/tab_groups/tab_groups_api.cc;l=340-361;drc=c4d16e4480885ba044064093bc1b83e15136b9fa

  • The intended outcome according to the source comment is "When moving to the right, adjust the target index for the size of the group, since the group itself may occupy several indices to the right."
    • The actual implementation: takes the given index if before the tab group, otherwise adds the number of tabs in the group to the index. The resulting index is validated: error if past end of the tab strip or if position is in another group.
    • Given tabs [0g, 1g, 2, 3, 4, 5], with tabGroups.move(g, { index: 3}):
      • Firefox's result is [2, 0g, 1g, 3, 4, 5] (moved to tab formerly at index 3). Chrome's output is [2, 3, 4, 0g, 1g, 5] (moved to place that is now index 3).
  • In Firefox and Chrome, moving a tab group to a different window has the same meaning: index is the current index in the new window (and simultaneously also the index of the group after moving). Interestingly, Chrome does not raise an error when the index is out of range in this case, unlike the same-window case, where it does.

(In reply to YUKI "Piro" Hiroshi from comment #5)

I've understand that Chrome compatibility does not have highest priority on Firefox's WebExtensions API as described at the bug 1965057

We do consider compatibility seriously. We don't do bug-for-bug compatibility though. I think that you meant to link to bug 1962475; Chrome's extension API currently follows their internal implementation closely, but they are willing to consider alternatives as expressed in https://issues.chromium.org/issues/414993460#comment8

Given the above explanation (comment 6), which behavior seems more correct to you?

Blocks: 1940631
Flags: needinfo?(yuki)

For comparison, the tabs.move() API also allows bulk-move of tabs. In Firefox, the given index specifies the insertion point for the first tab of the list of tabs, and all other tabs are put after it. With this logic, it is possible for the tabs to end up at a lower position than given by index, if the other tabs to move shift.

Chrome's tabs.move() bulk-moves tabs with similar semantics for the first tab. The following tabs are placed after it (but in a buggy order: https://issues.chromium.org/issues/336984026).

I'm going to work on a patch to change the semantics of "index" in tabGroups.move to match Chrome's: index should be the desired position after moving tabs.

This decision is based on the following:

  • We have not shipped tabGroups.move to release yet, it is currently only on Beta (139). Now would be the best time to make breaking changes.
  • Chrome's tabGroups API has been around for a few years, and shipping different behavior makes it more difficult to maintain a cross-browser compatible extension.
  • This report (and https://issues.chromium.org/issues/336984026) and a quick internal poll shows that developers may expect index to be the desired position, rather than the initial position (before move).
Assignee: nobody → rob
Status: NEW → ASSIGNED
Flags: needinfo?(yuki)
Priority: -- → P1
Whiteboard: [addons-jira]

[Tracking Requested - why for this release]: tabGroups extension API was added in Firefox 139 (currently beta). This report shows a significant incompatibility in tabGroups.move (added in bug 1961660) with Chrome, which we plan to address in the next few days.

Depends on: 1961660

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

Sorry for my delay, I've read the description of current API design on Firefox and I've understood why I had to specify the index 5 on the case.

Hmm, the chaos looks to came from the design decision on Chrome keeping compatibility of tabs API - tab groups have no index information and only tabs have, but moving of a tab group requires the destination position. Anyway, it is good news that the API will become compatible to existing implementation on Chrome. Thank you for reconsideration!

Blocks: 1966617

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

Original Revision: https://phabricator.services.mozilla.com/D249493

Attachment #9488191 - Flags: approval-mozilla-beta?

firefox-beta Uplift Approval Request

  • User impact if declined: tabGroups extension API was added in Firefox 139 (currently beta). This report shows a significant incompatibility in tabGroups.move (added in bug 1961660) with Chrome, which this patch resolves.
  • Code covered by automated testing: yes
  • Fix verified in Nightly: no
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: Not needed; API change covered by automated unit tests.
  • Risk associated with taking this patch: Low
  • Explanation of risk level: Change to extension API only (which landed in 139), not used anywhere else. Covered by unit tests
  • String changes made/needed: No
  • Is Android affected?: no
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/7f93b43a5a46 tabGroups.move index = desired final index r=zombie,tabbrowser-reviewers,dao
Regressions: 1966823
Flags: in-testsuite+
Attachment #9488191 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 140 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: