diff --git a/.changeset/fix-scrolling-issue-1258.md b/.changeset/fix-scrolling-issue-1258.md new file mode 100644 index 000000000..503f7660b --- /dev/null +++ b/.changeset/fix-scrolling-issue-1258.md @@ -0,0 +1,5 @@ +--- +'@tanstack/virtual-core': patch +--- + +Recover the bottom pin when the browser clamps an end-anchored scroll compensation write. `resizeItem` compensates a size change by writing `scrollTop` before the consumer has committed the new total size, so when the grown item does not itself extend the scroll range the browser clamps the write to the old maximum and the viewport is left short of the end with no scroll event to correct it. Two cases hit this: `paddingEnd > 0` with a growing last item, where the overflowing item only extends `scrollHeight` to its own end and the clamp lands exactly `paddingEnd` short ([#1258](https://github.com/TanStack/virtual/issues/1258)); and a row above the last one growing while the last row keeps its size, under `directDomUpdates` ([#1266](https://github.com/TanStack/virtual/issues/1266)). A compensation write whose target exceeds the scroll maximum at write time is now recorded as clamped and re-issued once the sizer has grown — right after `notify` for consumers that size the container synchronously in `onChange`, and from `_willUpdate` for consumers that size it during a render. The clamped read-back keeps the retry pending; any other scroll event cancels it, so a user reading history is never yanked. diff --git a/packages/react-virtual/e2e/app/chat-resize/index.html b/packages/react-virtual/e2e/app/chat-resize/index.html new file mode 100644 index 000000000..56f418f61 --- /dev/null +++ b/packages/react-virtual/e2e/app/chat-resize/index.html @@ -0,0 +1,10 @@ + + + + + + +
+ + + diff --git a/packages/react-virtual/e2e/app/chat-resize/main.tsx b/packages/react-virtual/e2e/app/chat-resize/main.tsx new file mode 100644 index 000000000..2eb5dcbaa --- /dev/null +++ b/packages/react-virtual/e2e/app/chat-resize/main.tsx @@ -0,0 +1,89 @@ +// Exact reproduction of #1266, adapted from PR #1265 by @tigerBeA: an +// end-anchored direct-DOM chat where a row ABOVE the last one grows. The last +// row is exactly the viewport height and keeps its size, so its overflow cannot +// extend the scroll range before the sizer is updated, and with `useFlushSync: +// false` and an unchanged range nothing re-renders after the resize. +import React from 'react' +import { createRoot } from 'react-dom/client' +import { useVirtualizer } from '@tanstack/react-virtual' + +const VIEWPORT_HEIGHT = 300 +const initialMessages = Array.from({ length: 8 }, (_, index) => ({ + id: `m-${index}`, + height: index === 7 ? VIEWPORT_HEIGHT : 50, +})) + +function App() { + const [messages, setMessages] = React.useState(initialMessages) + const parentRef = React.useRef(null) + const virtualizer = useVirtualizer({ + count: messages.length, + getScrollElement: () => parentRef.current, + getItemKey: (index) => messages[index]!.id, + estimateSize: (index) => initialMessages[index]!.height, + anchorTo: 'end', + followOnAppend: true, + scrollEndThreshold: 4, + overscan: 4, + directDomUpdates: true, + useFlushSync: false, + }) + + React.useLayoutEffect(() => { + virtualizer.scrollToEnd() + }, [virtualizer]) + + return ( +
+ +
+
+ {virtualizer.getVirtualItems().map((item) => ( +
+
+ Message {messages[item.index]!.id} +
+
+ ))} +
+
+
+ ) +} + +createRoot(document.getElementById('root')!).render() diff --git a/packages/react-virtual/e2e/app/chat/main.tsx b/packages/react-virtual/e2e/app/chat/main.tsx index 497095812..e30fe0d75 100644 --- a/packages/react-virtual/e2e/app/chat/main.tsx +++ b/packages/react-virtual/e2e/app/chat/main.tsx @@ -25,6 +25,10 @@ function App() { const firstMessageIndexRef = React.useRef(0) const nextMessageIndexRef = React.useRef(initialMessages.length) + const paddingEnd = Number( + new URLSearchParams(window.location.search).get('paddingEnd') ?? 0, + ) + const virtualizer = useVirtualizer({ count: messages.length, getScrollElement: () => parentRef.current, @@ -34,6 +38,7 @@ function App() { followOnAppend: true, scrollEndThreshold: 4, overscan: 4, + paddingEnd, }) React.useLayoutEffect(() => { diff --git a/packages/react-virtual/e2e/app/test/chat.spec.ts b/packages/react-virtual/e2e/app/test/chat.spec.ts index b35b3333b..ed2075a39 100644 --- a/packages/react-virtual/e2e/app/test/chat.spec.ts +++ b/packages/react-virtual/e2e/app/test/chat.spec.ts @@ -158,3 +158,37 @@ test('chat mode keeps streaming bottom message pinned as it grows', async ({ await expect(page.locator('[data-testid="message-m-29"]')).toBeVisible() }) + +test('chat mode keeps streaming bottom message pinned as it grows with paddingEnd', async ({ + page, +}) => { + await page.goto('/chat/?paddingEnd=80') + await waitForEnd(page) + + await page.click('#grow-last') + await waitForEnd(page) + + await expect(page.locator('[data-testid="message-m-29"]')).toBeVisible() +}) + +// #1266 — adapted from PR #1265 by @tigerBeA. Direct DOM updates, no flushSync, +// and a row ABOVE the last one grows: the compensation write is clamped against +// the old scroll range, and with an unchanged range nothing re-renders afterwards, +// so only the post-notify retry in core can recover the lost distance. +test('direct DOM chat stays pinned when a previous message grows without a re-render', async ({ + page, +}) => { + await page.goto('/chat-resize/') + await waitForEnd(page) + // Let the initial scrollToEnd's isScrolling debounce settle first. Its reset + // triggers a re-render that would run _willUpdate and mask a missing retry. + await page.waitForTimeout(300) + const before = await getScrollState(page) + + await page.click('#grow-previous') + await expect + .poll(async () => (await getScrollState(page)).scrollHeight) + .toBe(before.scrollHeight + 24) + await waitForEnd(page) + expect((await getScrollState(page)).scrollTop).toBe(before.scrollTop + 24) +}) diff --git a/packages/react-virtual/e2e/app/vite.config.ts b/packages/react-virtual/e2e/app/vite.config.ts index e5594d0f2..505279170 100644 --- a/packages/react-virtual/e2e/app/vite.config.ts +++ b/packages/react-virtual/e2e/app/vite.config.ts @@ -11,6 +11,7 @@ export default defineConfig({ scroll: path.resolve(__dirname, 'scroll/index.html'), 'scroll-anchor': path.resolve(__dirname, 'scroll-anchor/index.html'), chat: path.resolve(__dirname, 'chat/index.html'), + 'chat-resize': path.resolve(__dirname, 'chat-resize/index.html'), 'measure-element': path.resolve( __dirname, 'measure-element/index.html', diff --git a/packages/virtual-core/src/index.ts b/packages/virtual-core/src/index.ts index dc6f1010c..55c276edf 100644 --- a/packages/virtual-core/src/index.ts +++ b/packages/virtual-core/src/index.ts @@ -447,6 +447,16 @@ export class Virtualizer< // value when the diff is < 1.5 px, distinguishing it from a real user // scroll. The +0.5 over Math.abs lets us also absorb the +1 / -1 cases. private _intendedScrollOffset: number | null = null + // A compensation write from `applyScrollAdjustment` whose target exceeded + // the element's scroll max at the moment of the write. The browser clamps + // such a write because the consumer's sizer has not grown yet: an + // end-anchored item growing at the bottom only extends `scrollHeight` to + // its own end, so with `paddingEnd > 0` the clamp lands exactly + // `paddingEnd` short of the target (#1258). `_willUpdate` re-issues the + // write once the sizer has caught up; the clamped read-back keeps it + // pending, any other scroll event (a real gesture) cancels it. + private _clampedAdjustment: { target: number; maxAtWrite: number } | null = + null shouldAdjustScrollPositionOnItemSizeChange: | undefined | (( @@ -706,6 +716,18 @@ export class Virtualizer< this._iosDeferredAdjustment += delta return false } else { + const target = this.getScrollOffset() + this.scrollAdjustments + delta + // Guarded so a bare test double without `scrollHeight` / `document` + // does not crash in `getMaxScrollOffset`. + const el = this.scrollElement + const maxAtWrite = + el !== null && ('scrollHeight' in el || 'document' in el) + ? this.getMaxScrollOffset() + : null + this._clampedAdjustment = + maxAtWrite !== null && target > maxAtWrite + 0.5 + ? { target, maxAtWrite } + : null this._scrollToOffset(this.getScrollOffset(), { adjustments: (this.scrollAdjustments += delta), behavior, @@ -787,6 +809,7 @@ export class Virtualizer< this._iosDeferredAdjustment = 0 this._iosTouching = false this._iosJustTouchEnded = false + this._clampedAdjustment = null this.scrollElement = null this.targetWindow = null } @@ -861,6 +884,17 @@ export class Virtualizer< } this._intendedScrollOffset = null + // A pending clamped compensation write (#1258) survives only its + // own read-back, which the browser reports at the scroll max we + // saw at write time. Anything else is a real gesture (or the write + // landing after all), so drop it rather than yank the user later. + if ( + this._clampedAdjustment !== null && + Math.abs(offset - this._clampedAdjustment.maxAtWrite) >= 1.5 + ) { + this._clampedAdjustment = null + } + this.scrollAdjustments = 0 // If the offset hasn't moved, this is the echo of our own // adjustment write — `applyScrollAdjustment` already folded it @@ -980,6 +1014,39 @@ export class Virtualizer< this.scrollToEnd({ behavior: followOnAppend }) } } + + // The consumer has committed the new total size by now, so a clamped + // compensation write may have room (#1258). + this._retryClampedAdjustment() + } + + // Re-issue a compensation write the browser clamped because the sizer had + // not grown yet (#1258, #1266). Called after `notify` in `resizeItem`, + // which covers consumers that size the container synchronously inside + // `onChange` (direct DOM updates, flushSync renders — where no re-render + // may follow at all), and from `_willUpdate` for consumers that size it + // during an asynchronous render. Both the clamped read-back and the + // absence of one leave `_clampedAdjustment` set, so timing does not matter. + private _retryClampedAdjustment = () => { + if ( + this._clampedAdjustment === null || + !this.scrollElement || + !this.options.enabled + ) { + return + } + const { target, maxAtWrite } = this._clampedAdjustment + const max = this.getMaxScrollOffset() + if (max > maxAtWrite + 0.5) { + // Still short (the sizer grew only partially): stay pending against + // the new max so the next opportunity retries. + this._clampedAdjustment = + target > max + 0.5 ? { target, maxAtWrite: max } : null + this._scrollToOffset(target, { + adjustments: undefined, + behavior: undefined, + }) + } } // Apply any accumulated iOS-deferred scroll adjustment, but only when we're @@ -1646,6 +1713,11 @@ export class Virtualizer< // land in one paint. When nothing moved (or the write was deferred on // iOS), keep the cheaper async notify. this.notify(adjustedSync) + // A consumer that grows the sizer synchronously inside `onChange` + // (direct DOM updates) may never re-render when the range is + // unchanged, so retry a clamped write here rather than only in + // `_willUpdate` (#1266). + this._retryClampedAdjustment() } } diff --git a/packages/virtual-core/tests/index.test.ts b/packages/virtual-core/tests/index.test.ts index 05a674663..b9fc6996a 100644 --- a/packages/virtual-core/tests/index.test.ts +++ b/packages/virtual-core/tests/index.test.ts @@ -2879,6 +2879,7 @@ function createChatVirtualizer({ itemSize = 50, followOnAppend = false, threshold = 1, + paddingEnd = 0, }: { messages: Array<{ id: string }> offset: number @@ -2886,13 +2887,16 @@ function createChatVirtualizer({ itemSize?: number followOnAppend?: boolean | 'auto' | 'smooth' | 'instant' threshold?: number + paddingEnd?: number }) { let currentMessages = messages const scrollToFn = vi.fn() + let offsetCb: ((offset: number, isScrolling: boolean) => void) | null = null const scrollElement = { scrollTop: offset, scrollLeft: 0, - scrollHeight: messages.length * itemSize, + // The sizer is `getTotalSize()`, which includes paddingEnd. + scrollHeight: messages.length * itemSize + paddingEnd, scrollWidth: 1000, clientHeight: viewportSize, clientWidth: 400, @@ -2935,12 +2939,14 @@ function createChatVirtualizer({ _instance: any, cb: (offset: number, isScrolling: boolean) => void, ) => { + offsetCb = cb cb(scrollElement.scrollTop, false) return () => {} }, anchorTo: 'end' as const, followOnAppend, scrollEndThreshold: threshold, + paddingEnd, } } @@ -2956,9 +2962,15 @@ function createChatVirtualizer({ setMessages(nextMessages: Array<{ id: string }>) { currentMessages = nextMessages virtualizer.setOptions(makeOptions()) - ;(scrollElement as any).scrollHeight = nextMessages.length * itemSize + ;(scrollElement as any).scrollHeight = + nextMessages.length * itemSize + paddingEnd virtualizer._willUpdate() }, + // Simulate the browser's scroll event after a write (or a gesture). + emitScroll(nextOffset: number, isScrolling = false) { + scrollElement.scrollTop = nextOffset + offsetCb?.(nextOffset, isScrolling) + }, } } @@ -3753,3 +3765,171 @@ test('#1218: first measurement of a spanning item still compensates', () => { expect(v.scrollOffset).toBe(before + 70) }) + +// ─── #1258: compensation write clamped by a not-yet-grown sizer ───────────── +// 5 items × 50px + paddingEnd 80 = 330px sizer, 200px viewport → the bottom is +// scrollTop 130. When the last item grows by 70, resizeItem writes 200, but +// the consumer has not committed the 400px sizer yet, so the browser clamps +// the write to the current max (130) — exactly paddingEnd short once the +// overflowing item alone has extended scrollHeight. The write must be +// re-issued once the sizer has grown. + +function clampedGrowthSetup() { + const messages = Array.from({ length: 5 }, (_, i) => ({ id: `m-${i}` })) + const setup = createChatVirtualizer({ + messages, + offset: 130, + paddingEnd: 80, + threshold: 0, + }) + setup.virtualizer.resizeItem(4, 120) + // The end-anchor compensation fired against the not-yet-grown sizer. + expect(setup.scrollToFn).toHaveBeenCalledTimes(1) + expect(setup.scrollToFn.mock.calls[0]![1].adjustments).toBe(70) + return setup +} + +test('#1258: clamped end-anchor write is re-issued after the read-back once the sizer grows', () => { + const { virtualizer, scrollElement, scrollToFn, emitScroll } = + clampedGrowthSetup() + + // Browser read-back of the clamped write: it landed on the old max. + emitScroll(130) + // Consumer commits the grown sizer, then the layout effect runs. + ;(scrollElement as any).scrollHeight = 400 + virtualizer._willUpdate() + + expect(scrollToFn).toHaveBeenCalledTimes(2) + expect(scrollToFn.mock.calls[1]![0]).toBe(200) +}) + +test('#1258: clamped end-anchor write is re-issued in a sync render before the read-back', () => { + const { virtualizer, scrollElement, scrollToFn } = clampedGrowthSetup() + + // flushSync path: the layout effect runs before the scroll event arrives. + ;(scrollElement as any).scrollHeight = 400 + virtualizer._willUpdate() + + expect(scrollToFn).toHaveBeenCalledTimes(2) + expect(scrollToFn.mock.calls[1]![0]).toBe(200) +}) + +test('#1258: a render without sizer growth does not re-issue; a later one does', () => { + const { virtualizer, scrollElement, scrollToFn, emitScroll } = + clampedGrowthSetup() + + emitScroll(130) + virtualizer._willUpdate() + expect(scrollToFn).toHaveBeenCalledTimes(1) + ;(scrollElement as any).scrollHeight = 400 + virtualizer._willUpdate() + expect(scrollToFn).toHaveBeenCalledTimes(2) +}) + +test('#1258: a real gesture after the clamp cancels the pending re-issue', () => { + const { virtualizer, scrollElement, scrollToFn, emitScroll } = + clampedGrowthSetup() + + emitScroll(130) + // The user scrolls up to read history. + emitScroll(60, true) + ;(scrollElement as any).scrollHeight = 400 + virtualizer._willUpdate() + + expect(scrollToFn).toHaveBeenCalledTimes(1) +}) + +test('#1258: an unclamped compensation write is not re-issued when the sizer later grows', () => { + // 8 × 50 = 400px, viewport 200, user reading at 100. Item 0 (above the + // fold) re-measures +10 → compensation writes 110, well within max 200. + const messages = Array.from({ length: 8 }, (_, i) => ({ id: `m-${i}` })) + const { virtualizer, scrollElement, scrollToFn, emitScroll } = + createChatVirtualizer({ messages, offset: 100 }) + + virtualizer.resizeItem(0, 60) + expect(scrollToFn).toHaveBeenCalledTimes(1) + emitScroll(110) + ;(scrollElement as any).scrollHeight = 450 + virtualizer._willUpdate() + + expect(scrollToFn).toHaveBeenCalledTimes(1) +}) + +test('#1266: a consumer that grows the sizer synchronously in onChange gets the clamped write re-issued without a re-render', () => { + // Direct DOM updates: the adapter sets the container height inside + // onChange, and with an unchanged range nothing re-renders afterwards. + const messages = Array.from({ length: 5 }, (_, i) => ({ id: `m-${i}` })) + const { virtualizer, scrollElement, scrollToFn } = createChatVirtualizer({ + messages, + offset: 130, + paddingEnd: 80, + threshold: 0, + }) + virtualizer.options.onChange = (instance) => { + ;(scrollElement as any).scrollHeight = instance.getTotalSize() + } + + virtualizer.resizeItem(4, 120) + + expect(scrollToFn).toHaveBeenCalledTimes(2) + expect(scrollToFn.mock.calls[0]![1].adjustments).toBe(70) + expect(scrollToFn.mock.calls[1]![0]).toBe(200) + expect(virtualizer['_clampedAdjustment']).toBeNull() +}) + +test('#1258: both retry sites firing in one sync pass write exactly once', () => { + // flushSync consumer: onChange grows the sizer AND runs the layout effect + // (_willUpdate) synchronously inside notify. The retry after notify must then + // find nothing pending — no double write. + const messages = Array.from({ length: 5 }, (_, i) => ({ id: `m-${i}` })) + const { virtualizer, scrollElement, scrollToFn } = createChatVirtualizer({ + messages, + offset: 130, + paddingEnd: 80, + threshold: 0, + }) + virtualizer.options.onChange = (instance) => { + ;(scrollElement as any).scrollHeight = instance.getTotalSize() + instance._willUpdate() + } + + virtualizer.resizeItem(4, 120) + + expect(scrollToFn).toHaveBeenCalledTimes(2) + expect(scrollToFn.mock.calls[1]![0]).toBe(200) + expect(virtualizer['_clampedAdjustment']).toBeNull() +}) + +test('#1258: a partially grown sizer re-issues and stays pending until the target fits', () => { + const { virtualizer, scrollElement, scrollToFn, emitScroll } = + clampedGrowthSetup() + emitScroll(130) + + // Sizer grew only to 360 → max 160, still short of the 200 target. + ;(scrollElement as any).scrollHeight = 360 + virtualizer._willUpdate() + expect(scrollToFn).toHaveBeenCalledTimes(2) + expect(scrollToFn.mock.calls[1]![0]).toBe(200) + expect(virtualizer['_clampedAdjustment']).toEqual({ + target: 200, + maxAtWrite: 160, + }) + + // The browser clamps that write to the new max; its read-back keeps it pending. + emitScroll(160) + expect(virtualizer['_clampedAdjustment']).not.toBeNull() + ;(scrollElement as any).scrollHeight = 400 + virtualizer._willUpdate() + expect(scrollToFn).toHaveBeenCalledTimes(3) + expect(scrollToFn.mock.calls[2]![0]).toBe(200) + expect(virtualizer['_clampedAdjustment']).toBeNull() +}) + +test('#1258: cleanup drops a pending clamped write', () => { + const { virtualizer } = clampedGrowthSetup() + expect(virtualizer['_clampedAdjustment']).not.toBeNull() + + virtualizer['cleanup']() + + expect(virtualizer['_clampedAdjustment']).toBeNull() +})