diff --git a/chrome/content/zotero/collectionTree.jsx b/chrome/content/zotero/collectionTree.jsx index 21de5676ad..c2e34ba69c 100644 --- a/chrome/content/zotero/collectionTree.jsx +++ b/chrome/content/zotero/collectionTree.jsx @@ -1580,6 +1580,7 @@ var CollectionTree = class CollectionTree extends LibraryTree { try { // Prevent modifier keys from doing their normal things event.preventDefault(); + Zotero.DragDrop.currentDropEffect = null; var previousOrientation = Zotero.DragDrop.currentOrientation; Zotero.DragDrop.currentOrientation = getDragTargetOrient(event); @@ -2294,7 +2295,11 @@ var CollectionTree = class CollectionTree extends LibraryTree { this._flashingRow = null; this.tree.invalidateRow(oldFlashing); - if (!dataTransfer.dropEffect || dataTransfer.dropEffect == "none" + // Use the effect set in onDragOver(), which the drop event's dropEffect may not reflect + // (see LibraryTreeView::setDropEffect()) + var dropEffect = Zotero.DragDrop.currentDropEffect || dataTransfer.dropEffect; + Zotero.DragDrop.currentDropEffect = null; + if (!dropEffect || dropEffect == "none" || !(await this.canDropCheckAsync(row, orient, dataTransfer))) { return false; } @@ -2303,7 +2308,6 @@ var CollectionTree = class CollectionTree extends LibraryTree { Zotero.debug("No drag data"); return false; } - var dropEffect = dragData.dropEffect; var dataType = dragData.dataType; var data = dragData.data; var sourceTreeRow = Zotero.DragDrop.getDragSource(dataTransfer); diff --git a/chrome/content/zotero/collectionViewItemTree.jsx b/chrome/content/zotero/collectionViewItemTree.jsx index f16c5c614a..30ad42f7c8 100644 --- a/chrome/content/zotero/collectionViewItemTree.jsx +++ b/chrome/content/zotero/collectionViewItemTree.jsx @@ -1351,6 +1351,7 @@ class CollectionViewItemTree extends ItemTree { try { event.preventDefault(); event.stopPropagation(); + Zotero.DragDrop.currentDropEffect = null; var previousOrientation = Zotero.DragDrop.currentOrientation; Zotero.DragDrop.currentOrientation = getDragTargetOrient(event); Zotero.debug(`Dragging over item ${row} with ${Zotero.DragDrop.currentOrientation}, drop row: ${this._dropRow}`); @@ -1603,7 +1604,11 @@ class CollectionViewItemTree extends ItemTree { } this._dropRow = null; Zotero.DragDrop.currentDragSource = null; - if (!dataTransfer.dropEffect || dataTransfer.dropEffect == "none") { + // Use the effect set in onDragOver(), which the drop event's dropEffect may not reflect + // (see LibraryTreeView::setDropEffect()) + var dropEffect = Zotero.DragDrop.currentDropEffect || dataTransfer.dropEffect; + Zotero.DragDrop.currentDropEffect = null; + if (!dropEffect || dropEffect == "none") { return false; } @@ -1612,7 +1617,6 @@ class CollectionViewItemTree extends ItemTree { Zotero.debug("No drag data"); return false; } - var dropEffect = dragData.dropEffect; var dataType = dragData.dataType; var data = dragData.data; var sourceCollectionTreeRow = Zotero.DragDrop.getDragSource(dataTransfer); diff --git a/chrome/content/zotero/libraryTree.js b/chrome/content/zotero/libraryTree.js index e82c1f19be..ee80a5b157 100644 --- a/chrome/content/zotero/libraryTree.js +++ b/chrome/content/zotero/libraryTree.js @@ -276,6 +276,19 @@ var LibraryTree = class LibraryTree extends React.Component { // the same action as the dropEffect. This allows the dropEffect setting // (which we use in the tree's canDrop() and drop() to determine the desired // action) to be changed, even if the cursor doesn't reflect the new setting. + // + // The effect also has to be one of the actions allowed at drag start: on Windows, + // OLE refuses the drop entirely if it isn't. Some drags allow only 'copy' (see + // Zotero.Utilities.Internal.onDragItems()), so a 'move' within Zotero has to be sent + // as a 'copy'. The trees' onDrop() handlers act on the effect set here, kept in + // Zotero.DragDrop.currentDropEffect, rather than on the drop event's dropEffect, so + // the drop still moves. + Zotero.DragDrop.currentDropEffect = effect; + let allowed = event.dataTransfer.effectAllowed; + if (effect != 'none' && allowed && !['uninitialized', 'all'].includes(allowed) + && !allowed.toLowerCase().includes(effect)) { + effect = ['copy', 'move', 'link'].find(x => allowed.toLowerCase().includes(x)) || 'none'; + } if (Zotero.isWin || Zotero.isLinux) { event.dataTransfer.effectAllowed = effect; } diff --git a/chrome/content/zotero/xpcom/zotero.js b/chrome/content/zotero/xpcom/zotero.js index ee40548f7b..c9333c598f 100644 --- a/chrome/content/zotero/xpcom/zotero.js +++ b/chrome/content/zotero/xpcom/zotero.js @@ -2338,6 +2338,9 @@ Zotero.VersionHeader = { Zotero.DragDrop = { currentEvent: null, currentOrientation: 0, + // The effect set by the tree's last onDragOver() via LibraryTreeView::setDropEffect(), which + // can differ from the drop event's dropEffect + currentDropEffect: null, getDataFromDataTransfer: function (dataTransfer, firstOnly) { var dt = dataTransfer; diff --git a/test/tests/collectionTreeTest.js b/test/tests/collectionTreeTest.js index 669df1f3fc..0b009d5ec8 100644 --- a/test/tests/collectionTreeTest.js +++ b/test/tests/collectionTreeTest.js @@ -1049,7 +1049,7 @@ describe("Zotero.CollectionTree", function () { // Simulate a drag over a row and return the resulting dropEffect ('copy', 'move', or // 'none'). Pass { move: true } to simulate the platform's move modifier being held. - var dragOver = function (objectType, targetRowID, ids, { move = false } = {}) { + var dragOver = function (objectType, targetRowID, ids, { move = false, effectAllowed = 'copyMove' } = {}) { var index = cv.getRowIndexByID(targetRowID); Zotero.DragDrop.currentDragSource = objectType == "item" @@ -1063,7 +1063,7 @@ describe("Zotero.CollectionTree", function () { }; var dataTransfer = { dropEffect: 'copy', - effectAllowed: 'copyMove', + effectAllowed, types: [`zotero/${objectType}`], getData: function (type) { if (type == `zotero/${objectType}`) { @@ -1084,6 +1084,7 @@ describe("Zotero.CollectionTree", function () { dataTransfer }, index); Zotero.DragDrop.currentDragSource = null; + Zotero.DragDrop.currentDropEffect = null; return dataTransfer.dropEffect; }; @@ -1117,6 +1118,56 @@ describe("Zotero.CollectionTree", function () { assert.equal(treeRow.ref.id, item.id); }) + it("should move an item when the drag only allows copying", async function () { + var collection1 = await createDataObject('collection'); + await select(win, collection1); + var collection2 = await createDataObject('collection'); + var item = await createDataObject('item', { collections: [collection1.id] }); + + var index = cv.getRowIndexByID('C' + collection2.id); + var rowEl = { + classList: { contains: () => true }, + getBoundingClientRect: () => ({ y: 0, height: 100 }) + }; + var dataTransfer = { + dropEffect: 'copy', + effectAllowed: 'copy', + types: ['zotero/item'], + getData: function (type) { + if (type == 'zotero/item') { + return item.id + ""; + } + return ""; + }, + setDragImage: () => {} + }; + Zotero.DragDrop.currentDragSource = zp.itemsView.collectionTreeRows[0]; + cv.onDragOver({ + preventDefault: () => {}, + stopPropagation: () => {}, + currentTarget: rowEl, + target: rowEl, + clientY: 50, + metaKey: Zotero.isMac, + shiftKey: !Zotero.isMac, + dataTransfer + }, index); + // A file attachment drag allows only copying, so the requested move has to be + // sent as a copy for the drop to happen + assert.equal(dataTransfer.dropEffect, 'copy'); + + var promise = waitForNotifierEvent('add', 'collection-item'); + await cv.onDrop({ + persist: () => 0, + target: { ownerDocument: { defaultView: win } }, + dataTransfer + }, index); + await promise; + Zotero.DragDrop.currentDragSource = null; + + assert.sameMembers(item.getCollections(), [collection2.id]); + }); + it("should move an item from one collection to another", async function () { var collection1 = await createDataObject('collection'); await select(win, collection1); diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index 889b5dfc5a..301b975de8 100644 --- a/test/tests/collectionViewItemTreeTest.js +++ b/test/tests/collectionViewItemTreeTest.js @@ -2275,6 +2275,51 @@ describe("CollectionViewItemTree", function () { assert.isFalse(itemsView.isContainerEmpty(itemsView.getRowIndexByID(item2.id))); }); + it("should move a child item when the drag only allows copying", async function () { + var collection = await createDataObject('collection'); + await waitForItemsLoad(win); + var item1 = await createDataObject('item', { title: "A", collections: [collection.id] }); + var item2 = await createDataObject('item', { title: "B", collections: [collection.id] }); + var attachment = await importFileAttachment('test.pdf', { parentItemID: item1.id }); + + await itemsView.selectItem(attachment.id); + + var dataTransfer = { + dropEffect: 'copy', + effectAllowed: 'copy', + types: ['zotero/item'], + getData: function (type) { + if (type == 'zotero/item') { + return attachment.id + ""; + } + return ""; + }, + mozItemCount: 1 + }; + var index = itemsView.getRowIndexByID(item2.id); + var rowEl = { + classList: { contains: () => false }, + getBoundingClientRect: () => ({ y: 0, height: 100 }) + }; + Zotero.DragDrop.currentDragSource = itemsView.collectionTreeRows[0]; + itemsView.onDragOver({ + preventDefault: () => {}, + stopPropagation: () => {}, + currentTarget: rowEl, + target: rowEl, + clientY: 50, + dataTransfer + }, index); + // The requested move has to be sent as an allowed effect for the drop to happen + assert.equal(dataTransfer.dropEffect, 'copy'); + + var promise = itemsView.waitForSelect(); + await drop(index, 0, dataTransfer); + await promise; + + assert.equal(attachment.parentItemID, item2.id); + }); + it("should move a child item from last item in list to another", async function () { var collection = await createDataObject('collection'); await waitForItemsLoad(win);