browser.tabs.move() - unexpected behavior of resulting tab placement position when moving several tabs preceding move-to index

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: Kevin Jones, Assigned: andym)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [tabs] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
`browser.tabs.move()` can accept an array of tab ids for the first argument.

I have observed the following:
(Tests done on FF 53, OS X)

Move tabs by passing an array of `tab.id`s which correlate with tabs at indexes 2, 3, 4, 5, to index 12 (all tabs to be moved preceding the move-to index).

Tabs move to positions 9, 11, 13, 15 (with the tab at index 12 being moved to index 8, as a result of the fact that the moved tabs which once preceded, now succeed.)

Move tabs by passing an array of `tab.id`s which correlate with tabs at indexes 2, 4, 6, 8, to index 12 (all tabs to be moved preceding the move-to index).

Tabs move to positions 9, 11, 13, 15 (with the tab at index 12 being moved to index 8, as a result of the fact that the moved tabs which once preceded, now succeed.)

Move tabs by passing an array of `tab.id`s which correlate with tabs at indexes 2, 4, 12, 14, to index 10 (2 tabs to be moved preceding, 2 succeeding the move-to index).

Tabs move to positions 9, 11, 12, 13 (with the tab at index 10 being moved to index 8, as a result of the fact that the moved tabs which once preceded, now succeed.)

1)  There doesn't seem to be a consistent pattern as to which positions the tabs will end up in after a move, other than that the first tab in the array of tabs moved will directly succeed the move-to index (argument[1] property).

2)  The desired result would be that all moved tabs would directly succeed the tab which was originally at the move-to index, and the moved tabs would end up consecutive with each other, and precession/succession orientation relative to the other moved tabs would remain the same.  In other words:

Move tabs by passing an array of `tab.id`s which correlate with tabs at indexes 2, 3, 4, 5, to index 12 (all tabs to be moved preceding the move-to index).

Tabs move to positions 9, 10, 11, 12 (with the tab at index 12 being moved to index 8, as a result of the fact that the moved tabs which once preceded, now succeed.)

Move tabs by passing an array of `tab.id`s which correlate with tabs at indexes 2, 4, 6, 8, to index 12 (all tabs to be moved preceding the move-to index).

Tabs move to positions 9, 10, 11, 12 (with the tab at index 12 being moved to index 8, as a result of the fact that the moved tabs which once preceded, now succeed.)

Move tabs by passing an array of `tab.id`s which correlate with tabs at indexes 2, 4, 12, 14, to index 10 (2 tabs to be moved preceding, 2 succeeding the move-to index).

Tabs move to positions 9, 10, 11, 12 (with the tab at index 10 being moved to index 8, as a result of the fact that the moved tabs which once preceded, now succeed.)

What is the intended behavior regarding resultant positions after tabs have been moved, if moved tabs originally preceded the move-to index?

In the case of moving tabs which succeed the move-to index, there doesn't seem to be an issue, as the desired result is as stated before, that all moved tabs would directly succeed the tab which was originally at the move-to index, and the moved tabs would end up consecutive with each other, and precession/succession orientation relative to the other moved tabs would remain the same.
(Reporter)

Comment 1

6 months ago
A workaround for this that seems consistent:

(When moving tabs which precede the move-to index)

1)  Move all tabs to the end of the list
2)  Get the new position of the tab which was originally at the position we wanted
    to move the tabs to (since it has now moved to a new position).
3)  Move same tabs to this position.

  tabid == id of the tab at the index we want to move tabs to, before beginning any moves:

  browser.tabs.move(ids, { index : -1 }).then(function() {
    browser.tabs.get(tabid).then(function(tab) {
      browser.tabs.move(ids, { index : tab.index });
    });
  });

Updated

6 months ago
Assignee: nobody → amckay
Priority: -- → P1
Whiteboard: [tabs] triaged
(Assignee)

Comment 2

6 months ago
I think this because the insertionPoint blindly always adds +1 to the index of the tab. But that might not always be true. If the tab being moved is before the insertion point, we don't have to increment the insertion point because all the tabs "move up" by one.

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#917
(Reporter)

Updated

6 months ago
Summary: browser.tabs.move() - unpredictable behavior of resulting tab placement position when moving several tabs preceding move-to index → browser.tabs.move() - unexpected behavior of resulting tab placement position when moving several tabs preceding move-to index
(Assignee)

