Bug 1437310 - Fix moving bookmarks across special folders. r?mak
MozReview-Commit-ID: dj6dOwDxOT
--- 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);
}