Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. r=mak
authorMark Banner <standard8@mozilla.com>
Wed, 23 Aug 2017 12:24:53 +0100
changeset 377905 7d7e8ffee135
parent 377904 f950e9aa797d
child 377906 71d4452dd938
push id50097
push usermbanner@mozilla.com
push dateThu, 31 Aug 2017 05:58:34 +0000
treeherderautoland@7d7e8ffee135 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1391166
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. r=mak MozReview-Commit-ID: 7Adz2Et0NQn
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_controller_onDrop.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -1597,20 +1597,37 @@ var PlacesControllerDragHelper = {
         throw new Error("bogus data was passed as a tab");
 
       for (let unwrapped of nodes) {
         let index = await insertionPoint.getIndex();
 
         // Adjust insertion index to prevent reversal of dragged items. When you
         // drag multiple elts upward: need to increment index or each successive
         // elt will be inserted at the same index, each above the previous.
-        let dragginUp = insertionPoint.itemId == unwrapped.parent &&
-                        index < (await PlacesUtils.bookmarks.fetch(unwrapped.itemGuid)).index;
-        if (index != -1 && dragginUp)
-          index += movedCount++;
+        if (index != -1 && unwrapped.itemGuid) {
+          // Note: we use the parent from the existing bookmark as the sidebar
+          // gives us an unwrapped.parent that is actually a query and not the real
+          // parent.
+          let existingBookmark = await PlacesUtils.bookmarks.fetch(unwrapped.itemGuid);
+          let dragginUp = parentGuid == existingBookmark.parentGuid &&
+                          index < existingBookmark.index;
+
+          if (dragginUp) {
+            index += movedCount++;
+          } else if (PlacesUIUtils.useAsyncTransactions) {
+            if (index == existingBookmark.index) {
+              // We're moving to the same index, so there's nothing for us to do.
+              continue;
+            }
+            // If we're dragging down, we need to go one lower to insert at
+            // the real point as moving the element changes the index of
+            // everything below by 1.
+            index--;
+          }
+        }
 
         // If dragging over a tag container we should tag the item.
         if (insertionPoint.isTag) {
           let uri = NetUtil.newURI(unwrapped.uri);
           let tagItemId = insertionPoint.itemId;
           if (PlacesUIUtils.useAsyncTransactions)
             transactions.push(PlacesTransactions.Tag({ uri, tag: tagName }));
           else
@@ -1633,17 +1650,21 @@ var PlacesControllerDragHelper = {
           } else {
             transactions.push(PlacesUIUtils.makeTransaction(unwrapped,
                                 flavor, insertionPoint.itemId,
                                 index, doCopy));
           }
         }
       }
     }
-
+    // Check if we actually have something to add, if we don't it probably wasn't
+    // valid, or it was moving to the same location, so just ignore it.
+    if (!transactions.length) {
+      return;
+    }
     if (PlacesUIUtils.useAsyncTransactions) {
       await PlacesTransactions.batch(transactions);
     } else {
       let txn = new PlacesAggregatedTransaction("DropItems", transactions);
       PlacesUtils.transactionManager.doTransaction(txn);
     }
   },
 
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -23,16 +23,17 @@ support-files =
 [browser_bookmarkProperties_bookmarkAllTabs.js]
 [browser_bookmarkProperties_editTagContainer.js]
 [browser_bookmarkProperties_readOnlyRoot.js]
 [browser_bookmarksProperties.js]
 [browser_check_correct_controllers.js]
 [browser_click_bookmarks_on_toolbar.js]
 [browser_cutting_bookmarks.js]
 subsuite = clipboard
