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