bug 1323311 make tabs.move on multiple tabs more reliable r=kmag draft
authorAndy McKay <amckay@mozilla.com>
Mon, 10 Apr 2017 11:18:04 -0700
changeset 559873 392718783a20d59860cf8315b1687985799f076e
parent 558673 35c7be9c2db288d1d449e3cc586c4164d642c5fd
child 623539 8662c55ff0e9c0a5950615b599e40ec4aeaa591f
push id53244
push userbmo:amckay@mozilla.com
push dateMon, 10 Apr 2017 18:19:00 +0000
reviewerskmag
bugs1323311
milestone55.0a1
bug 1323311 make tabs.move on multiple tabs more reliable r=kmag MozReview-Commit-ID: 9o4huF1f60g
browser/components/extensions/ext-tabs.js
browser/components/extensions/test/browser/browser-common.ini
browser/components/extensions/test/browser/browser_ext_tabs_move_array.js
--- a/browser/components/extensions/ext-tabs.js
+++ b/browser/components/extensions/ext-tabs.js
@@ -511,17 +511,16 @@ this.tabs = class extends ExtensionAPI {
 
         async removeCSS(tabId, details) {
           let tab = await promiseTabWhenReady(tabId);
 
           return tab.removeCSS(context, details);
         },
 
         async move(tabIds, moveProperties) {
-          let index = moveProperties.index;
           let tabsMoved = [];
           if (!Array.isArray(tabIds)) {
             tabIds = [tabIds];
           }
 
           let destinationWindow = null;
           if (moveProperties.windowId !== null) {
             destinationWindow = windowTracker.getWindow(moveProperties.windowId);
@@ -534,49 +533,60 @@ this.tabs = class extends ExtensionAPI {
           /*
             Indexes are maintained on a per window basis so that a call to
               move([tabA, tabB], {index: 0})
                 -> tabA to 0, tabB to 1 if tabA and tabB are in the same window
               move([tabA, tabB], {index: 0})
                 -> tabA to 0, tabB to 0 if tabA and tabB are in different windows
           */
           let indexMap = new Map();
+          let lastInsertion = new Map();
 
           let tabs = tabIds.map(tabId => tabTracker.getTab(tabId));
           for (let nativeTab of tabs) {
             // If the window is not specified, use the window from the tab.
             let window = destinationWindow || nativeTab.ownerGlobal;
             let gBrowser = window.gBrowser;
 
-            let insertionPoint = indexMap.get(window) || index;
+            let insertionPoint = indexMap.get(window) || moveProperties.index;
             // If the index is -1 it should go to the end of the tabs.
             if (insertionPoint == -1) {
               insertionPoint = gBrowser.tabs.length;
             }
 
             // We can only move pinned tabs to a point within, or just after,
             // the current set of pinned tabs. Unpinned tabs, likewise, can only
             // be moved to a position after the current set of pinned tabs.
             // Attempts to move a tab to an illegal position are ignored.
             let numPinned = gBrowser._numPinnedTabs;
             let ok = nativeTab.pinned ? insertionPoint <= numPinned : insertionPoint >= numPinned;
             if (!ok) {
               continue;
             }
 
-            indexMap.set(window, insertionPoint + 1);
+            // If this is not the first tab to be inserted into this window and
+            // the insertion point is the same as the last insertion and
+            // the tab is further to the right than the current insertion point
+            // then you need to bump up the insertion point. See bug 1323311.
+            if (lastInsertion.has(window) &&
+                lastInsertion.get(window) === insertionPoint &&
+                nativeTab._tPos > insertionPoint) {
+              insertionPoint++;
+              indexMap.set(window, insertionPoint);
+            }
 
             if (nativeTab.ownerGlobal != window) {
               // If the window we are moving the tab in is different, then move the tab
               // to the new window.
               nativeTab = gBrowser.adoptTab(nativeTab, insertionPoint, false);
             } else {
               // If the window we are moving is the same, just move the tab.
               gBrowser.moveTabTo(nativeTab, insertionPoint);
             }
+            lastInsertion.set(window, nativeTab._tPos);
             tabsMoved.push(nativeTab);
           }
 
           return tabsMoved.map(nativeTab => tabManager.convert(nativeTab));
         },
 
         duplicate(tabId) {
           let nativeTab = tabTracker.getTab(tabId);
--- a/browser/components/extensions/test/browser/browser-common.ini
+++ b/browser/components/extensions/test/browser/browser-common.ini
@@ -103,16 +103,17 @@ support-files =
 [browser_ext_tabs_executeScript_good.js]
 [browser_ext_tabs_executeScript_bad.js]
 [browser_ext_tabs_executeScript_no_create.js]
 [browser_ext_tabs_executeScript_runAt.js]
 [browser_ext_tabs_getCurrent.js]
 [browser_ext_tabs_insertCSS.js]
 [browser_ext_tabs_removeCSS.js]
 [browser_ext_tabs_move.js]
+[browser_ext_tabs_move_array.js]
 [browser_ext_tabs_move_window.js]
 [browser_ext_tabs_move_window_multiple.js]
 [browser_ext_tabs_move_window_pinned.js]
 [browser_ext_tabs_onHighlighted.js]
 [browser_ext_tabs_onUpdated.js]
 [browser_ext_tabs_query.js]
 [browser_ext_tabs_reload.js]
 [browser_ext_tabs_reload_bypass_cache.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_move_array.js
@@ -0,0 +1,72 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+"use strict";
+
+add_task(function* moveMultiple() {
+  let tabs = [];
+  for (let k of [1, 2, 3, 4]) {
+    let tab = yield BrowserTestUtils.openNewForegroundTab(window.gBrowser, `http://example.com/?${k}`);
+    tabs.push(tab);
+  }
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {permissions: ["tabs"]},
+    background: async function() {
+      function num(url) {
+        return parseInt(url.slice(-1), 10);
+      }
+
+      async function check(expected) {
+        let tabs = await browser.tabs.query({url: "http://example.com/*"});
+        let endings = tabs.map(tab => num(tab.url));
+        browser.test.assertTrue(
+          expected.every((v, i) => v === endings[i]),
+          `Tab order should be ${expected}, got ${endings}.`
+        );
+      }
+
+      async function reset() {
+        let tabs = await browser.tabs.query({url: "http://example.com/*"});
+        await browser.tabs.move(
+            tabs.sort((a, b) => (num(a.url) - num(b.url))).map(tab => tab.id),
+            {index: 0}
+        );
+      }
+
+      async function move(moveIndexes, moveTo) {
+        let tabs = await browser.tabs.query({url: "http://example.com/*"});
+        await browser.tabs.move(
+          moveIndexes.map(e => tabs[e - 1].id),
+          {index: moveTo});
+      }
+
+      let tests = [
+        // Start -> After first tab  -> After second tab
+        // [1, 2, 3, 4] -> [1, 4, 2, 3] -> [1, 4, 3, 2]
+        {"move": [4, 3], "index":  1, "result": [1, 4, 3, 2]},
+        // [1, 2, 3, 4] -> [2, 3, 1, 4] -> [3, 1, 2, 4]
+        {"move": [1, 2], "index":  2, "result": [3, 1, 2, 4]},
+        // [1, 2, 3, 4] -> [1, 2, 4, 3] -> [2, 4, 1, 3]
+        {"move": [4, 1], "index":  2, "result": [2, 4, 1, 3]},
+        // [1, 2, 3, 4] -> [2, 3, 1, 4] -> [2, 3, 1, 4]
+        {"move": [1, 4], "index":  2, "result": [2, 3, 1, 4]},
+      ];
+
+      for (let test of tests) {
+        await reset();
+        await move(test.move, test.index);
+        await check(test.result);
+      }
+
+      browser.test.notifyPass("tabs.move");
+    },
+  });
+
+  yield extension.startup();
+  yield extension.awaitFinish("tabs.move");
+  yield extension.unload();
+
+  for (let tab of tabs) {
+    yield BrowserTestUtils.removeTab(tab);
+  }
+});