diff --git a/.changeset/lanes-row-alignment.md b/.changeset/lanes-row-alignment.md new file mode 100644 index 000000000..d7e81619a --- /dev/null +++ b/.changeset/lanes-row-alignment.md @@ -0,0 +1,8 @@ +--- +'@tanstack/virtual-core': minor +--- + +fix(virtual-core): keep lanes aligned after changing `lanes` with `measureElement` (#1036) + +- `overscan` now counts whole rows when `lanes > 1`, so every lane renders (and measures) the same number of extra items. This renders more items than before: `lanes: 4, overscan: 2` now adds 8 items on each side instead of 2. `Range` gains an optional `lanes` field that `defaultRangeExtractor` reads; a custom `rangeExtractor` gets row overscan only if it delegates to `defaultRangeExtractor`. +- A ResizeObserver callback now measures all of its entries before notifying, so a synchronous re-render can no longer unmount part of a row before it is measured. diff --git a/docs/api/virtualizer.md b/docs/api/virtualizer.md index 0a910e25c..4d8885363 100644 --- a/docs/api/virtualizer.md +++ b/docs/api/virtualizer.md @@ -80,7 +80,7 @@ The sync parameter indicates whether scrolling is currently in progress. It is ` overscan?: number ``` -The number of items to render above and below the visible area. Increasing this number will increase the amount of time it takes to render the virtualizer, but might decrease the likelihood of seeing slow-rendering blank items at the top and bottom of the virtualizer when scrolling. The default value is `1`. +The number of items to render above and below the visible area. Increasing this number will increase the amount of time it takes to render the virtualizer, but might decrease the likelihood of seeing slow-rendering blank items at the top and bottom of the virtualizer when scrolling. The default value is `1`. When `lanes` is greater than `1`, overscan counts whole rows of `lanes` items, so every lane renders the same number of extra items. ### `horizontal` @@ -146,7 +146,7 @@ This function is passed the index of each item and should return a unique key fo rangeExtractor?: (range: Range) => number[] ``` -This function receives visible range indexes and should return array of indexes to render. This is useful if you need to add or remove items from the virtualizer manually regardless of the visible range, eg. rendering sticky items, headers, footers, etc. The default range extractor implementation will return the visible range indexes and is exported as `defaultRangeExtractor`. +This function receives visible range indexes and should return array of indexes to render. This is useful if you need to add or remove items from the virtualizer manually regardless of the visible range, eg. rendering sticky items, headers, footers, etc. The default range extractor implementation will return the visible range indexes and is exported as `defaultRangeExtractor`. When `lanes` is greater than `1`, `range.lanes` holds the lane count and `defaultRangeExtractor` applies `overscan` as whole rows; a custom extractor that computes its own indexes should do the same to keep lanes aligned. ### `scrollToFn` diff --git a/packages/react-virtual/e2e/app/lanes-change/index.html b/packages/react-virtual/e2e/app/lanes-change/index.html new file mode 100644 index 000000000..56f418f61 --- /dev/null +++ b/packages/react-virtual/e2e/app/lanes-change/index.html @@ -0,0 +1,10 @@ + + + + + + +
+ + + diff --git a/packages/react-virtual/e2e/app/lanes-change/main.tsx b/packages/react-virtual/e2e/app/lanes-change/main.tsx new file mode 100644 index 000000000..37adc028c --- /dev/null +++ b/packages/react-virtual/e2e/app/lanes-change/main.tsx @@ -0,0 +1,78 @@ +import React from 'react' +import ReactDOM from 'react-dom/client' +import { useVirtualizer } from '@tanstack/react-virtual' + +// Port of the reproduction in TanStack/virtual#1036: a grid of square cells +// whose height follows the lane width, so a lane change resizes every item. +const itemSize = 50 +const gap = 2 +const items = new Array(100) + +function App() { + const [lanes, setLanes] = React.useState(10) + const parentRef = React.useRef(null) + + const rowVirtualizer = useVirtualizer({ + count: items.length, + getScrollElement: () => parentRef.current, + estimateSize: () => itemSize, + lanes, + gap, + }) + ;(window as any).virtualizer = rowVirtualizer + + const visibleItems = rowVirtualizer.getVirtualItems() + + return ( + <> +
+
+ {visibleItems.map((virtualRow) => { + const widthPrc = 100 / rowVirtualizer.options.lanes + return ( +
+
+ {virtualRow.index} +
+
+ ) + })} +
+
+ + + ) +} + +ReactDOM.createRoot(document.getElementById('root')!).render() diff --git a/packages/react-virtual/e2e/app/test/lanes-change.spec.ts b/packages/react-virtual/e2e/app/test/lanes-change.spec.ts new file mode 100644 index 000000000..4e6bdd039 --- /dev/null +++ b/packages/react-virtual/e2e/app/test/lanes-change.spec.ts @@ -0,0 +1,54 @@ +import { expect, test } from '@playwright/test' +import type { Page } from '@playwright/test' + +// Layout of the rendered cells. `measured` is true once every rendered cell's +// virtual size (the wrapper height) matches the real height of its square +// content, i.e. the virtualizer has measured everything on screen. +// `misaligned` lists rows (index / lanes) whose cells don't share one start, +// meaning a lane drifted away from the others. +const layout = (page: Page) => + page.evaluate(() => { + const lanes = (window as any).virtualizer.options.lanes as number + const rows = new Map>() + let measured = true + for (const el of document.querySelectorAll('.cell')) { + const content = el.firstElementChild as HTMLElement + if (el.offsetHeight !== content.offsetHeight) measured = false + const row = Math.floor(Number(content.dataset.index) / lanes) + if (!rows.has(row)) rows.set(row, new Set()) + rows.get(row)!.add(Number(el.dataset.start)) + } + const misaligned = [...rows] + .filter(([, starts]) => starts.size > 1) + .map(([row, starts]) => `row ${row}: ${[...starts].join(', ')}`) + return { lanes, measured, misaligned } + }) + +test('rows stay aligned after changing lanes while scrolled to the end (#1036)', async ({ + page, +}) => { + await page.goto('/lanes-change/') + + // Scroll to the end until it settles: measurements can grow the list. + await expect(async () => { + const atEnd = await page.evaluate(() => { + const c = document.querySelector('#scroll-container')! + c.scrollTop = c.scrollHeight + return c.scrollTop + c.clientHeight >= c.scrollHeight - 1 + }) + expect(atEnd).toBe(true) + expect((await layout(page)).measured).toBe(true) + }).toPass() + + await page.click('[data-testid="toggle"]') + + // Right after the toggle every cell still has the same estimate, so rows + // are trivially aligned. Wait until the 5-lane sizes are measured. + await expect + .poll(async () => { + const { lanes, measured } = await layout(page) + return lanes === 5 && measured + }) + .toBe(true) + expect((await layout(page)).misaligned).toEqual([]) +}) diff --git a/packages/react-virtual/e2e/app/vite.config.ts b/packages/react-virtual/e2e/app/vite.config.ts index 87bc8b9d7..a7910bcf5 100644 --- a/packages/react-virtual/e2e/app/vite.config.ts +++ b/packages/react-virtual/e2e/app/vite.config.ts @@ -23,6 +23,7 @@ export default defineConfig({ __dirname, 'direct-dom-updates/index.html', ), + 'lanes-change': path.resolve(__dirname, 'lanes-change/index.html'), 'cached-measurements': path.resolve( __dirname, 'cached-measurements/index.html', diff --git a/packages/virtual-core/src/index.ts b/packages/virtual-core/src/index.ts index d7748de49..8a2896f1f 100644 --- a/packages/virtual-core/src/index.ts +++ b/packages/virtual-core/src/index.ts @@ -56,6 +56,11 @@ export interface Range { endIndex: number overscan: number count: number + /** + * Lane count of the virtualizer. When greater than 1, `overscan` counts + * rows of `lanes` items rather than single items. + */ + lanes?: number } type Key = number | string | bigint @@ -84,8 +89,13 @@ const getRect = (element: HTMLElement): Rect => { export const defaultKeyExtractor = (index: number) => index export const defaultRangeExtractor = (range: Range) => { - const start = Math.max(range.startIndex - range.overscan, 0) - const end = Math.min(range.endIndex + range.overscan, range.count - 1) + // With lanes, the visible range is already aligned to whole rows; overscan + // by whole rows too, so every lane gets the same number of items rendered + // (and measured). Overscanning single items leaves partial rows, letting one + // lane collect more measured sizes than the others and drift (#1036). + const overscan = range.overscan * Math.max(range.lanes ?? 1, 1) + const start = Math.max(range.startIndex - overscan, 0) + const end = Math.min(range.endIndex + overscan, range.count - 1) const len = end - start + 1 const arr = new Array(len) @@ -505,6 +515,9 @@ export class Virtualizer< instance: Virtualizer, ) => boolean) elementsCache = new Map() + // Set while a ResizeObserver callback measures its entries; `resizeItem` + // records the pending notify here instead of firing it per entry. + private _resizeBatch: { sync: boolean; notify?: boolean } | null = null private now = () => this.targetWindow?.performance?.now?.() ?? Date.now() private observer = (() => { let _ro: ResizeObserver | null = null @@ -519,46 +532,60 @@ export class Virtualizer< } return (_ro = new this.targetWindow.ResizeObserver((entries) => { - entries.forEach((entry) => { - const run = () => { - const node = entry.target as TItemElement - const index = this.indexFromElement(node) - - if (!node.isConnected) { - this.observer.unobserve(node) - // Find the cache entry pointing to this exact node and remove - // it. We can't call getItemKey(index) here because items may - // have been removed since this node was rendered — the index - // could be stale and out-of-bounds in the user's data array - // (regression test in e2e/.../stale-index.spec.ts, fix #1148). - // The === comparison naturally handles the React-replaced- - // a-node-for-the-same-key case: that entry now points to a - // different node, so this loop won't match. - for (const [cacheKey, cachedNode] of this.elementsCache) { - if (cachedNode === node) { - this.elementsCache.delete(cacheKey) - break - } - } - return - } - - if (!this.isIndexInRange(index)) return - - if (this.shouldMeasureDuringScroll(index)) { - this.resizeItem( - index, - this.options.measureElement(node, entry, this), - ) + const run = () => { + // Measure every entry before notifying. A notify that adjusted + // scroll re-renders synchronously, and that render can unmount + // nodes whose entries are still queued here, leaving them + // unmeasured. With lanes, a half-measured row lets one lane drift + // away from the others (#1036). + const batch: { sync: boolean; notify?: boolean } = { sync: false } + this._resizeBatch = batch + try { + entries.forEach(measureEntry) + } finally { + this._resizeBatch = null + if (batch.notify) { + this.notify(batch.sync) + this._retryClampedAdjustment() } } - this.options.useAnimationFrameWithResizeObserver - ? requestAnimationFrame(run) - : run() - }) + } + this.options.useAnimationFrameWithResizeObserver + ? requestAnimationFrame(run) + : run() })) } + const measureEntry = (entry: ResizeObserverEntry) => { + const node = entry.target as TItemElement + const index = this.indexFromElement(node) + + if (!node.isConnected) { + this.observer.unobserve(node) + // Find the cache entry pointing to this exact node and remove + // it. We can't call getItemKey(index) here because items may + // have been removed since this node was rendered — the index + // could be stale and out-of-bounds in the user's data array + // (regression test in e2e/.../stale-index.spec.ts, fix #1148). + // The === comparison naturally handles the React-replaced- + // a-node-for-the-same-key case: that entry now points to a + // different node, so this loop won't match. + for (const [cacheKey, cachedNode] of this.elementsCache) { + if (cachedNode === node) { + this.elementsCache.delete(cacheKey) + break + } + } + return + } + + if (!this.isIndexInRange(index)) return + + if (this.shouldMeasureDuringScroll(index)) { + this.resizeItem(index, this.options.measureElement(node, entry, this)) + } + } + return { disconnect: () => { get()?.disconnect() @@ -1610,9 +1637,10 @@ export class Virtualizer< this.options.count, startIndex, endIndex, + this.options.lanes, ] }, - (rangeExtractor, overscan, count, startIndex, endIndex) => { + (rangeExtractor, overscan, count, startIndex, endIndex, lanes) => { return startIndex === null || endIndex === null ? [] : rangeExtractor({ @@ -1620,6 +1648,7 @@ export class Virtualizer< endIndex, overscan, count, + lanes, }) }, { @@ -1718,6 +1747,14 @@ export class Virtualizer< resizeItem = (index: number, size: number) => { if (!this.isIndexInRange(index)) return + // An earlier entry in this ResizeObserver batch moved `scrollOffset`. + // Outside a batch its sync notify would have re-rendered and rebuilt the + // caches read below; rebuild them here so the anchoring check compares + // this item's current start, not a stale one, against the adjusted + // offset (#1218). Without a scroll move, the caches are exactly as stale + // as they were before batching, so skip the rebuild. + if (this._resizeBatch?.sync) this.getMeasurements() + // Fast field reads. For lanes===1 we read raw start/size from the flat // typed array, avoiding a Proxy.get + VirtualItem allocation per call. // For lanes>1 we fall back to the cached VirtualItem array. @@ -1809,6 +1846,13 @@ export class Virtualizer< // synchronous notify flushes the render in this same callback, so both // land in one paint. When nothing moved (or the write was deferred on // iOS), keep the cheaper async notify. + if (this._resizeBatch) { + // Inside a ResizeObserver callback: notify once after every entry + // is measured, synchronously if any of them moved `scrollTop`. + this._resizeBatch.notify = true + this._resizeBatch.sync ||= adjustedSync + return + } this.notify(adjustedSync) // A consumer that grows the sizer synchronously inside `onChange` // (direct DOM updates) may never re-render when the range is diff --git a/packages/virtual-core/tests/index.test.ts b/packages/virtual-core/tests/index.test.ts index db7851eed..e2511385b 100644 --- a/packages/virtual-core/tests/index.test.ts +++ b/packages/virtual-core/tests/index.test.ts @@ -1679,6 +1679,52 @@ test('defaultRangeExtractor: clamps end to count-1 when overscan would go past', expect(result[result.length - 1]).toBe(99) }) +test('defaultRangeExtractor: overscan expands by whole rows when lanes > 1', () => { + const result = defaultRangeExtractor({ + startIndex: 35, + endIndex: 54, + overscan: 1, + count: 100, + lanes: 5, + }) + expect(result[0]).toBe(30) + expect(result[result.length - 1]).toBe(59) + expect(result.length).toBe(30) +}) + +test('defaultRangeExtractor: row overscan clamps to the list bounds', () => { + const result = defaultRangeExtractor({ + startIndex: 0, + endIndex: 98, + overscan: 2, + count: 100, + lanes: 5, + }) + expect(result[0]).toBe(0) + expect(result[result.length - 1]).toBe(99) +}) + +test('multi-lane virtualizer renders whole rows with overscan (#1036)', () => { + const virtualizer = new Virtualizer({ + count: 100, + lanes: 5, + gap: 2, + overscan: 1, + estimateSize: () => 50, + initialRect: { width: 600, height: 200 }, + initialOffset: 418, + getScrollElement: () => null, + scrollToFn: vi.fn(), + observeElementRect: vi.fn(), + observeElementOffset: vi.fn(), + }) + const indexes = virtualizer.getVirtualIndexes() + // Every rendered row is complete, so every lane gets the same number of + // items measured. A partial row lets one lane drift from the others. + expect(indexes[0]! % 5).toBe(0) + expect(indexes[indexes.length - 1]! % 5).toBe(4) +}) + test('defaultRangeExtractor: single item range', () => { const result = defaultRangeExtractor({ startIndex: 5, @@ -4922,3 +4968,121 @@ test('#1257: scrollToEnd() with paddingEnd scrolls to getMaxScrollOffset()', () expect.any(Object), ) }) + +// ─── ResizeObserver batching ───────────────────────────────────────────────── + +function setupResizeBatch(onChange: (instance: any, sync: boolean) => void) { + let roCallback: ResizeObserverCallback | null = null + const mockWindow = { + requestAnimationFrame: vi.fn(), + cancelAnimationFrame: vi.fn(), + performance: { now: () => Date.now() }, + ResizeObserver: vi.fn(function (cb: ResizeObserverCallback) { + roCallback = cb + return { observe: vi.fn(), unobserve: vi.fn(), disconnect: vi.fn() } + }), + } + const el = { + scrollTop: 0, + scrollLeft: 0, + scrollWidth: 400, + scrollHeight: 100000, + offsetWidth: 400, + offsetHeight: 300, + clientHeight: 300, + ownerDocument: { defaultView: mockWindow }, + } as unknown as HTMLDivElement + const sizes = new Map() + let setOffset: (offset: number, isScrolling: boolean) => void = () => {} + const virtualizer = new Virtualizer({ + count: 20, + estimateSize: () => 90, + overscan: 0, + getScrollElement: () => el, + scrollToFn: (offset, { adjustments }) => { + el.scrollTop = offset + (adjustments ?? 0) + }, + measureElement: (node) => + sizes.get(Number((node as any).getAttribute('data-index')))!, + observeElementRect: (_instance, cb) => { + cb({ width: 400, height: 300 }) + return () => {} + }, + observeElementOffset: (_instance, cb) => { + setOffset = cb + cb(0, false) + return () => {} + }, + onChange, + }) + virtualizer._willUpdate() + virtualizer.getVirtualItems() + + const nodes = new Map() + const node = (i: number) => { + if (!nodes.has(i)) { + nodes.set(i, { + getAttribute: (name: string) => + name === 'data-index' ? String(i) : null, + isConnected: true, + }) + } + return nodes.get(i) + } + // Measure items 0..9 at 100px while at offset 0, then scroll to 450: + // item 3 spans 300..400 (entirely above), item 4 spans 400..500 (across + // the fold). + for (let i = 0; i < 10; i++) { + sizes.set(i, 100) + virtualizer.measureElement(node(i)) + } + el.scrollTop = 450 + setOffset(450, false) + virtualizer._willUpdate() + virtualizer.getVirtualItems() + + const deliver = (indexes: Array) => + roCallback!( + indexes.map( + (i) => ({ target: node(i) }) as unknown as ResizeObserverEntry, + ), + {} as ResizeObserver, + ) + return { virtualizer, sizes, node, deliver } +} + +test('ResizeObserver batch: later entries see starts updated by earlier ones (#1218)', () => { + const { virtualizer, sizes, deliver } = setupResizeBatch((instance) => { + instance.getVirtualItems() + }) + expect(virtualizer.getMeasurements()[4]!.start).toBe(400) + + // Item 3 grows above the fold: compensated (+100). Item 4 spans the fold, + // so its growth happens below the anchor point and must not move scroll. + sizes.set(3, 200) + sizes.set(4, 150) + deliver([3, 4]) + + expect(virtualizer.scrollOffset).toBe(550) +}) + +test('ResizeObserver batch: a sync re-render does not drop queued entries (#1036)', () => { + const notifies: Array = [] + const { virtualizer, sizes, node, deliver } = setupResizeBatch( + (instance, sync) => { + notifies.push(sync) + // A synchronous render that moves the range unmounts item 5. + if (sync) node(5).isConnected = false + instance.getVirtualItems() + }, + ) + + notifies.length = 0 + sizes.set(3, 200) + sizes.set(5, 140) + deliver([3, 5]) + + // Both entries are measured before the single (sync) notify. + expect(virtualizer.getMeasurements()[5]!.size).toBe(140) + expect(notifies).toEqual([true]) +})