Don't scroll item tree on changes made by the user in the view

The scroll position restored after a change was anchored to the selected
row, so expanding a container above the selection or dragging an
attachment to another item shifted the view. Anchor those to the first
visible row instead, keeping the selection anchor for items arriving in
the background, which shouldn't move what the user has selected.

Fixes #6023

chrome/content/zotero/collectionViewItemTree.jsx

@@ -679,6 +679,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
 		let selectInActiveWindow = false;
 		let restoreSelection = true;
 		let restoreScroll = true;
+		let preserveViewport = false;
 		let rowsToSelect = null;
 		let items = null;
 
@@ -859,6 +860,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
 						// Close container to remove any sub-items
 						this._closeContainer(row, true);
 						this._removeRow(row);
+						preserveViewport = true;
 					}
 					// If moved from under another item to top level, remove old row and add new one
 					else if (parentIndex != -1 && !parentItemID) {
@@ -869,6 +871,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
 						this._addRow(this.createRow(item, 0, false), beforeRow);
 
 						sort = id;
+						preserveViewport = true;
 					}
 					// If moved from one parent to another, remove from old parent
 					else if (parentItemID && parentIndex != -1 && this._rowMap[parentItemID] != parentIndex) {
@@ -878,6 +881,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
 						if (newParentIndex !== undefined) {
 							this._refreshContainer(newParentIndex);
 						}
+						preserveViewport = true;
 					}
 					// If Unfiled Items and item was added to a collection, remove from view
 					else if (this.itemTree.isContainer(row) && this.viewMode == 'unfiled' && item.getCollections().length) {
@@ -1101,6 +1105,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
 			await this.runListeners('update', true, {
 				restoreSelection,
 				restoreScroll,
+				preserveViewport,
 				selectInActiveWindow,
 				selection: rowsToSelect
 			});

chrome/content/zotero/itemTree.jsx

@@ -302,6 +302,7 @@ class ItemTreeRowProvider {
 			restoreSelection: !preserveDetachedFocus,
 			expandCollapsedParents: false,
 			restoreScroll: true,
+			preserveViewport: true,
 		});
 		if (preserveDetachedFocus) {
 			// Collapsing a container with selected descendants moves their selection to the
@@ -360,6 +361,7 @@ class ItemTreeRowProvider {
 			restoreSelection: true,
 			expandCollapsedParents: false,
 			restoreScroll: true,
+			preserveViewport: true,
 		});
 	}
 
@@ -394,6 +396,7 @@ class ItemTreeRowProvider {
 			restoreSelection: true,
 			expandCollapsedParents: false,
 			restoreScroll: true,
+			preserveViewport: true,
 		});
 	}
 
