Repository navigation
fix(virtual-core): preserve reading position on paddingStart/scrollMargin change with anchorTo: 'end' #1370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@tanstack/virtual-core': patch | ||
| --- | ||
|
|
||
| fix(virtual-core): preserve reading position when `paddingStart` or `scrollMargin` changes under `anchorTo: 'end'` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -822,6 +822,7 @@ export class Virtualizer< | |
| let anchor: [Key, number] | null = null | ||
| let followOnAppend: ScrollBehavior | null = null | ||
| let edgeKeysChanged = false | ||
| let leadOffsetChanged = false | ||
|
|
||
| if ( | ||
| prevOptions !== undefined && | ||
|
|
@@ -839,15 +840,22 @@ export class Virtualizer< | |
| const prevFirstKey = prevCount > 0 ? getPreviousKey(0) : null | ||
| const prevLastKey = prevCount > 0 ? getPreviousKey(prevCount - 1) : null | ||
| const didCountChange = nextCount !== prevCount | ||
| const didLeadOffsetChange = | ||
| merged.paddingStart !== prevOptions.paddingStart || | ||
| merged.scrollMargin !== prevOptions.scrollMargin | ||
| const didEdgeKeysChange = | ||
| didCountChange || | ||
| (prevCount > 0 && | ||
| nextCount > 0 && | ||
| (merged.getItemKey(0) !== prevFirstKey || | ||
| merged.getItemKey(nextCount - 1) !== prevLastKey)) | ||
|
|
||
| if (didEdgeKeysChange) { | ||
| edgeKeysChanged = true | ||
| if (didEdgeKeysChange || didLeadOffsetChange) { | ||
| if (didEdgeKeysChange) { | ||
| edgeKeysChanged = true | ||
| } else { | ||
| leadOffsetChanged = true | ||
| } | ||
| const item = | ||
| prevCount > 0 | ||
| ? (this.getVirtualItemForOffset(this.getScrollOffset()) ?? | ||
|
|
@@ -858,27 +866,30 @@ export class Virtualizer< | |
| anchor = [item.key, this.getScrollOffset() - item.start] | ||
| } | ||
|
|
||
| const behavior = | ||
| merged.followOnAppend === true | ||
| ? 'auto' | ||
| : merged.followOnAppend || null | ||
| if (didEdgeKeysChange) { | ||
| const behavior = | ||
| merged.followOnAppend === true | ||
| ? 'auto' | ||
| : merged.followOnAppend || null | ||
|
|
||
| if ( | ||
| behavior && | ||
| nextCount > 0 && | ||
| this.isAtEnd(prevOptions.scrollEndThreshold) && | ||
| (prevCount === 0 || merged.getItemKey(nextCount - 1) !== prevLastKey) | ||
| ) { | ||
| if ( | ||
| nextCount > prevCount || | ||
| isAppendWithTrim( | ||
| prevCount, | ||
| nextCount, | ||
| getPreviousKey, | ||
| merged.getItemKey, | ||
| ) | ||
| behavior && | ||
| nextCount > 0 && | ||
| this.isAtEnd(prevOptions.scrollEndThreshold) && | ||
| (prevCount === 0 || | ||
| merged.getItemKey(nextCount - 1) !== prevLastKey) | ||
| ) { | ||
| followOnAppend = behavior | ||
| if ( | ||
| nextCount > prevCount || | ||
| isAppendWithTrim( | ||
| prevCount, | ||
| nextCount, | ||
| getPreviousKey, | ||
| merged.getItemKey, | ||
| ) | ||
| ) { | ||
| followOnAppend = behavior | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -894,6 +905,8 @@ export class Virtualizer< | |
| if (edgeKeysChanged) { | ||
| this.pendingMin = 0 | ||
| this.itemSizeCacheVersion++ | ||
| } else if (leadOffsetChanged) { | ||
| this.pendingMin = 0 | ||
| } | ||
|
|
||
| // Eagerly adjust scrollOffset so the virtualizer computes the correct | ||
|
|
@@ -941,15 +954,22 @@ export class Virtualizer< | |
| if (followOnAppend) { | ||
| this.pendingScrollAnchor = { kind: 'follow', behavior: followOnAppend } | ||
| } else if (anchorDelta !== null) { | ||
| const prevDelta = | ||
| this.pendingScrollAnchor?.kind === 'anchor' | ||
| ? this.pendingScrollAnchor.delta | ||
| : 0 | ||
| const nextDelta = prevDelta + anchorDelta | ||
| if (this._canWriteScroll()) { | ||
| this.pendingScrollAnchor = { kind: 'anchor', delta: anchorDelta } | ||
| this.pendingScrollAnchor = | ||
| nextDelta === 0 ? null : { kind: 'anchor', delta: nextDelta } | ||
|
Comment on lines
+957
to
+964
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } else { | ||
| this.pendingScrollAnchor = null | ||
| // The element cannot take the write (a touch owns it on iOS, or it is | ||
| // overscrolled). Hold the delta instead: this render already exposes | ||
| // the rows and the total shifted back by it, so the reader's row | ||
| // stays put, and the flush writes it once a write is possible. | ||
| this._holdOffset += anchorDelta | ||
| this._followOffsetTarget(anchorDelta) | ||
| this._holdOffset += nextDelta | ||
| this._followOffsetTarget(nextDelta) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1298,7 +1318,17 @@ export class Virtualizer< | |
| // reconcile not yet retired it) still gets the anchor sync. | ||
| } else { | ||
| this._followOffsetTarget(anchor.delta) | ||
| this._scrollToOffset(this.getScrollOffset()) | ||
| const target = this.getScrollOffset() | ||
| const layoutMax = Math.max( | ||
| this._getLayoutTotalSize() + | ||
| this.options.scrollMargin - | ||
| this.getSize(), | ||
| 0, | ||
| ) | ||
| if (target <= layoutMax + 0.5) { | ||
| this._clampedAdjustment = this._clampedBy(target) | ||
| } | ||
|
Comment on lines
+1328
to
+1330
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| this._scrollToOffset(target) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep When An upper-bound clamp to Location
|
||
| } | ||
| } else { | ||
| this.scrollToEnd({ behavior: anchor.behavior }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Script executed:
rg -n 'getMaxScrollOffset|isAtEnd|pendingScrollAnchor|setOptions\(' packages/virtual-core/src/index.ts packages/virtual-core/tests/anchor-end.test.tsRepository: TanStack/virtual
Length of output: 3830
Script executed:
Repository: TanStack/virtual
Length of output: 8602
Script executed:
Repository: TanStack/virtual
Length of output: 7678
Script executed:
Repository: TanStack/virtual
Length of output: 2033
Script executed:
Repository: TanStack/virtual
Length of output: 234
Script executed:
Repository: TanStack/virtual
Length of output: 299
Script executed:
Repository: TanStack/virtual
Length of output: 846
Script executed:
Repository: TanStack/virtual
Length of output: 2042
Compare the follow check against the same offset space that
scrollToEnduses.After a
paddingStartorscrollMarginchange,setOptionsmovesscrollOffsetbefore the DOM sizer commits. If a secondsetOptionsappends an item before that commit,isAtEnd()compares the moved offset with the stale element maximum. A reader 20px above the bottom can then be treated as end-pinned and scrolled to the bottom. The follow check should use the lead-adjusted maximum, or exclude the pending lead delta.