Comment 3

6 months ago
I get the exact same behaviour in Chrome as comment 1.
(Assignee)

Comment 4

6 months ago
Created attachment 8820927 [details] [diff] [review]
tabs.move.fix.patch

Dropping this down in priority significantly since Chrome does this as well and the question as to what is correct is slightly harder. I may have gotten a bit complicated with this and the addition of the `lastInsertion` Map isn't great.

I wouldn't mind some feedback on the tests though in line 45+. Hopefully those cases make sense and are what we are all expecting from the move. Also, any test cases I'm missing?
Attachment #8820927 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 5

6 months ago
Given that the same behaviour occurs in Chrome, dropping the priority down.
Priority: P1 → P3

Comment 6

5 months ago
Comment on attachment 8820927 [details] [diff] [review]
tabs.move.fix.patch

Review of attachment 8820927 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-tabs.js
@@ +920,5 @@
> +          // then you need to bump up the insertion point.
> +          if (lastInsertion.has(window) &&
> +            lastInsertion.get(window) === insertionPoint &&
> +            tab._tPos > insertionPoint) {
> +            indexMap.set(window, insertionPoint += 1);

Please move the side-effect to its own line.
Attachment #8820927 - Flags: feedback?(kmaglione+bmo) → feedback+
(Assignee)

Comment 7

5 months ago
Does the ordering of tabs as a result of this patch make sense to you Kevin?
Flags: needinfo?(kevinhowjones)
(Assignee)

Comment 8

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=507935adfec8
(Reporter)

Comment 9

5 months ago
(In reply to Andy McKay [:andym] from comment #7)
> Does the ordering of tabs as a result of this patch make sense to you Kevin?

It is much better, but there is a little glitch.  When moving tabs from the left of the insertion point, they are kept together now, however, they start one tab to the right of where I would expect, and inconsistent with a move from from the right:

** Move tabs from the right:

Starting tabs: 0 1 2 3 4 5 6 7 8 9

move tabs 6, 7 to index 5 (target "between" tabs 4 and 5):

result:  0 1 2 3 4 [6 7] 5 8 9   -expected behavior (tabs between 4 and 5)

** Move tabs from the left:

Starting tabs 0 1 2 3 4 5 6 7 8 9

move tabs 2, 3 to index 5 (target "between" tabs 4 and 5):

result:  0 1 4 5 [2 3] 6 7 8 9   -one tab to the right of expected (tabs between 5 and 6)

expected behavior: 0 1 4 [2 3] 5 6 7 8 9     (tabs between 4 and 5)

**

This is due to moveTabTo behavior, which moves the tab in such a way as its position after the move will be at the move-to index, rather than the target position.  Of course since a tab moved from the left will shift all tabs after it one index to the left, the target tab changes.  This paradigm may make some sense for a single tab move, in the fact that the tab after the move in both cases will be at the same _tPos.  However, with multi-tab moves, that paradigm falls apart.  This especially reveals itself with moves that contain both tabs from the left and from the right of the insertion point.

I think the paradigm that we are working on here is that we are moving tabs relative to another tab, and the insertion point is just before that tab, rather than an arbitrary final position.  (Actually the reason for this bug in the first place.)

If you add this to your patch, it takes care of the problem:

         let indexMap = new Map();
         let lastInsertion = new Map();

         let tabs = tabIds.map(tabId => TabManager.getTab(tabId, context));
+
+        // If first tab to be moved is on the left of the index, decrement index.
+        if (tabs[0]._tPos < moveProperties.index) {
+          moveProperties.index--;
+        }
+
         for (let tab of tabs) {
           // If the window is not specified, use the window from the tab.
           let window = destinationWindow || tab.ownerGlobal;
           let gBrowser = window.gBrowser;
Flags: needinfo?(kevinhowjones)
(Reporter)

Updated

5 months ago
Flags: needinfo?(amckay)
(Assignee)

Comment 10

4 months ago
There's two different issues there. 

One is that when you move more than one tab they should insert themselves after the initial point, so you should have 4 [2 3] 5 and not 4 [2] 5 [3]. That makes sense to me and though Chrome also has this problem, feel like this is an edge case to fix in Firefox, although part of me is leaning to wont fix.

The second is that the insertion point for one moveTo produces different results for Chrome and Firefox. This behaviour is consistent on both Chrome and Firefox:

If you have: 0 1 2 3 4 5 6 7 8 9
* then move tab 0 to index 5, it will be after 5.
* then move tab 9 to index 5, it will be after 4.

Since this behaviour is consistent across both browsers. If we change the initial insertion of tabs, we'll break compatibility between Chrome and Firefox for the approx. 298+ Chrome extensions that use this API and that doesn't seem worth it.
Flags: needinfo?(amckay)
(Assignee)

Comment 11

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eed4ae220d33
(Assignee)

Comment 12

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b58efc5730f
(Reporter)

Comment 13

4 months ago
(In reply to Andy McKay [:andym] from comment #10)
> There's two different issues there. 
> 
> One is that when you move more than one tab they should insert themselves
> after the initial point, so you should have 4 [2 3] 5 and not 4 [2] 5 [3].
> That makes sense to me and though Chrome also has this problem, feel like
> this is an edge case to fix in Firefox, although part of me is leaning to
> wont fix.
> 
> The second is that the insertion point for one moveTo produces different
> results for Chrome and Firefox. This behaviour is consistent on both Chrome
> and Firefox:
> 
> If you have: 0 1 2 3 4 5 6 7 8 9
> * then move tab 0 to index 5, it will be after 5.
> * then move tab 9 to index 5, it will be after 4.
> 
> Since this behaviour is consistent across both browsers. If we change the
> initial insertion of tabs, we'll break compatibility between Chrome and
> Firefox for the approx. 298+ Chrome extensions that use this API and that
> doesn't seem worth it.

Not a problem, dicing up the tabs after a multi-tab move was unacceptable, but what we have now is workable without a lot of effort with some anticipatory action from the caller.
(Reporter)

Comment 14

4 months ago
> Not a problem, dicing up the tabs after a multi-tab move was unacceptable,
> but what we have now is workable without a lot of effort with some
> anticipatory action from the caller.

er, "what we have now" meaning what we'll have as a result of your patch.
Comment hidden (mozreview-request)

Comment 16

4 months ago
mozreview-review
Comment on attachment 8839956 [details]
bug 1323311 make tabs.move on multiple tabs more reliable

https://reviewboard.mozilla.org/r/114532/#review118894

::: browser/components/extensions/ext-tabs.js:922
(Diff revision 1)
> +            lastInsertion.get(window) === insertionPoint &&
> +            tab._tPos > insertionPoint) {

Please add 2 spaces to indentation.

::: browser/components/extensions/ext-tabs.js:925
(Diff revision 1)
> +          // then you need to bump up the insertion point. See bug 1323311.
> +          if (lastInsertion.has(window) &&
> +            lastInsertion.get(window) === insertionPoint &&
> +            tab._tPos > insertionPoint) {
> +            insertionPoint++;
> +            indexMap.set(window, insertionPoint);

I don't think this is necessary, given that we wind up setting it to `tab._tPos` later, anyway.
Attachment #8839956 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 18

3 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/efb859759339
make tabs.move on multiple tabs more reliable r=kmag
Keywords: checkin-needed

Comment 19

3 months ago
sorry had to back this out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=89985667&repo=autoland&lineNumber=4397

https://hg.mozilla.org/integration/autoland/rev/ba85f1082f97
Flags: needinfo?(amckay)
(Assignee)

Comment 20

3 months ago
mozreview-review
Comment on attachment 8839956 [details]
bug 1323311 make tabs.move on multiple tabs more reliable

https://reviewboard.mozilla.org/r/114532/#review130526

::: browser/components/extensions/ext-tabs.js:925
(Diff revision 1)
> +          // then you need to bump up the insertion point. See bug 1323311.
> +          if (lastInsertion.has(window) &&
> +            lastInsertion.get(window) === insertionPoint &&
> +            tab._tPos > insertionPoint) {
> +            insertionPoint++;
> +            indexMap.set(window, insertionPoint);

I'm not sure where we do set it to `tab._tPos` later on. But I think we need to keep the indexMap so we can remember that index point for the window.
Comment hidden (mozreview-request)
(Assignee)

Comment 22

3 months ago
Sorry, fixed.
Flags: needinfo?(amckay)
Keywords: checkin-needed

Comment 23

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5813e4358a17
make tabs.move on multiple tabs more reliable r=kmag
Keywords: checkin-needed

Comment 24

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5813e4358a17
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.