@@ -1246,6 +1249,8 @@ var ItemTree = class ItemTree extends LibraryTree {
 	 * @param {boolean} options.restoreSelection - Whether to restore the cached selection.
 	 * @param {boolean} options.ensureRowsAreVisible - Whether to ensure selected rows are visible.
 	 * @param {boolean} options.restoreScroll - Whether to restore the cached scroll position.
+	 * @param {boolean} options.preserveViewport - Whether to restore the scroll position by
+	 *     keeping the first visible row in place rather than the selected row.
 	 * @param {boolean} options.loading - Whether to show loading state (hides tree, shows message).
 	 * @param {string} options.message - Optional message to display (for loading, errors, intro text).
 	 */
@@ -1256,6 +1261,7 @@ var ItemTree = class ItemTree extends LibraryTree {
 		expandCollapsedParents: true,
 		ensureRowsAreVisible: true,
 		restoreScroll: false,
+		preserveViewport: false,
 		loading: false,
 		message: null,
 	}) {
@@ -1304,7 +1310,7 @@ var ItemTree = class ItemTree extends LibraryTree {
 		}
 
 		if (options.restoreScroll) {
-			this._restoreScrollPosition();
+			this._restoreScrollPosition(null, options.preserveViewport);
 		}
 
 		// Allow selection events to propagate and redraw the needed rows
@@ -2684,26 +2690,40 @@ var ItemTree = class ItemTree extends LibraryTree {
 	 * If scrollPosition is provided, restores from it without touching the cache.
 	 *
 	 * @param {Object|null} scrollPosition - Scroll position to restore, or null to use cached
+	 * @param {Boolean} preserveViewport - Keep the first visible row in position rather than the
+	 *     selected row, for changes the user made in this view (e.g. expanding a container or
+	 *     dragging an attachment to another item), which shouldn't shift what's on screen
 	 */
-	_restoreScrollPosition(scrollPosition = null) {
+	_restoreScrollPosition(scrollPosition = null, preserveViewport = false) {
 		if (scrollPosition === null) {
 			scrollPosition = this._cachedScrollPosition;
 			this._cachedScrollPosition = null;
 		}
-		if (!scrollPosition || !scrollPosition.id || !this._treebox) {
+		if (!scrollPosition || !this._treebox) {
 			return;
 		}
-		var row = this._rowMap[scrollPosition.id];
-		if (row === undefined) {
+		let anchors = preserveViewport
+			? [scrollPosition.viewport, scrollPosition.selection]
+			: [scrollPosition.selection, scrollPosition.viewport];
+		// Use the first anchor that's still in the view
+		for (let anchor of anchors) {
+			if (!anchor) {
+				continue;
+			}
+			let row = this._rowMap[anchor.id];
+			if (row === undefined) {
+				continue;
+			}
+			this._treebox.scrollToRow(Math.max(row - anchor.offset, 0), true);
 			return;
 		}
-		this._treebox.scrollToRow(Math.max(row - scrollPosition.offset, 0), true);
 	}
 
 	/**
 	 * Return an object describing the current scroll position to restore after changes
 	 *
-	 * @return {Object|Boolean} - Object with .id (a treeViewID) and .offset, or false if no rows
+	 * @return {Object|Boolean} - Object with .selection and .viewport anchors, each with .id (a
+	 *     treeViewID) and .offset, or false if there's nothing to anchor to
 	 */
 	_saveScrollPosition() {
 		if (!this._treebox) return false;
@@ -2713,15 +2733,19 @@ var ItemTree = class ItemTree extends LibraryTree {
 			return false;
 		}
 		var last = treebox.getLastVisibleRow();
+
+		// If an object is selected, keep the first selected one in position
+		var selection = null;
 		for (let i = first; i <= last; i++) {
-			// If an object is selected, keep the first selected one in position
 			if (this.selection.isSelected(i)) {
 				let row = this.getRow(i);
-				if (!row) return false;
-				return {
-					id: row.ref.treeViewID,
-					offset: i - first
-				};
+				if (row) {
+					selection = {
+						id: row.ref.treeViewID,
+						offset: i - first
+					};
+				}
+				break;
 			}
 		}
 
@@ -2729,17 +2753,25 @@ var ItemTree = class ItemTree extends LibraryTree {
 		// view is already at the top of the list. Otherwise restoring after an
 		// insertion would pin the previously-top row in place (pushing the view
 		// down) instead of leaving the list scrolled to its new top.
-		if (!first) {
+		if (!selection && !first) {
 			return false;
 		}
 
-		// Otherwise keep the first visible row in position
-		let row = this.getRow(first);
-		if (!row) return false;
-		return {
-			id: row.ref.treeViewID,
-			offset: 0
-		};
+		// Keep the first visible row in position, for changes that shouldn't shift what's on
+		// screen even when the selected row moves
+		var viewport = null;
+		let firstRow = this.getRow(first);
+		if (firstRow) {
+			viewport = {
+				id: firstRow.ref.treeViewID,
+				offset: 0
+			};
+		}
+
+		if (!selection && !viewport) {
+			return false;
+		}
+		return { selection, viewport };
 	}
 
 	/**

test/tests/collectionViewItemTreeTest.js

@@ -757,6 +757,48 @@ describe("CollectionViewItemTree", function () {
 			assert.equal(treebox.getFirstVisibleRow(), firstVisibleBefore);
 			assert.isFalse(itemsView.tree.rowIsVisible(itemsView.getRowIndexByID(selectedItemID)));
 		});
+		
+		it("shouldn't scroll when opening a container above the selected row", async function () {
+			var collection = await createDataObject('collection');
+			await select(win, collection);
+			itemsView = zp.itemsView;
+			
+			var treebox = itemsView._treebox;
+			var numVisibleRows = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow();
+			
+			// Sort the container to the top, with more rows below than fit in the view
+			var num = numVisibleRows + 10;
+			var parentItem = await createDataObject('item', {
+				title: String(0).padStart(num, '0'),
+				collections: [collection.id]
+			});
+			await importFileAttachment('test.png', { parentItemID: parentItem.id });
+			await Zotero.DB.executeTransaction(async function () {
+				for (let i = 1; i < num; i++) {
+					let item = createUnsavedDataObject('item', {
+						title: String(i).padStart(num, '0'),
+						collections: [collection.id]
+					});
+					await item.save();
+				}
+			});
+			await waitForItemsLoad(win);
+			
+			var parentRow = itemsView.getRowIndexByID(parentItem.id);
+			treebox.scrollToRow(parentRow);
+			var firstVisibleBefore = treebox.getFirstVisibleRow();
+			
+			// Select a visible row below the container
+			await itemsView.selectItem(itemsView.getRow(parentRow + 2).ref.id);
+			assert.isFalse(itemsView.isContainerOpen(parentRow));
+			
+			await itemsView.toggleOpenState(parentRow);
+			await itemsView.waitForLoad();
+			
+			assert.isTrue(itemsView.isContainerOpen(parentRow));
+			assert.equal(treebox.getFirstVisibleRow(), firstVisibleBefore);
+			assert.isTrue(itemsView.tree.rowIsVisible(parentRow));
+		});
 	});
 	
 	describe("#sort()", function () {
@@ -1236,6 +1278,53 @@ describe("CollectionViewItemTree", function () {
 			assert.equal(treebox.getFirstVisibleRow(), 0);
 		});
 		
+		it("shouldn't scroll items list when a child item is moved to a parent further down", async function () {
+			var collection = await createDataObject('collection');
+			await select(win, collection);
+			itemsView = zp.itemsView;
+			
+			var treebox = itemsView._treebox;
+			var numVisibleRows = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow();
+			
+			var num = numVisibleRows + 10;
+			var parentItem1 = await createDataObject('item', {
+				title: String(0).padStart(num, '0'),
+				collections: [collection.id]
+			});
+			var parentItem2 = await createDataObject('item', {
+				title: String(3).padStart(num, '0'),
+				collections: [collection.id]
+			});
+			await Zotero.DB.executeTransaction(async function () {
+				for (let i = 4; i < num; i++) {
+					let item = createUnsavedDataObject('item', {
+						title: String(i).padStart(num, '0'),
+						collections: [collection.id]
+					});
+					await item.save();
+				}
+			});
+			var attachment = await importFileAttachment('test.png', { parentItemID: parentItem1.id });
+			await waitForItemsLoad(win);
+			
+			itemsView.expandAllRows(true);
+			treebox.scrollToRow(0);
+			await itemsView.selectItem(attachment.id);
+			var firstVisibleBefore = treebox.getFirstVisibleRow();
+			assert.isTrue(itemsView.tree.rowIsVisible(itemsView.getRowIndexByID(parentItem2.id)));
+			
+			// Move the attachment to the parent below
+			attachment.parentItemID = parentItem2.id;
+			await attachment.saveTx();
+			await itemsView.waitForLoad();
+			
+			assert.equal(
+				itemsView.getRowIndexByID(attachment.id),
+				itemsView.getRowIndexByID(parentItem2.id) + 1
+			);
+			assert.equal(treebox.getFirstVisibleRow(), firstVisibleBefore);
+		});
+		
 		it("should update search results when items are added", async function () {
 			var search = await createDataObject('search');
 			await select(win, search);