From 210c67ec10c3af072b68a85f6be2c5ae56cea8b9 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 17 Sep 2026 10:28:55 -0400 Subject: [PATCH 1/4] 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 --- .../content/zotero/collectionViewItemTree.jsx | 5 ++ chrome/content/zotero/itemTree.jsx | 74 ++++++++++----- test/tests/collectionViewItemTreeTest.js | 89 +++++++++++++++++++ 3 files changed, 147 insertions(+), 21 deletions(-) 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); From e9ffaf0457b930d8568f455f2c7c42c75447e834 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 17 Sep 2026 11:09:52 -0400 Subject: [PATCH 2/4] Keep fractional item tree scroll position when restoring it Restoring by row index snapped the list to a row boundary after a change, so a partly scrolled row jumped into place. Anchor on the row's pixel offset instead, which also makes windowed-list's forceScrollToTop unnecessary. --- .../zotero/components/virtualized-table.jsx | 2 +- .../zotero/components/windowed-list.js | 21 ++++++++++--------- chrome/content/zotero/itemTree.jsx | 11 ++++++---- test/tests/collectionViewItemTreeTest.js | 12 +++++------ 4 files changed, 25 insertions(+), 21 deletions(-) diff --git a/chrome/content/zotero/components/virtualized-table.jsx b/chrome/content/zotero/components/virtualized-table.jsx index 99facfbeda..f50d92dff8 100644 --- a/chrome/content/zotero/components/virtualized-table.jsx +++ b/chrome/content/zotero/components/virtualized-table.jsx @@ -898,7 +898,7 @@ class VirtualizedTable extends React.Component { && this._getSectionHeaderIndices().some(i => i < index)) { topOffset = this._rowHeight; } - this._jsWindow.scrollToRow(index, false, topOffset); + this._jsWindow.scrollToRow(index, topOffset); } /** diff --git a/chrome/content/zotero/components/windowed-list.js b/chrome/content/zotero/components/windowed-list.js index cd264e6851..4142a69224 100644 --- a/chrome/content/zotero/components/windowed-list.js +++ b/chrome/content/zotero/components/windowed-list.js @@ -203,13 +203,11 @@ module.exports = class { /** * Scroll the scrollbox to a specified item. No-op if already in view * @param {Integer} index - * @param {Boolean} forceScrollToTop If true, the row will be scrolled to the top of the scrollbox - * even if it is below the current scroll window. * @param {Integer} topOffset Amount of space reserved at the top of the scrollbox (e.g. for a * sticky section header that overlays the rows). When scrolling a row into view from above, the * row is positioned below this offset rather than flush with the top edge. */ - scrollToRow(index, forceScrollToTop = false, topOffset = 0) { + scrollToRow(index, topOffset = 0) { const { scrollOffset } = this; const itemCount = this._getItemCount(); const height = this.getWindowHeight(); @@ -217,13 +215,6 @@ module.exports = class { index = Math.max(0, Math.min(index, itemCount - 1)); let startPosition = this._getItemPosition(index); let endPosition = this._getItemPosition(index + 1); - // If forceScrollToTop is set, always scroll to the start position even if the row is - // already visible. This is used when restoring scroll position, where we need an exact - // first-visible-row rather than just ensuring the row is within view. - if (forceScrollToTop) { - this.scrollTo(startPosition); - return; - } if (startPosition - topOffset < scrollOffset) { this.scrollTo(startPosition - topOffset); } @@ -232,6 +223,16 @@ module.exports = class { } } + /** + * Return the position of the top of a row relative to the top of the list + * + * @param {Integer} index + * @return {Integer} + */ + getRowPosition(index) { + return this._getItemPosition(index); + } + getFirstVisibleRow() { const idx = this._binarySearchOffsets(this._rowOffsets, this.scrollOffset, true); const [offsetIdx, offset] = this._rowOffsets[idx]; diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx index a21a35d4c0..b4390b8d42 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/chrome/content/zotero/itemTree.jsx @@ -2714,7 +2714,7 @@ var ItemTree = class ItemTree extends LibraryTree { if (row === undefined) { continue; } - this._treebox.scrollToRow(Math.max(row - anchor.offset, 0), true); + this._treebox.scrollTo(this._treebox.getRowPosition(row) - anchor.offset); return; } } @@ -2723,7 +2723,9 @@ var ItemTree = class ItemTree extends LibraryTree { * Return an object describing the current scroll position to restore after changes * * @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 + * treeViewID) and .offset (pixels between the top of the view and the top of the row, + * so that a partly scrolled row is restored where it was), or false if there's nothing + * to anchor to */ _saveScrollPosition() { if (!this._treebox) return false; @@ -2733,6 +2735,7 @@ var ItemTree = class ItemTree extends LibraryTree { return false; } var last = treebox.getLastVisibleRow(); + var scrollOffset = treebox.scrollOffset; // If an object is selected, keep the first selected one in position var selection = null; @@ -2742,7 +2745,7 @@ var ItemTree = class ItemTree extends LibraryTree { if (row) { selection = { id: row.ref.treeViewID, - offset: i - first + offset: treebox.getRowPosition(i) - scrollOffset }; } break; @@ -2764,7 +2767,7 @@ var ItemTree = class ItemTree extends LibraryTree { if (firstRow) { viewport = { id: firstRow.ref.treeViewID, - offset: 0 + offset: treebox.getRowPosition(first) - scrollOffset }; } diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index 088469d811..2daf655d1c 100644 --- a/test/tests/collectionViewItemTreeTest.js +++ b/test/tests/collectionViewItemTreeTest.js @@ -785,8 +785,8 @@ describe("CollectionViewItemTree", function () { await waitForItemsLoad(win); var parentRow = itemsView.getRowIndexByID(parentItem.id); - treebox.scrollToRow(parentRow); - var firstVisibleBefore = treebox.getFirstVisibleRow(); + treebox.scrollTo(treebox.getRowPosition(parentRow)); + var scrollOffsetBefore = treebox.scrollOffset; // Select a visible row below the container await itemsView.selectItem(itemsView.getRow(parentRow + 2).ref.id); @@ -796,7 +796,7 @@ describe("CollectionViewItemTree", function () { await itemsView.waitForLoad(); assert.isTrue(itemsView.isContainerOpen(parentRow)); - assert.equal(treebox.getFirstVisibleRow(), firstVisibleBefore); + assert.equal(treebox.scrollOffset, scrollOffsetBefore); assert.isTrue(itemsView.tree.rowIsVisible(parentRow)); }); }); @@ -1308,9 +1308,9 @@ describe("CollectionViewItemTree", function () { await waitForItemsLoad(win); itemsView.expandAllRows(true); - treebox.scrollToRow(0); + // Scroll partway into a row, which should be preserved as is + treebox.scrollTo(7); 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 @@ -1322,7 +1322,7 @@ describe("CollectionViewItemTree", function () { itemsView.getRowIndexByID(attachment.id), itemsView.getRowIndexByID(parentItem2.id) + 1 ); - assert.equal(treebox.getFirstVisibleRow(), firstVisibleBefore); + assert.equal(treebox.scrollOffset, 7); }); it("should update search results when items are added", async function () { From 0a7e4d26b4263096b96970f737c0b3d477294a23 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 17 Sep 2026 11:54:11 -0400 Subject: [PATCH 3/4] Deprecate windowed-list _getItemPosition() in favor of getRowPosition() --- .../zotero/components/virtualized-table.jsx | 6 ++-- .../zotero/components/windowed-list.js | 34 +++++++++++-------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/chrome/content/zotero/components/virtualized-table.jsx b/chrome/content/zotero/components/virtualized-table.jsx index f50d92dff8..f8479d122d 100644 --- a/chrome/content/zotero/components/virtualized-table.jsx +++ b/chrome/content/zotero/components/virtualized-table.jsx @@ -1557,7 +1557,7 @@ class VirtualizedTable extends React.Component { let currentIndex = -1; let nextIndex = -1; for (let index of headerIndices) { - if (this._jsWindow._getItemPosition(index) <= scrollTop) { + if (this._jsWindow.getRowPosition(index) <= scrollTop) { currentIndex = index; } else { @@ -1568,7 +1568,7 @@ class VirtualizedTable extends React.Component { // Show the pinned copy only once the header row has scrolled up past the top edge of // the view let stuck = currentIndex != -1 - && scrollTop > this._jsWindow._getItemPosition(currentIndex); + && scrollTop > this._jsWindow.getRowPosition(currentIndex); if (!stuck) { clip.style.display = 'none'; this._stickyHeaderIndex = null; @@ -1597,7 +1597,7 @@ class VirtualizedTable extends React.Component { // Push the pinned header up as the next section's header approaches the top let translateY = 0; if (nextIndex != -1) { - let nextTop = this._jsWindow._getItemPosition(nextIndex) - scrollTop; + let nextTop = this._jsWindow.getRowPosition(nextIndex) - scrollTop; if (nextTop < this._rowHeight) { translateY = nextTop - this._rowHeight; } diff --git a/chrome/content/zotero/components/windowed-list.js b/chrome/content/zotero/components/windowed-list.js index 4142a69224..46fbc8c15e 100644 --- a/chrome/content/zotero/components/windowed-list.js +++ b/chrome/content/zotero/components/windowed-list.js @@ -102,7 +102,7 @@ module.exports = class { if (!this._renderedRows.has(index)) return; let oldElem = this._renderedRows.get(index); let elem = this.renderItem(index, oldElem); - elem.style.top = this._getItemPosition(index) + "px"; + elem.style.top = this.getRowPosition(index) + "px"; elem.style.position = "absolute"; if (elem == oldElem) return; this.innerElem.replaceChild(elem, this._renderedRows.get(index)); @@ -138,7 +138,7 @@ module.exports = class { for (let index = startIndex; index < stopIndex; index++) { if (this._renderedRows.has(index)) continue; let elem = renderItem(index); - elem.style.top = this._getItemPosition(index) + "px"; + elem.style.top = this.getRowPosition(index) + "px"; elem.style.position = "absolute"; innerElem.appendChild(elem); this._renderedRows.set(index, elem); @@ -213,8 +213,8 @@ module.exports = class { const height = this.getWindowHeight(); index = Math.max(0, Math.min(index, itemCount - 1)); - let startPosition = this._getItemPosition(index); - let endPosition = this._getItemPosition(index + 1); + let startPosition = this.getRowPosition(index); + let endPosition = this.getRowPosition(index + 1); if (startPosition - topOffset < scrollOffset) { this.scrollTo(startPosition - topOffset); } @@ -223,16 +223,6 @@ module.exports = class { } } - /** - * Return the position of the top of a row relative to the top of the list - * - * @param {Integer} index - * @return {Integer} - */ - getRowPosition(index) { - return this._getItemPosition(index); - } - getFirstVisibleRow() { const idx = this._binarySearchOffsets(this._rowOffsets, this.scrollOffset, true); const [offsetIdx, offset] = this._rowOffsets[idx]; @@ -246,12 +236,26 @@ module.exports = class { return Math.max(1, offsetIdx + Math.ceil(((this.scrollOffset + height + 1) - offset) / this.itemHeight)) - 1; } - _getItemPosition = (index) => { + /** + * Return the position of the top of a row relative to the top of the list + * + * @param {Integer} index + * @return {Integer} + */ + getRowPosition = (index) => { const idx = this._binarySearchOffsets(this._rowOffsets, index); const [offsetIdx, offset] = this._rowOffsets[idx]; return offset + (this.itemHeight * (index - offsetIdx)); }; + /** + * @deprecated Use getRowPosition() + */ + _getItemPosition = (index) => { + Zotero.warn('windowed-list _getItemPosition() is deprecated -- use getRowPosition()'); + return this.getRowPosition(index); + }; + _getRangeToRender() { const { overscanCount, scrollDirection } = this; const itemCount = this._getItemCount(); From 4a3566eb30ebb0ce26dbfa785cf9f966d5455b10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adomas=20Ven=C4=8Dkauskas?= Date: Tue, 22 Sep 2026 15:38:01 +0300 Subject: [PATCH 4/4] Make tree scroll behavior more consistent. Default to preserving viewport position (top item), only preserve selection position in tree viewport when editing the item metadata (which might change sort position) or moving attachments. --- .../content/zotero/collectionViewItemTree.jsx | 5 - chrome/content/zotero/itemTree.jsx | 122 +++---- test/tests/collectionViewItemTreeTest.js | 315 +++++++----------- 3 files changed, 169 insertions(+), 273 deletions(-) diff --git a/chrome/content/zotero/collectionViewItemTree.jsx b/chrome/content/zotero/collectionViewItemTree.jsx index 523dda32e6..30ad42f7c8 100644 --- a/chrome/content/zotero/collectionViewItemTree.jsx +++ b/chrome/content/zotero/collectionViewItemTree.jsx @@ -679,7 +679,6 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { let selectInActiveWindow = false; let restoreSelection = true; let restoreScroll = true; - let preserveViewport = false; let rowsToSelect = null; let items = null; @@ -860,7 +859,6 @@ 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) { @@ -871,7 +869,6 @@ 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) { @@ -881,7 +878,6 @@ 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) { @@ -1105,7 +1101,6 @@ 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 b4390b8d42..515ceaee17 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/chrome/content/zotero/itemTree.jsx @@ -302,7 +302,6 @@ class ItemTreeRowProvider { restoreSelection: !preserveDetachedFocus, expandCollapsedParents: false, restoreScroll: true, - preserveViewport: true, }); if (preserveDetachedFocus) { // Collapsing a container with selected descendants moves their selection to the @@ -361,7 +360,6 @@ class ItemTreeRowProvider { restoreSelection: true, expandCollapsedParents: false, restoreScroll: true, - preserveViewport: true, }); } @@ -396,7 +394,6 @@ class ItemTreeRowProvider { restoreSelection: true, expandCollapsedParents: false, restoreScroll: true, - preserveViewport: true, }); } @@ -1233,9 +1230,17 @@ var ItemTree = class ItemTree extends LibraryTree { return this.rowProvider.refresh(options); }) - _cacheState() { + /** + * Capture selection and scroll position before changing rows. Preserve the first + * visible row by default; single-item edits and reparenting preserve selection scroll. + * The update's restoreSelection and restoreScroll flags independently apply this state. + * + * @param {Object} [options] + * @param {boolean} [options.preserveSelectionScroll=false] + */ + _cacheState({ preserveSelectionScroll = false } = {}) { this._cachedSelection = this.getSelectedObjects(); - this._cachedScrollPosition = this._saveScrollPosition(); + this._cachedScrollPosition = this._saveScrollPosition({ preserveSelectionScroll }); } /** @@ -1249,8 +1254,6 @@ 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). */ @@ -1261,7 +1264,6 @@ var ItemTree = class ItemTree extends LibraryTree { expandCollapsedParents: true, ensureRowsAreVisible: true, restoreScroll: false, - preserveViewport: false, loading: false, message: null, }) { @@ -1310,7 +1312,7 @@ var ItemTree = class ItemTree extends LibraryTree { } if (options.restoreScroll) { - this._restoreScrollPosition(null, options.preserveViewport); + this._restoreScrollPosition(); } // Allow selection events to propagate and redraw the needed rows @@ -1358,7 +1360,17 @@ var ItemTree = class ItemTree extends LibraryTree { return; } - this._cacheState(); + // Preserve selection scroll for single-item edits and reparenting, including + // moves of multiple items. Other bulk changes preserve the first visible row. + let preserveSelectionScroll = action == 'modify' && type == 'item' + && (ids.length == 1 || ids.some(id => { + let row = this._rowMap[id]; + if (row === undefined) return false; + let parentIndex = this.getParentIndex(row); + let oldParentID = parentIndex == -1 ? null : this.getRow(parentIndex).ref.id; + return oldParentID != (this.getRow(row).ref.parentItemID || null); + })); + this._cacheState({ preserveSelectionScroll }); await this.rowProvider.notify(action, type, ids, extraData); } @@ -2690,11 +2702,8 @@ 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, preserveViewport = false) { + _restoreScrollPosition(scrollPosition = null) { if (scrollPosition === null) { scrollPosition = this._cachedScrollPosition; this._cachedScrollPosition = null; @@ -2702,79 +2711,56 @@ var ItemTree = class ItemTree extends LibraryTree { if (!scrollPosition || !this._treebox) { return; } - 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.scrollTo(this._treebox.getRowPosition(row) - anchor.offset); + if (scrollPosition.id === undefined) { + this._treebox.scrollTo(scrollPosition.offset); return; } + let row = this._rowMap[scrollPosition.id]; + if (row === undefined) { + return; + } + this._treebox.scrollTo(this._treebox.getRowPosition(row) - scrollPosition.offset); } /** * Return an object describing the current scroll position to restore after changes * - * @return {Object|Boolean} - Object with .selection and .viewport anchors, each with .id (a - * treeViewID) and .offset (pixels between the top of the view and the top of the row, - * so that a partly scrolled row is restored where it was), or false if there's nothing - * to anchor to + * Anchors to top visible item (viewport) scroll by default, can override to anchor to + * selected item instead. This ensures that collapse/expand doesn't move the row from under + * cursor. + * + * @param {Object} [options] + * @param {boolean} [options.preserveSelectionScroll=false] - Prefer a visible selected row over the viewport + * @return {Object|false} - A row ID and relative pixel offset */ - _saveScrollPosition() { + _saveScrollPosition({ preserveSelectionScroll = false } = {}) { if (!this._treebox) return false; var treebox = this._treebox; var first = treebox.getFirstVisibleRow(); if (first === undefined || first === null) { return false; } - var last = treebox.getLastVisibleRow(); - var scrollOffset = treebox.scrollOffset; + let scrollOffset = treebox.scrollOffset; + if (scrollOffset == 0) { + return { offset: 0 }; + } - // If an object is selected, keep the first selected one in position - var selection = null; - for (let i = first; i <= last; i++) { - if (this.selection.isSelected(i)) { - let row = this.getRow(i); - if (row) { - selection = { - id: row.ref.treeViewID, - offset: treebox.getRowPosition(i) - scrollOffset - }; + let anchorIndex = first; + if (preserveSelectionScroll) { + for (let i = first; i <= treebox.getLastVisibleRow(); i++) { + if (this.selection.isSelected(i)) { + anchorIndex = i; + break; } - break; } } - // With no selection to anchor to, don't save a scroll position when the - // 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 (!selection && !first) { - return false; - } - - // 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: treebox.getRowPosition(first) - scrollOffset - }; - } - - if (!selection && !viewport) { - return false; - } - return { selection, viewport }; + let row = this.getRow(anchorIndex); + if (!row) return false; + return { + id: row.ref.treeViewID, + offset: treebox.getRowPosition(anchorIndex) - scrollOffset + }; } /** diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index 2daf655d1c..8d7e1c9308 100644 --- a/test/tests/collectionViewItemTreeTest.js +++ b/test/tests/collectionViewItemTreeTest.js @@ -1121,210 +1121,125 @@ describe("CollectionViewItemTree", function () { assert.sameMembers(zp.itemsView.getSelectedItems(true), [item.id]); }); - it("should keep first visible item in view when other items are added with skipSelect and nothing in view is selected", async function () { - var collection = await createDataObject('collection'); - await waitForItemsLoad(win); - itemsView = zp.itemsView; - - var treebox = itemsView._treebox; - var numVisibleRows = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow(); - - // Get a numeric string left-padded with zeroes - function getTitle(i, max) { - return new String(new Array(max + 1).join(0) + i).slice(-1 * max); - } - - var num = numVisibleRows + 10; - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < num; i++) { - let title = getTitle(i, num); - let item = createUnsavedDataObject('item', { title }); - item.addToCollection(collection.id); - await item.save(); - } - }.bind(this)); - - // Scroll halfway - treebox.scrollToRow(Math.round(num / 2) - Math.round(numVisibleRows / 2)); - - var firstVisibleItemID = itemsView.getRow(treebox.getFirstVisibleRow()).ref.id; - - // Add one item at the beginning - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.saveTx({ - skipSelect: true + describe("scroll position", function () { + var collection, items, treebox; + + beforeEach(async function () { + collection = await createDataObject('collection'); + await select(win, collection); + itemsView = zp.itemsView; + treebox = itemsView._treebox; + items = []; + let count = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow() + 30; + await Zotero.DB.executeTransaction(async function () { + for (let i = 0; i < count; i++) { + let item = createUnsavedDataObject('item', { + title: String(i).padStart(4, '0'), + collections: [collection.id], + }); + await item.save({ skipSelect: true }); + items.push(item); + } + }); + await itemsView.selectItem(items[8].id); + treebox.scrollTo(treebox.getRowPosition(4) + 7); }); - // Then add a few more in a transaction - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < 3; i++) { - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } + + it("should preserve the selected item's position in viewport when an edit changes its sort position", async function () { + let offset = treebox.getRowPosition(itemsView.getRowIndexByID(items[8].id)) - treebox.scrollOffset; + items[8].setField('title', '0012a'); + await items[8].saveTx(); + assert.equal( + treebox.getRowPosition(itemsView.getRowIndexByID(items[8].id)) - treebox.scrollOffset, + offset + ); + }); + + it("should preserve scroll position for multi-item edits", async function () { + let offset = treebox.scrollOffset; + await Zotero.DB.executeTransaction(async function () { + for (let i of [8, 9]) { + items[i].setField('title', '0012' + i); + await items[i].save(); + } + }); + assert.equal(treebox.scrollOffset, offset); + }); + + it("should preserve scroll position when items are added above the visible rows with no selection", async function () { + itemsView.selection.clearSelection(); + let firstItem = itemsView.getRow(treebox.getFirstVisibleRow()).ref; + let offset = treebox.getRowPosition(itemsView.getRowIndexByID(firstItem.id)) - treebox.scrollOffset; + await Zotero.DB.executeTransaction(async function () { + for (let i = 0; i < 3; i++) { + let item = createUnsavedDataObject('item', { + title: '0000a', collections: [collection.id] + }); + await item.save({ skipSelect: true }); + } + }); + assert.equal(itemsView.getRow(treebox.getFirstVisibleRow()).ref.id, firstItem.id); + assert.equal(treebox.getRowPosition(itemsView.getRowIndexByID(firstItem.id)) - treebox.scrollOffset, offset); + }); + + it("shouldn't scroll when items are added within the visible rows with skipSelect", async function () { + let offset = treebox.scrollOffset; + await Zotero.DB.executeTransaction(async function () { + for (let i = 0; i < 3; i++) { + let item = createUnsavedDataObject('item', { + title: '0005a', collections: [collection.id] + }); + await item.save({ skipSelect: true }); + } + }); + assert.sameMembers(itemsView.getSelectedItems(true), [items[8].id]); + assert.equal(treebox.scrollOffset, offset); + }); + + it("shouldn't scroll when at the top and items are added with skipSelect", async function () { + await itemsView.selectItem(items[2].id); + treebox.scrollTo(0); + await Zotero.DB.executeTransaction(async function () { + for (let i = 0; i < 3; i++) { + let item = createUnsavedDataObject('item', { + title: '000', collections: [collection.id] + }); + await item.save({ skipSelect: true }); + } + }); + assert.equal(treebox.scrollOffset, 0); + }); + + for (let count of [1, 2]) { + it(`should preserve selection scroll position when moving ${count} child item(s) to another parent`, async function () { + let attachments = []; + for (let i = 0; i < count; i++) { + attachments.push(await importFileAttachment('test.png', { parentItemID: items[0].id })); + } + itemsView.expandAllRows(true); + await itemsView.selectItems(attachments.map(item => item.id)); + treebox.scrollTo(7); + let firstSelected = itemsView.getRow(itemsView.getRowIndexByID(items[0].id) + 1).ref; + let offset = treebox.getRowPosition(itemsView.getRowIndexByID(firstSelected.id)) - treebox.scrollOffset; + + await Zotero.DB.executeTransaction(async function () { + for (let attachment of attachments) { + attachment.parentItemID = items[3].id; + await attachment.save(); + } + }); + await itemsView.waitForLoad(); + + assert.sameMembers(itemsView.getSelectedItems(true), attachments.map(item => item.id)); + assert.equal( + treebox.getRowPosition(itemsView.getRowIndexByID(firstSelected.id)) - treebox.scrollOffset, + offset ); - await item.save({ - skipSelect: true - }); - } - }.bind(this)); - - // Make sure the same item is still in the first visible row - assert.equal(itemsView.getRow(treebox.getFirstVisibleRow()).ref.id, firstVisibleItemID); - }); - - it("should keep first visible selected item in position when other items are added with skipSelect", async function () { - var collection = await createDataObject('collection'); - await select(win, collection); - itemsView = zp.itemsView; - - var treebox = itemsView._treebox; - var numVisibleRows = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow(); - - // Get a numeric string left-padded with zeroes - function getTitle(i, max) { - return new String(new Array(max + 1).join(0) + i).slice(-1 * max); + assert.isAbove(treebox.scrollOffset, 7); + }); } - - var num = numVisibleRows + 10; - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < num; i++) { - let title = getTitle(i, num); - let item = createUnsavedDataObject('item', { title }); - item.addToCollection(collection.id); - await item.save(); - } - }); - - // Scroll halfway - treebox.scrollToRow(Math.round(num / 2) - Math.round(numVisibleRows / 2)); - - // Select an item - itemsView.selection.select(Math.round(num / 2)); - var selectedItem = itemsView.getSelectedItems()[0]; - var offset = itemsView.getRowIndexByID(selectedItem.treeViewID) - treebox.getFirstVisibleRow(); - - // Add one item at the beginning - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.saveTx({ - skipSelect: true - }); - // Then add a few more in a transaction - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < 3; i++) { - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.save({ - skipSelect: true - }); - } - }); - - // Make sure the selected item is still at the same position - assert.equal(itemsView.getSelectedItems()[0], selectedItem); - var newOffset = itemsView.getRowIndexByID(selectedItem.treeViewID) - treebox.getFirstVisibleRow(); - assert.equal(newOffset, offset); }); - - it("shouldn't scroll items list if at top when other items are added with skipSelect", async function () { - var collection = await createDataObject('collection'); - await select(win, collection); - itemsView = zp.itemsView; - - var treebox = itemsView._treebox; - var numVisibleRows = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow(); - - // Get a numeric string left-padded with zeroes - function getTitle(i, max) { - return new String(new Array(max + 1).join(0) + i).slice(-1 * max); - } - - var num = numVisibleRows + 10; - await Zotero.DB.executeTransaction(async function () { - // Start at "*1" so we can add items before - for (let i = 1; i < num; i++) { - let title = getTitle(i, num); - let item = createUnsavedDataObject('item', { title }); - item.addToCollection(collection.id); - await item.save(); - } - }.bind(this)); - - // Scroll to top - treebox.scrollToRow(0); - - // Add one item at the beginning - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.saveTx({ - skipSelect: true - }); - // Then add a few more in a transaction - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < 3; i++) { - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.save({ - skipSelect: true - }); - } - }.bind(this)); - - // Make sure the first row is still at the top - 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); - // Scroll partway into a row, which should be preserved as is - treebox.scrollTo(7); - await itemsView.selectItem(attachment.id); - 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.scrollOffset, 7); - }); - + it("should update search results when items are added", async function () { var search = await createDataObject('search'); await select(win, search);