Bug 1437310 - Fix moving bookmarks across special folders. r?mak draft
authorMark Banner <standard8@mozilla.com>
Mon, 12 Feb 2018 21:51:08 +0000
changeset 758105 faf9a120894ad61a84fa4209b0e8c9881fb0cc74
parent 757856 dd6caba141428343fb26f3ec23a3ee1844a4b241
push id99950
push userbmo:standard8@mozilla.com
push dateWed, 21 Feb 2018 21:37:08 +0000
reviewersmak
bugs1437310
milestone60.0a1
Bug 1437310 - Fix moving bookmarks across special folders. r?mak MozReview-Commit-ID: dj6dOwDxOT
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_controller_onDrop_sidebar.js
toolkit/components/places/PlacesUtils.jsm
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -1377,29 +1377,30 @@ var PlacesControllerDragHelper = {
    *        A node unwrapped by PlacesUtils.unwrapNodes().
    * @return True if the node can be moved, false otherwise.
    */
   canMoveUnwrappedNode(unwrappedNode) {
     if ((unwrappedNode.concreteGuid && PlacesUtils.isRootItem(unwrappedNode.concreteGuid)) ||
         unwrappedNode.id <= 0 || PlacesUtils.isRootItem(unwrappedNode.id)) {
       return false;
     }
-    let parentId = unwrappedNode.parent;
-    if (parentId <= 0 ||
-        parentId == PlacesUtils.placesRootId ||
-        parentId == PlacesUtils.tagsFolderId ||
-        unwrappedNode.grandParentId == PlacesUtils.tagsFolderId) {
+
+    let parentGuid = unwrappedNode.parentGuid;
+    // If there's no parent Guid, this was likely a virtual query that returns
+    // bookmarks, such as a tags query.
+    if (!parentGuid ||
+        parentGuid == PlacesUtils.bookmarks.rootGuid) {
       return false;
     }
     // leftPaneFolderId and allBookmarksFolderId are lazy getters running
     // at least a synchronous DB query. Therefore we don't want to invoke
     // them first, especially because isCommandEnabled may be called way
     // before the left pane folder is even necessary.
     if (typeof Object.getOwnPropertyDescriptor(PlacesUIUtils, "leftPaneFolderId").get != "function" &&
-        (parentId == PlacesUIUtils.leftPaneFolderId)) {
+        (unwrappedNode.parent == PlacesUIUtils.leftPaneFolderId)) {
       return false;
     }
     return true;
   },
 
   /**
    * Determines if a node can be moved.
    *
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -45,16 +45,18 @@ skip-if = (os == 'win' && ccov) # Bug 14
 [browser_bookmarkProperties_editTagContainer.js]
 [browser_bookmarkProperties_readOnlyRoot.js]
 skip-if = (os == 'win' && ccov) # Bug 1423667
 [browser_bookmarksProperties.js]
 skip-if = (os == 'win' && ccov) # Bug 1423667
 [browser_check_correct_controllers.js]
 skip-if = (os == 'win' && ccov) # Bug 1423667
 [browser_click_bookmarks_on_toolbar.js]
+[browser_controller_onDrop_sidebar.js]
+skip-if = (os == 'win' && ccov) # Bug 1423667
 [browser_controller_onDrop_tagFolder.js]
 skip-if = (os == 'win' && ccov) # Bug 1423667
 [browser_controller_onDrop.js]
 skip-if = (os == 'win' && ccov) # Bug 1423667
 [browser_copy_query_without_tree.js]
 skip-if = (os == 'win' && ccov) # Bug 1423667
 subsuite = clipboard
 [browser_cutting_bookmarks.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_controller_onDrop_sidebar.js
@@ -0,0 +1,157 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const TEST_URL = "about:buildconfig";
+
+add_task(async function setup() {
+  // Clean before and after so we don't have anything in the folders.
+  await PlacesUtils.bookmarks.eraseEverything();
+
+  registerCleanupFunction(async function() {
+    await PlacesUtils.bookmarks.eraseEverything();
+  });
+});
+
+// Simulating actual drag and drop is hard for a xul tree as we can't get the
+// required source elements, so we have to do a lot more work by hand.
+async function simulateDrop(selectTargets, sourceBm, dropEffect, targetGuid,
+                            isVirtualRoot = false) {
+  await withSidebarTree("bookmarks", async function(tree) {
+    for (let target of selectTargets) {
+      tree.selectItems([target]);
+      if (tree.selectedNode instanceof Ci.nsINavHistoryContainerResultNode) {
+        tree.selectedNode.containerOpen = true;
+      }
+    }
+
+    let dataTransfer = {
+      _data: [],
+      dropEffect,
+      mozCursor: "auto",
+      mozItemCount: 1,
+      types: [ PlacesUtils.TYPE_X_MOZ_PLACE ],
+      mozTypesAt(i) {
+        return [this._data[0].type];
+      },
+      mozGetDataAt(i) {
+        return this._data[0].data;
+      },
+      mozSetDataAt(type, data, index) {
+        this._data.push({
+          type,
+          data,
+          index
+        });
+      },
+    };
+
+    let event = {
+      dataTransfer,
+      preventDefault() {},
+      stopPropagation() {},
+    };
+
+    tree._controller.setDataTransfer(event);
+
+    Assert.equal(dataTransfer.mozTypesAt(0), PlacesUtils.TYPE_X_MOZ_PLACE,
+      "Should have x-moz-place as the first data type.");
+
+    let dataObject = JSON.parse(dataTransfer.mozGetDataAt(0));
+
+    let guid = isVirtualRoot ? dataObject.concreteGuid : dataObject.itemGuid;
+
+    Assert.equal(guid, sourceBm.guid,
+      "Should have the correct guid.");
+    Assert.equal(dataObject.title, PlacesUtils.bookmarks.getLocalizedTitle(sourceBm),
+      "Should have the correct title.");
+
+    Assert.equal(dataTransfer.dropEffect, dropEffect);
+
+    let ip = new InsertionPoint({
+      parentId: await PlacesUtils.promiseItemId(targetGuid),
+      parentGuid: targetGuid,
+      index: 0,
+      orientation: Ci.nsITreeView.DROP_ON
+    });
+
+    await PlacesControllerDragHelper.onDrop(ip, dataTransfer);
+  });
+}
+
+add_task(async function test_move_normal_bm_in_sidebar() {
+  let bm = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    title: "Fake",
+    url: TEST_URL
+  });
+
+  await simulateDrop([bm.guid], bm, "move", PlacesUtils.bookmarks.unfiledGuid);
+
+  let newBm = await PlacesUtils.bookmarks.fetch(bm.guid);
+
+  Assert.equal(newBm.parentGuid, PlacesUtils.bookmarks.unfiledGuid,
+    "Should have moved to the new parent.");
+
+  let oldLocationBm = await PlacesUtils.bookmarks.fetch({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    index: 0
+  });
+
+  Assert.ok(!oldLocationBm,
+    "Should not have a bookmark at the old location.");
+});
+
+add_task(async function test_try_move_root_in_sidebar() {
+  let menuFolder = await PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.menuGuid);
+  await simulateDrop([menuFolder.guid], menuFolder, "move",
+    PlacesUtils.bookmarks.toolbarGuid, true);
+
+  menuFolder = await PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.menuGuid);
+
+  Assert.equal(menuFolder.parentGuid, PlacesUtils.bookmarks.rootGuid,
+    "Should have remained in the root");
+
+  let newFolder = await PlacesUtils.bookmarks.fetch({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    index: 0,
+  });
+
+  Assert.notEqual(newFolder.guid, menuFolder.guid,
+    "Should have created a different folder");
+  Assert.equal(newFolder.title, PlacesUtils.bookmarks.getLocalizedTitle(menuFolder),
+    "Should have copied the folder title.");
+  Assert.equal(newFolder.type, PlacesUtils.bookmarks.TYPE_BOOKMARK,
+    "Should have a bookmark type (for a folder shortcut).");
+  Assert.equal(newFolder.url, "place:folder=BOOKMARKS_MENU",
+    "Should have the correct url for the folder shortcut.");
+});
+
+add_task(async function test_try_move_bm_within_two_root_folder_queries() {
+  await PlacesUtils.bookmarks.eraseEverything();
+
+  let bookmark = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    title: "Fake",
+    url: TEST_URL
+  });
+
+  let queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS;
+
+  let queries = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.toolbarGuid,
+    children: [{
+      title: "Query",
+      url: `place:queryType=${queryType}&terms=Fake`
+    }]
+  });
+
+  await simulateDrop([queries[0].guid, bookmark.guid],
+    bookmark, "move", PlacesUtils.bookmarks.toolbarGuid);
+
+  let newBm = await PlacesUtils.bookmarks.fetch(bookmark.guid);
+
+  Assert.equal(newBm.parentGuid, PlacesUtils.bookmarks.toolbarGuid,
+    "should have moved the bookmark to a new folder.");
+});
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -112,27 +112,33 @@ function serializeNode(aNode, aIsLivemar
   data.title = aNode.title;
   data.id = aNode.itemId;
   data.livemark = aIsLivemark;
   // Add an instanceId so we can tell which instance of an FF session the data
   // is coming from.
   data.instanceId = PlacesUtils.instanceId;
 
   let guid = aNode.bookmarkGuid;
+  let grandParentId;
+
   // Some nodes, e.g. the unfiled/menu/toolbar ones can have a virtual guid, so
   // we ignore any that are a folder shortcut. These will be handled below.
   if (guid && !PlacesUtils.bookmarks.isVirtualRootItem(guid)) {
     // TODO: Really guid should be set on everything, however currently this upsets
     // the drag 'n' drop / cut/copy/paste operations.
     data.itemGuid = guid;
-    if (aNode.parent)
+    if (aNode.parent) {
       data.parent = aNode.parent.itemId;
+      data.parentGuid = aNode.parent.bookmarkGuid;
+    }
+
     let grandParent = aNode.parent && aNode.parent.parent;
-    if (grandParent)
-      data.grandParentId = grandParent.itemId;
+    if (grandParent) {
+      grandParentId = grandParent.itemId;
+    }
 
     data.dateAdded = aNode.dateAdded;
     data.lastModified = aNode.lastModified;
 
     let annos = PlacesUtils.getAnnotationsForItem(data.id);
     if (annos.length > 0)
       data.annos = annos;
   }
@@ -147,17 +153,17 @@ function serializeNode(aNode, aIsLivemar
 
     data.type = PlacesUtils.TYPE_X_MOZ_PLACE;
     data.uri = aNode.uri;
 
     if (aNode.tags)
       data.tags = aNode.tags;
   } else if (PlacesUtils.nodeIsContainer(aNode)) {
     // Tag containers accept only uri nodes.
-    if (data.grandParentId == PlacesUtils.tagsFolderId)
+    if (grandParentId == PlacesUtils.tagsFolderId)
       throw new Error("Unexpected node type");
 
     let concreteId = PlacesUtils.getConcreteItemId(aNode);
     if (concreteId != -1) {
       // This is a bookmark or a tag container.
       if (PlacesUtils.nodeIsQuery(aNode) || concreteId != aNode.itemId) {
         // This is a folder shortcut.
         data.type = PlacesUtils.TYPE_X_MOZ_PLACE;
@@ -171,17 +177,17 @@ function serializeNode(aNode, aIsLivemar
     } else {
       // This is a grouped container query, dynamically generated.
       data.type = PlacesUtils.TYPE_X_MOZ_PLACE;
       data.uri = aNode.uri;
     }
   } else if (PlacesUtils.nodeIsSeparator(aNode)) {
     // Tag containers don't accept separators.
     if (data.parent == PlacesUtils.tagsFolderId ||
-        data.grandParentId == PlacesUtils.tagsFolderId)
+        grandParentId == PlacesUtils.tagsFolderId)
       throw new Error("Unexpected node type");
 
     data.type = PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR;
   }
 
   return JSON.stringify(data);
 }