+[browser_controller_onDrop.js]
 [browser_copy_folder_tree.js]
 [browser_copy_query_without_tree.js]
 subsuite = clipboard
 [browser_drag_bookmarks_on_toolbar.js]
 [browser_forgetthissite_single.js]
 [browser_history_sidebar_search.js]
 [browser_library_batch_delete.js]
 [browser_library_commands.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_controller_onDrop.js
@@ -0,0 +1,132 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+/* global sinon */
+Services.scriptloader.loadSubScript("resource://testing-common/sinon-2.3.2.js");
+
+const sandbox = sinon.sandbox.create();
+
+var bookmarks;
+var bookmarkIds;
+
+add_task(async function setup() {
+  registerCleanupFunction(async function() {
+    sandbox.restore();
+    delete window.sinon;
+    await PlacesUtils.bookmarks.eraseEverything();
+    await PlacesTestUtils.clearHistory();
+  });
+
+  sandbox.stub(PlacesUIUtils, "getTransactionForData");
+  sandbox.stub(PlacesTransactions, "batch");
+
+  bookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: [{
+      title: "bm1",
+      url: "http://example1.com"
+    }, {
+      title: "bm2",
+      url: "http://example2.com"
+    }, {
+      title: "bm3",
+      url: "http://example3.com"
+    }]
+  });
+
+  bookmarkIds = await PlacesUtils.promiseManyItemIds([
+    bookmarks[0].guid,
+    bookmarks[1].guid,
+    bookmarks[2].guid,
+  ]);
+});
+
+async function run_drag_test(startBookmarkIndex, insertionIndex,
+                             realInsertionIndex, expectTransactionCreated = true) {
+  // Reset the stubs so that previous test runs don't count against us.
+  PlacesUIUtils.getTransactionForData.reset();
+  PlacesTransactions.batch.reset();
+
+  let dragBookmark = bookmarks[startBookmarkIndex];
+
+  await withSidebarTree("bookmarks", async (tree) => {
+    tree.selectItems([PlacesUtils.unfiledBookmarksFolderId]);
+    PlacesUtils.asContainer(tree.selectedNode).containerOpen = true;
+
+    // Simulating a drag-drop with a tree view turns out to be really difficult
+    // as you can't get a node for the source/target. Hence, we fake the
+    // insertion point and drag data and call the function direct.
+    let ip = new InsertionPoint({
+      parentId: await PlacesUtils.promiseItemId(PlacesUtils.bookmarks.unfiledGuid),
+      parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+      index: insertionIndex,
+      orientation: Ci.nsITreeView.DROP_ON
+    });
+
+    let bookmarkWithId = JSON.stringify(Object.assign({
+      id: bookmarkIds.get(dragBookmark.guid),
+      itemGuid: dragBookmark.guid,
+      parent: PlacesUtils.unfiledBookmarksFolderId,
+    }, dragBookmark));
+
+    let dt = {
+      dropEffect: "move",
+      mozCursor: "auto",
+      mozItemCount: 1,
+      types: [ PlacesUtils.TYPE_X_MOZ_PLACE ],
+      mozTypesAt(i) {
+        return this.types;
+      },
+      mozGetDataAt(i) {
+        return bookmarkWithId;
+      }
+    };
+
+    await PlacesControllerDragHelper.onDrop(ip, dt);
+
+    if (!expectTransactionCreated) {
+      Assert.ok(PlacesUIUtils.getTransactionForData.notCalled,
+        "Should not have created transaction data");
+      return;
+    }
+
+    Assert.ok(PlacesUIUtils.getTransactionForData.calledOnce,
+      "Should have called getTransactionForData at least once.");
+
+    let args = PlacesUIUtils.getTransactionForData.args[0];
+
+    Assert.deepEqual(args[0], JSON.parse(bookmarkWithId),
+      "Should have called getTransactionForData with the correct unwrapped bookmark");
+    Assert.equal(args[1], PlacesUtils.TYPE_X_MOZ_PLACE,
+      "Should have called getTransactionForData with the correct flavor");
+    Assert.equal(args[2], PlacesUtils.bookmarks.unfiledGuid,
+      "Should have called getTransactionForData with the correct parent guid");
+    Assert.equal(args[3], realInsertionIndex,
+      "Should have called getTransactionForData with the correct index");
+    Assert.equal(args[4], false,
+      "Should have called getTransactionForData with a move");
+  });
+}
+
+add_task(async function test_simple_move_down() {
+  // When we move items down the list, we'll get a drag index that is one higher
+  // than where we actually want to insert to - as the item is being moved up,
+  // everything shifts down one. Hence the index to pass to the transaction should
+  // be one less than the supplied index.
+  await run_drag_test(0, 2, 1);
+});
+
+add_task(async function test_simple_move_up() {
+  // When we move items up the list, we want the matching index to be passed to
+  // the transaction as there's no changes below the item in the list.
+  await run_drag_test(2, 0, 0);
+});
+
+add_task(async function test_simple_move_to_same() {
+  // If we move to the same index, then we don't expect any transactions to be
+  // created.
+  await run_drag_test(1, 1, 1, false);
+});