diff --git a/chrome/content/zotero/collectionViewItemTree.jsx b/chrome/content/zotero/collectionViewItemTree.jsx index 30ad42f7c8..523dda32e6 100644 --- a/chrome/content/zotero/collectionViewItemTree.jsx +++ b/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 }); diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx index 4bbe9be9e6..a21a35d4c0 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/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 }; } /** diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index 301b975de8..088469d811 100644 --- a/test/tests/collectionViewItemTreeTest.js +++ b/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);