From bdb656ad5cd3e9f8bbb10f0bc8c070c3c22d0e04 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Tue, 15 Sep 2026 22:15:55 -0400 Subject: [PATCH] Fix dragging file attachments to and from parent items on Windows Since 56eb77b704, drags of file attachments allow only 'copy' so that File Explorer doesn't move the file out of storage, but the trees set dropEffect to 'move' in onDragOver() for drops within Zotero, and OLE refuses a drop whose dropEffect isn't among the drag's allowed effects. Have setDropEffect() fall back to an allowed effect and have onDrop() act on the effect the tree chose, kept in Zotero.DragDrop.currentDropEffect, rather than on the drop event's dropEffect. https://forums.zotero.org/discussion/133765/ --- chrome/content/zotero/collectionTree.jsx | 8 ++- .../content/zotero/collectionViewItemTree.jsx | 8 ++- chrome/content/zotero/libraryTree.js | 13 +++++ chrome/content/zotero/xpcom/zotero.js | 3 + test/tests/collectionTreeTest.js | 55 ++++++++++++++++++- test/tests/collectionViewItemTreeTest.js | 45 +++++++++++++++ 6 files changed, 126 insertions(+), 6 deletions(-) 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);