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 () {