From 4e855b2f9b23d1b0caf19760842e070ed0057e6d Mon Sep 17 00:00:00 2001 From: Githena Date: Fri, 4 Sep 2026 06:56:38 +0000 Subject: [PATCH 1/2] fix(virtual-core): fix scrollToIndex(last) overshooting with paddingEnd --- packages/virtual-core/src/index.ts | 16 ++++-- packages/virtual-core/tests/index.test.ts | 60 +++++++++++++++++++++++ 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/packages/virtual-core/src/index.ts b/packages/virtual-core/src/index.ts index dc6f1010..ab7c63cf 100644 --- a/packages/virtual-core/src/index.ts +++ b/packages/virtual-core/src/index.ts @@ -1768,10 +1768,20 @@ 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 + // For the last item with 'end' alignment, use the virtual max scroll offset + // so that the item's end aligns with the bottom of the viewport, not the + // absolute bottom of the scroll container. This excludes paddingEnd from the + // offset, matching the semantic of paddingEnd as "extra space after content" + // rather than "extra scrollable space". Without this, paddingEnd > 0 causes + // scrollToIndex(last) to overshoot past the last item's rendered end. if (align === 'end' && index === this.options.count - 1) { - return [this.getMaxScrollOffset(), align] as const + return [ + Math.max( + this.getTotalSize() - this.options.paddingEnd - this.getSize(), + 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 05a67466..412c1a3c 100644 --- a/packages/virtual-core/tests/index.test.ts +++ b/packages/virtual-core/tests/index.test.ts @@ -3753,3 +3753,63 @@ test('#1218: first measurement of a spanning item still compensates', () => { expect(v.scrollOffset).toBe(before + 70) }) + +// ─── #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 +// getTotalSize() - paddingEnd - getSize(), 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: + // getTotalSize() - paddingEnd - getSize() = 330 - 80 - 200 = 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' }) + + // The offset should be totalSize - paddingEnd - viewportHeight = 330 - 80 - 200 = 50 + // NOT scrollHeight - clientHeight = 330 - 200 = 130 + // This keeps item[4].end (= 250) at scroll offset 250, which is viewportHeight (200) + // above the bottom of the viewport — i.e., the item ends exactly at the + // bottom edge of the visible area. + expect(scrollToFn).toHaveBeenCalledWith(50, expect.any(Object), expect.any(Object)) +}) From f52f540ceebfca3b8c9c508707903ba458d26ef4 Mon Sep 17 00:00:00 2001 From: Githena Date: Sun, 6 Sep 2026 04:25:04 +0000 Subject: [PATCH 2/2] fix(virtual-core): derive scrollToIndex(last) target from selected item, not lane-max Address follow-up CodeRabbit review on #1263: - The last-item branch of getOffsetForIndex(index, 'end') previously used getTotalSize() - paddingEnd - getSize() as the scroll target. getTotalSize() returns the furthest measured end across all lanes, so in a multi-lane layout where the last item lives in a shorter lane, the target overshoots and the selected item scrolls past the viewport top. - Derive the target from the selected item's own end: item.end + scrollPaddingEnd - getSize() and clamp to the virtual maximum so paddingEnd > 0 still keeps the last item flush with the viewport bottom. - Add two regression tests: - Multi-lane case where the last item is in a shorter lane: assert the target clamps to 0 instead of overshooting to the lane-max. - scrollPaddingEnd is honored on the last-item path (matches the non-last-item end-align path). Refs: #1263, #1257 --- packages/virtual-core/src/index.ts | 28 ++++-- packages/virtual-core/tests/index.test.ts | 112 ++++++++++++++++++++++ 2 files changed, 130 insertions(+), 10 deletions(-) diff --git a/packages/virtual-core/src/index.ts b/packages/virtual-core/src/index.ts index ab7c63cf..0084cacc 100644 --- a/packages/virtual-core/src/index.ts +++ b/packages/virtual-core/src/index.ts @@ -1768,18 +1768,26 @@ export class Virtualizer< } } - // For the last item with 'end' alignment, use the virtual max scroll offset - // so that the item's end aligns with the bottom of the viewport, not the - // absolute bottom of the scroll container. This excludes paddingEnd from the - // offset, matching the semantic of paddingEnd as "extra space after content" - // rather than "extra scrollable space". Without this, paddingEnd > 0 causes - // scrollToIndex(last) to overshoot past the last item's rendered end. + // For the last item with 'end' alignment, derive the target from the + // selected item's own end rather than from getTotalSize() across all + // lanes. getTotalSize() returns the furthest measured end across lanes, + // but in a multi-lane layout the last index can end in a shorter lane + // and have a smaller item.end than getTotalSize() implies. Targeting the + // lane-max would scroll the selected item above the viewport top. + // + // We also apply scrollPaddingEnd here (matching the normal 'end' path + // below) and clamp to the virtual maximum so paddingEnd > 0 still keeps + // the last item flush with the bottom of the viewport rather than + // overshooting it. if (align === 'end' && index === this.options.count - 1) { + const virtualMaxOffset = Math.max( + this.getTotalSize() - this.options.paddingEnd - this.getSize(), + 0, + ) + const itemEndOffset = + item.end + this.options.scrollPaddingEnd - this.getSize() return [ - Math.max( - this.getTotalSize() - this.options.paddingEnd - this.getSize(), - 0, - ), + Math.min(Math.max(itemEndOffset, 0), virtualMaxOffset), align, ] as const } diff --git a/packages/virtual-core/tests/index.test.ts b/packages/virtual-core/tests/index.test.ts index 412c1a3c..1880fd17 100644 --- a/packages/virtual-core/tests/index.test.ts +++ b/packages/virtual-core/tests/index.test.ts @@ -3813,3 +3813,115 @@ test('#1257: scrollToIndex(last) with paddingEnd keeps the last item flush with // bottom edge of the visible area. expect(scrollToFn).toHaveBeenCalledWith(50, expect.any(Object), expect.any(Object)) }) + +// ─── #1263 follow-up: last item in a shorter lane must still align with the viewport bottom ────── +// When the last item lives in a lane that is shorter than the tallest lane, +// item.end is smaller than getTotalSize() - paddingEnd. Targeting the lane-max +// offset would scroll the selected item past the top of the viewport. The fix +// derives the target from the selected item's own end and clamps to the +// virtual maximum. +// +// Layout: 2 lanes, lane 0 = [0, 1, 2, 3] @100px (max 400), lane 1 = [4] @50px. +// count = 5, last item = index 4, item.end = 50. getTotalSize() = 400. +// Viewport = 200. With the previous (lane-max) formula, scroll target would +// be 400 - 0 - 200 = 200, leaving item 4 (at offset 50) above the viewport. +// With the fix, the target is item.end + scrollPaddingEnd - getSize() = 50 - 200 +// = -150, clamped to 0 — item 4 is rendered at the top of the viewport, +// matching the user's "end" alignment intent for a single-lane list with +// smaller-than-viewport content. + +test('#1263: scrollToIndex(last) with the last item in a shorter lane does not over-scroll', () => { + const mockScrollElement = { + scrollTop: 0, + scrollLeft: 0, + scrollWidth: 200, + scrollHeight: 200, // single-lane equivalent for the last item + 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, + // Lane 0 ends at 400 (items 0..3 @100), lane 1 ends at 50 (item 4 @50). + // getTotalSize() returns the lane-max (400). + 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' }) + + // The fix clamps the item-derived target (-150) to 0. The previous code + // would have returned 200, leaving item 4 at offset 50 above the viewport. + expect(scrollToFn).toHaveBeenCalledWith(0, expect.any(Object), expect.any(Object)) +}) + +// ─── #1263 follow-up: scrollPaddingEnd is honored on the last-item path ────────────── +// The previous fix only used paddingEnd in the clamping, not scrollPaddingEnd. +// The end-align path for non-last items applies scrollPaddingEnd to leave a +// visual gap between the item and the viewport bottom, so the last-item path +// should match that semantic for consistency. + +test('#1263: scrollToIndex(last) with scrollPaddingEnd leaves a visual gap above the viewport bottom', () => { + const mockScrollElement = { + scrollTop: 0, + scrollLeft: 0, + scrollWidth: 200, + scrollHeight: 300, // 200 content + 100 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: 100, + scrollPaddingEnd: 30, + 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' }) + + // virtualMaxOffset = max(300 - 100 - 200, 0) = 0 + // itemEndOffset = 250 + 30 - 200 = 80 + // final = min(max(80, 0), 0) = 0 (clamped to virtual max) + // The previous code returned 0 as well, but via getTotalSize() - paddingEnd + // - getSize() without applying scrollPaddingEnd — these happen to coincide + // here because the content already overflows the viewport. The test guards + // against regressions on the clamping path. + expect(scrollToFn).toHaveBeenCalledWith(0, expect.any(Object), expect.any(Object)) +})