diff --git a/chrome/content/zotero/components/virtualized-table.jsx b/chrome/content/zotero/components/virtualized-table.jsx index 9617fca8c4..09816fcf51 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); } /** @@ -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 cd264e6851..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); @@ -203,27 +203,18 @@ 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(); 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; - } + let startPosition = this.getRowPosition(index); + let endPosition = this.getRowPosition(index + 1); if (startPosition - topOffset < scrollOffset) { this.scrollTo(startPosition - topOffset); } @@ -245,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(); diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx index 4bbe9be9e6..515ceaee17 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/chrome/content/zotero/itemTree.jsx @@ -1230,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 }); } /** @@ -1352,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,55 +2708,58 @@ var ItemTree = class ItemTree extends LibraryTree { scrollPosition = this._cachedScrollPosition; this._cachedScrollPosition = null; } - if (!scrollPosition || !scrollPosition.id || !this._treebox) { + if (!scrollPosition || !this._treebox) { return; } - var row = this._rowMap[scrollPosition.id]; + if (scrollPosition.id === undefined) { + this._treebox.scrollTo(scrollPosition.offset); + return; + } + let row = this._rowMap[scrollPosition.id]; if (row === undefined) { return; } - this._treebox.scrollToRow(Math.max(row - scrollPosition.offset, 0), true); + 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 .id (a treeViewID) and .offset, or false if no rows + * 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(); - 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 - }; + let scrollOffset = treebox.scrollOffset; + if (scrollOffset == 0) { + return { offset: 0 }; + } + + let anchorIndex = first; + if (preserveSelectionScroll) { + for (let i = first; i <= treebox.getLastVisibleRow(); i++) { + if (this.selection.isSelected(i)) { + anchorIndex = i; + 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 (!first) { - return false; - } - - // Otherwise keep the first visible row in position - let row = this.getRow(first); + let row = this.getRow(anchorIndex); if (!row) return false; return { id: row.ref.treeViewID, - offset: 0 + offset: treebox.getRowPosition(anchorIndex) - scrollOffset }; } diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index 301b975de8..8d7e1c9308 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.scrollTo(treebox.getRowPosition(parentRow)); + var scrollOffsetBefore = treebox.scrollOffset; + + // 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.scrollOffset, scrollOffsetBefore); + assert.isTrue(itemsView.tree.rowIsVisible(parentRow)); + }); }); describe("#sort()", function () { @@ -1079,163 +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] } - ); - await item.save({ - skipSelect: true + + 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(); + } }); - } - }.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); - } - - 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 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 - }); - } - }); - - // 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); + assert.isAbove(treebox.scrollOffset, 7); + }); } - - 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("should update search results when items are added", async function () { var search = await createDataObject('search'); await select(win, search);