Closed Bug 2016762 Opened 2 months ago Closed 1 month ago

tabs.move() cannot swap two tabs that are part of a split view

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox-esr140 unaffected, firefox147 unaffected, firefox148 unaffected, firefox149 wontfix, firefox150 fixed)

RESOLVED FIXED
150 Branch
Tracking Status
firefox-esr140 --- unaffected
firefox147 --- unaffected
firefox148 --- unaffected
firefox149 --- wontfix
firefox150 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: dev-doc-complete, regression, Whiteboard: [addons-jira])

Attachments

(1 file)

Bug 2004383 changes the logic for moving tabs such that tabs within a split view cannot be moved individually, only as one unit.

That makes a lot of sense for the browser UI, where tabs in a split view can only be moved as one unit.

In the extension API, tabs can be moved individually. The current behavior is as follows:

  • When a tab is moved out of a split view, the split view closes.
  • When a tab (not a member of a split view) is moved into a split view, it is placed outside the split view (bug 2016751).
  • When a tab is moved within a split view, it does currently not move.

The last behavior is the subject of this bug. it should be possible to swap tabs within a split view.

(I also checked the behavior in Chrome, extensions in Chrome can swap tabs in a split view)

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

This does not only affect tabs within the split, but also prevents attempts to move a tab/split view after tab at the right of the split.

Because isTab(element) is false when the element is a split view, the tabbrowser's moveTabTo implementation always prepends the element before the destination tab instead of after it.

From my recollection, moveTabTo was originally supposed to place a tab into a position as specified by the given index. With git blame, I was indeed able to trace the code back to a state before bug 1927540, which includes a comment about intended behavior:

        if (aIndex < aTab._tPos) {
          neighbor.before(aTab);
        } else if (!neighbor) {
          // Put the tab after the neighbor, as once we remove the tab from its current position,
          // the indexing of the tabs will shift.
          aTab.parentElement.append(aTab);
        } else {
          neighbor.after(aTab);

This showed that moveTabTo is meant to make the modification such that the tab's new position is at index aIndex. This semantic is relied upon by the extension's tabs.move() extension API implementation.

Now, with the changes in bug 2004383, were meant to preserve the unity of a tab split as much as possible. That is a (sensible) product decision, but does not imply that one cannot swap tabs within a split, let alone prevent a split view from being moved to the right.

I've already authored unit tests (which is how I discovered this additional bug) before starting a fix.

Depends on: 2016754

The "Move to End" functionality is also broken, regressed by the same change, caused by the same implementation flaw as explained in comment 2.

STR:

  1. Right-click on a tab split, choose "Move Tab" > "Move to End"
  2. Look at where it ends up.

Expected:

  • Tab split should move to end

Actual:

  • Tab split is at the penultimate location.

The move to end functionality is being addressed in bug 2010201, which already has a patch in review.

Flags: needinfo?(rob)
See Also: → 2010201

The "unable to swap" behavior is due to the logic before the #handleTabMove part, at https://searchfox.org/firefox-main/rev/2477384eae35d0ee85d43dfdeba6f2ba51d9abfb/browser/components/tabbrowser/content/tabbrowser.js#6824-6825

Bug 2010201 fixes the issue of comment 2 and comment 3.

The unit tests (with TODOs) in bug 2016754 cover the initially reported bug here, but not the issues I mentioned in comment 2 and comment 3, so the behavior of these tests do not change even if I apply the patch from bug 2010201.

Flags: needinfo?(rob)

Handing bug over to Sarah since this change is mostly in tabbrowser code, and the lot of extension-specific test coverage has already been written (in bug 2016754), so all that needs to happen here is to fix the bug and adjust the test expectations marked as TODO bug 2016762.

Assignee: rob → sclements
Severity: -- → S3
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [addons-jira] → [addons-jira], [fidefe-splitview]

Moving this back to Rob since after looking at this issue, the solution is for extensions code to check whether the tab that wants to move and its targetIndex share the same splitview id, then it'd need to call tab.splitview.reverseTabs(). The reason being is that more needs to happen beyond just swapping tabs within a splitview; the splitview wrapper itself needs updating of its children and reactivating of the panels.

Assignee: sclements → rob
Component: Tabbed Browser: Split View → General
Product: Firefox → WebExtensions
Whiteboard: [addons-jira], [fidefe-splitview] → [addons-jira]

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

Is a fix planned for the initial release of split view?

Flags: needinfo?(rob)

Yes. This behavior is very visible to extensions so I want to submit a patch and uplift it to beta.

Flags: needinfo?(rob)

The bug is marked as tracked for firefox149 (beta). However, the bug still has low severity.

:mixedpuppy, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Flags: needinfo?(mixedpuppy)

Rob, any news on the patch?

Flags: needinfo?(rob)

Currently working on a patch.

The desired behaviors are described at https://bugzilla.mozilla.org/show_bug.cgi?id=2016751#c2.

Flags: needinfo?(rob)
Flags: needinfo?(mixedpuppy)
Depends on: 2021527

And fix an issue where splitview.reverseTabs() caused TabMove to fire
too often. This is because #handleTabMove dispatches TabMove events
for all tabs in a split view (due to the patch to bug 2004383), even
when only one tab has moved. Due to this dispatch of too many events,
any observer that aims to reconstruct the tab strip could observe
non-sensical event sequences such as:

  • Move tabA from index 2 to index 1.
  • Move tabB from index 1 to index 2.

After observing tabA moving to index 1, tabB cannot be moving from tab 1
because tabA is there!
This patch fixes the issue by firing only one event in this scenario.

We are not going to uplift this patch. The change will ride the 150 train.

In the meantime extensions cannot rearrange tabs in a split (in 149), but we consider that to just be a bug that is in the 149 release, fixed in 150.

Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 150 Branch
Keywords: dev-doc-needed
See Also: → 2028832
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: