From 299140fa81dd691279173dbcf552ea2470212ca3 Mon Sep 17 00:00:00 2001 From: Githena Date: Thu, 10 Sep 2026 11:19:35 +0000 Subject: [PATCH] fix(virtual-core): use getMaxScrollOffset() - paddingEnd for scrollToIndex(last) end-align MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1257: when paddingEnd > 0, scrollToIndex(last, { align: 'end' }) was returning the raw DOM max scroll (scrollHeight - clientHeight), which equals (content + paddingEnd - clientHeight) and overshoots the rendered end of the last item by exactly paddingEnd pixels. The fix subtracts paddingEnd from getMaxScrollOffset(), which equals (content - clientHeight) — the correct virtual max offset that keeps the last item flush with the bottom of the viewport. Also preserves #1001: getMaxScrollOffset() still absorbs DOM extras (borders, padding, unmeasured items) that aren't in our measurements, so multi-lane layouts where the last item lives in a shorter lane still scroll to the lane-max rather than leaving the item above the viewport. Closes TANStack/virtual#1263 --- .changeset/fix-scrolltoindex-paddingend.md | 5 + packages/virtual-core/src/index.ts | 9 +- packages/virtual-core/tests/index.test.ts | 147 +++++++++++++++++++++ 3 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 .changeset/fix-scrolltoindex-paddingend.md diff --git a/.changeset/fix-scrolltoindex-paddingend.md b/.changeset/fix-scrolltoindex-paddingend.md new file mode 100644 index 000000000..6c849fa29 --- /dev/null +++ b/.changeset/fix-scrolltoindex-paddingend.md @@ -0,0 +1,5 @@ +--- +"@tanstack/virtual-core": patch +--- + +fix(virtual-core): use getMaxScrollOffset() - paddingEnd for scrollToIndex(last) end-align, preserving lane-max behavior from #1001 diff --git a/packages/virtual-core/src/index.ts b/packages/virtual-core/src/index.ts index 55c276edf..aa8494cab 100644 --- a/packages/virtual-core/src/index.ts +++ b/packages/virtual-core/src/index.ts @@ -1841,9 +1841,14 @@ export class Virtualizer< } // For the last item with 'end' alignment, use browser's actual max scroll - // to account for borders/padding that aren't in our measurements + // to account for borders/padding that aren't in our measurements (#1001), + // but subtract paddingEnd to keep the last item flush with the viewport + // bottom rather than overshooting past it (#1257). if (align === 'end' && index === this.options.count - 1) { - return [this.getMaxScrollOffset(), align] as const + return [ + Math.max(this.getMaxScrollOffset() - this.options.paddingEnd, 0), + align, + ] as const } const toOffset = diff --git a/packages/virtual-core/tests/index.test.ts b/packages/virtual-core/tests/index.test.ts index b9fc6996a..39cc62d45 100644 --- a/packages/virtual-core/tests/index.test.ts +++ b/packages/virtual-core/tests/index.test.ts @@ -3933,3 +3933,150 @@ test('#1258: cleanup drops a pending clamped write', () => { expect(virtualizer['_clampedAdjustment']).toBeNull() }) + +// ─── #1257: paddingEnd must not make scrollToIndex(last) overshoot the last item ───────────── +// When paddingEnd > 0, getOffsetForIndex(last, 'end') was returning the raw +// DOM max scroll offset (scrollHeight - clientHeight), which equals +// (content + paddingEnd - clientHeight). This caused scrollToIndex(last) to +// scroll past the rendered end of the last item. The fix uses +// getMaxScrollOffset() - paddingEnd, which equals +// (content - clientHeight) — the correct virtual max offset that keeps the +// last item flush with the bottom of the viewport. + +test('#1257: scrollToIndex(last) with paddingEnd keeps the last item flush with the viewport bottom', () => { + // 5 items × 50px = 250px content, paddingEnd = 80, scrollMargin = 0 + // viewport = 200px → total scrollHeight = 330px (250 + 80) + // Expected virtual max scroll offset: + // getMaxScrollOffset() - paddingEnd = (330 - 200) - 80 = 50 + // Without the fix (using raw scrollHeight - clientHeight): + // 330 - 200 = 130 → overshoots by 80px (exactly the paddingEnd) + const mockScrollElement = { + scrollTop: 0, + scrollLeft: 0, + scrollWidth: 200, + scrollHeight: 330, // 250 (content) + 80 (paddingEnd) + clientWidth: 200, + clientHeight: 200, + offsetWidth: 200, + offsetHeight: 200, + ownerDocument: { defaultView: globalThis }, + scrollTo: vi.fn(), + } as unknown as HTMLDivElement + + const scrollToFn = vi.fn() + const virtualizer = new Virtualizer({ + count: 5, + estimateSize: () => 50, + paddingEnd: 80, + getScrollElement: () => mockScrollElement, + scrollToFn, + observeElementRect: (_instance, cb) => { + cb({ width: 200, height: 200 }) + return () => {} + }, + observeElementOffset: (_instance, cb) => { + cb(0, false) + return () => {} + }, + }) + + virtualizer._willUpdate() + scrollToFn.mockClear() + + // Scroll to last item with 'end' alignment + virtualizer.scrollToIndex(4, { align: 'end' }) + + // getMaxScrollOffset() = 330 - 200 = 130; minus paddingEnd(80) = 50 + expect(scrollToFn).toHaveBeenCalledWith(50, expect.any(Object), expect.any(Object)) +}) + +// ─── #1263: getMaxScrollOffset() preserves lane-max for multi-lane layouts ───────────── +// The fix uses getMaxScrollOffset() directly (rather than getTotalSize()), which +// preserves the lane-max behavior added in #1105 (#1001 fix). In multi-lane layouts +// where the last item lives in a shorter lane, getMaxScrollOffset() still absorbs +// DOM extras that aren't in our measurements. + +test('#1263: scrollToIndex(last) in a shorter lane uses getMaxScrollOffset() lane-max', () => { + // 2 lanes: lane 0 = [0..3] @100px (ends at 400), lane 1 = [4] @50px (ends at 50). + // getTotalSize() = 400 (lane-max), getMaxScrollOffset() = 400 - 200 = 200. + // The fix uses getMaxScrollOffset() - paddingEnd = 200 - 0 = 200. + // This preserves the #1001 behavior where lane-max absorbs DOM extras. + const mockScrollElement = { + scrollTop: 0, + scrollLeft: 0, + scrollWidth: 200, + scrollHeight: 200, + clientWidth: 200, + clientHeight: 200, + offsetWidth: 200, + offsetHeight: 200, + ownerDocument: { defaultView: globalThis }, + scrollTo: vi.fn(), + } as unknown as HTMLDivElement + + const scrollToFn = vi.fn() + const virtualizer = new Virtualizer({ + count: 5, + estimateSize: (index) => (index === 4 ? 50 : 100), + lanes: 2, + getScrollElement: () => mockScrollElement, + scrollToFn, + observeElementRect: (_instance, cb) => { + cb({ width: 200, height: 200 }) + return () => {} + }, + observeElementOffset: (_instance, cb) => { + cb(0, false) + return () => {} + }, + }) + + virtualizer._willUpdate() + scrollToFn.mockClear() + + virtualizer.scrollToIndex(4, { align: 'end' }) + + // getMaxScrollOffset() = 200; minus paddingEnd(0) = 200 + expect(scrollToFn).toHaveBeenCalledWith(200, expect.any(Object), expect.any(Object)) +}) + +// ─── #1257: paddingEnd=0 is a no-op ───────────── +test('#1257: scrollToIndex(last) with paddingEnd=0 uses getMaxScrollOffset() unchanged', () => { + const mockScrollElement = { + scrollTop: 0, + scrollLeft: 0, + scrollWidth: 200, + scrollHeight: 250, // 5 × 50px content, no paddingEnd + clientWidth: 200, + clientHeight: 200, + offsetWidth: 200, + offsetHeight: 200, + ownerDocument: { defaultView: globalThis }, + scrollTo: vi.fn(), + } as unknown as HTMLDivElement + + const scrollToFn = vi.fn() + const virtualizer = new Virtualizer({ + count: 5, + estimateSize: () => 50, + paddingEnd: 0, + getScrollElement: () => mockScrollElement, + scrollToFn, + observeElementRect: (_instance, cb) => { + cb({ width: 200, height: 200 }) + return () => {} + }, + observeElementOffset: (_instance, cb) => { + cb(0, false) + return () => {} + }, + }) + + virtualizer._willUpdate() + scrollToFn.mockClear() + + virtualizer.scrollToIndex(4, { align: 'end' }) + + // getMaxScrollOffset() = 250 - 200 = 50; minus paddingEnd(0) = 50 + expect(scrollToFn).toHaveBeenCalledWith(50, expect.any(Object), expect.any(Object)) +})