scriptoriumadmin/zotero
1
0
Fork 0
mirror of https://github.com/zotero/zotero.git synced 2026-10-01 02:01:24 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

Fix dragging file attachments to and from parent items on Windows

Browse source 
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/
This commit is contained in:
Dan Stillman 2026-09-15 22:15:55 -04:00
parent c30163ed90
commit bdb656ad5c
6 changed files with 126 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
chrome/content/zotero/collectionTree.jsx
View file

@ -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);

8
chrome/content/zotero/collectionViewItemTree.jsx
View file

@ -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);

13
chrome/content/zotero/libraryTree.js
View file

@ -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;
}

3
chrome/content/zotero/xpcom/zotero.js
View file

@ -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;

55
test/tests/collectionTreeTest.js
View file

@ -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);

45
test/tests/collectionViewItemTreeTest.js
View file

@ -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);