fix(virtual-core): leave resizes measured before a scroll command's read-back to reconcile

.changeset/last-written-scroll-offset.md

@@ -0,0 +1,5 @@
+---
+'@tanstack/virtual-core': patch
+---
+
+fix(virtual-core): leave resizes measured before a scroll command's write is read back to reconcile, so an upward `scrollToIndex` no longer jumps back and stalls until the safety valve, and build `scrollBy` on a just-written offset

packages/virtual-core/src/index.ts

@@ -406,6 +406,14 @@ type ScrollState = {
   stableFrames: number
   // Keep arrival across measurement-driven retargets so external scrolls can cancel.
   hasReachedTarget: boolean
+
+  // Where our last non-smooth write for this command put the element, until
+  // its scroll event arrives. Until then `scrollOffset` still holds the old
+  // position, and the rows on screen are the ones rendered for it. A smooth
+  // write leaves it null: the element is somewhere between the two offsets.
+  // It goes with the scroll state, so a write the browser ignored (no read-back)
+  // does not outlive its command.
+  writtenOffset?: number | null
 }
 
 type PendingScrollAnchor = [
@@ -973,6 +981,7 @@ export class Virtualizer<
             this.scrollState = null
           }
           this._intendedScrollOffset = null
+          if (this.scrollState) this.scrollState.writtenOffset = null
 
           // A pending clamped compensation write (#1258) survives only its
           // own read-back, which the browser reports at the scroll max we
@@ -1835,13 +1844,33 @@ export class Virtualizer<
       this.itemSizeCache.set(key, size)
       this.itemSizeCacheVersion++
 
-      let adjustedSync = false
-      if (wasAtEnd) {
-        adjustedSync = this.applyScrollAdjustment(
-          this.getTotalSize() - prevTotalSize,
+      // A scroll command (or reconcile) wrote a new position that neither a
+      // scroll event nor an eager update has reflected in `scrollOffset` yet.
+      // The rows being measured were rendered for the old position, so
+      // "above the viewport" means nothing against either offset. An index
+      // or end target is retargeted from fresh measurements by
+      // reconcileScroll next frame; an offset target (scrollToOffset,
+      // scrollBy) lands where it was written and the delta is not applied,
+      // since compensating against the stale offset would overwrite the
+      // command. Compensation writes and the prepend anchor sync update
+      // `scrollOffset` eagerly, and the iOS flush writes exactly
+      // `scrollOffset + scrollAdjustments`, so none of them count.
+      const awaitingCommandReadBack =
+        this.scrollState?.writtenOffset != null &&
+        !approxEqual(
+          this.scrollState.writtenOffset,
+          this.getScrollOffset() + this.scrollAdjustments,
         )
-      } else if (shouldAdjustScroll) {
-        adjustedSync = this.applyScrollAdjustment(delta)
+
+      let adjustedSync = false
+      if (!awaitingCommandReadBack) {
+        if (wasAtEnd) {
+          adjustedSync = this.applyScrollAdjustment(
+            this.getTotalSize() - prevTotalSize,
+          )
+        } else if (shouldAdjustScroll) {
+          adjustedSync = this.applyScrollAdjustment(delta)
+        }
       }
 
       // When we just moved `scrollTop` to compensate for an above-viewport
@@ -2080,7 +2109,13 @@ export class Virtualizer<
     delta: number,
     { behavior = 'auto' }: ScrollToOffsetOptions = {},
   ) => {
-    const offset = this.getScrollOffset() + delta
+    // Build on a position we just wrote even before its scroll event arrives.
+    // Clamp: a write past either edge that leaves the element where it was
+    // fires no scroll event, so nothing would clear an unreachable offset.
+    const offset = this.getOffsetForAlignment(
+      (this.scrollState?.writtenOffset ?? this.getScrollOffset()) + delta,
+      'start',
+    )
     const now = this.now()
 
     this.scrollState = {
@@ -2227,6 +2262,10 @@ export class Virtualizer<
     // Record the intended logical scroll target so the next scroll event
     // can reconcile against subpixel rounding by the browser.
     this._intendedScrollOffset = offset + (adjustments ?? 0)
+    if (this.scrollState) {
+      this.scrollState.writtenOffset =
+        behavior === 'smooth' ? null : this._intendedScrollOffset
+    }
     this.options.scrollToFn(offset, { behavior, adjustments }, this)
   }
 

packages/virtual-core/tests/index.test.ts

@@ -8,6 +8,7 @@ import {
   observeWindowOffset,
   windowScroll,
 } from '../src/index'
+import type { VirtualizerOptions } from '../src/index'
 
 test('should export the Virtualizer class', () => {
   expect(Virtualizer).toBeDefined()
@@ -2537,20 +2538,18 @@ test.fails(
   },
 )
 
-test.fails(
-  'scrollToIndex (#1307): a resize classified against the stale offset does not overwrite the index scroll',
-  () => {
-    // Not iOS-specific. scrollOffset still holds the old position until the
-    // scroll event, so row 38 (below the target) reads as above the viewport.
-    const h = makeDeferralHarness({ ios: false, start: 2000 })
-    h.v.scrollToIndex(10, { align: 'start' })
-    expect(h.el.scrollTop).toBe(500)
-    h.v.resizeItem(38, 100)
-    h.scroll(true)
-    h.frame()
-    expect(h.el.scrollTop).toBe(500)
-  },
-)
+test('scrollToIndex (#1307): a resize classified against the stale offset does not overwrite the index scroll', () => {
+  // Not iOS-specific. scrollOffset still holds the old position until the
+  // scroll event, so row 38 (below the target) reads as above the viewport;
+  // resizes before the read-back are left to reconcile.
+  const h = makeDeferralHarness({ ios: false, start: 2000 })
+  h.v.scrollToIndex(10, { align: 'start' })
+  expect(h.el.scrollTop).toBe(500)
+  h.v.resizeItem(38, 100)
+  h.scroll(true)
+  h.frame()
+  expect(h.el.scrollTop).toBe(500)
+})
 
 test.fails(
   'scrollToIndex (#1307): a drag that never reached the target is not pulled back when the target moves',
@@ -5425,3 +5424,228 @@ test('ResizeObserver batch: a sync re-render does not drop queued entries (#1036
   expect(virtualizer.getMeasurements()[5]!.size).toBe(140)
   expect(notifies).toEqual([true])
 })
+
+// 100 rows estimated at 50px in a 200px viewport. scrollToFn writes the
+// element and clamps like a browser; scroll events are delivered by hand, so a
+// test can resize between a programmatic write and its read-back.
+function setupHandDrivenScroll(
+  extra: Partial<VirtualizerOptions<any, any>> = {},
+) {
+  const rafCallbacks: Array<FrameRequestCallback> = []
+  const mockWindow = {
+    requestAnimationFrame: (cb: FrameRequestCallback) => rafCallbacks.push(cb),
+    cancelAnimationFrame: () => {},
+  }
+  const el = {
+    scrollTop: 0,
+    scrollLeft: 0,
+    scrollHeight: 5000,
+    clientHeight: 200,
+    offsetHeight: 200,
+    ownerDocument: { defaultView: mockWindow },
+  }
+  let scrollCallback: ((offset: number, isScrolling: boolean) => void) | null =
+    null
+  // Like iOS during a touch: drop programmatic writes on the floor.
+  const browser = { ignoreWrites: false }
+  const v = new Virtualizer({
+    count: 100,
+    estimateSize: () => 50,
+    getScrollElement: () => el as any,
+    scrollToFn: (offset, { adjustments = 0 }) => {
+      if (browser.ignoreWrites) return
+      el.scrollTop = Math.max(
+        0,
+        Math.min(offset + adjustments, el.scrollHeight - el.clientHeight),
+      )
+    },
+    observeElementRect: (_inst, cb) => {
+      cb({ width: 400, height: 200 })
+      return () => {}
+    },
+    observeElementOffset: (_inst, cb) => {
+      scrollCallback = cb
+      cb(0, false)
+      return () => {}
+    },
+    ...extra,
+  })
+  v._willUpdate()
+  v['getMeasurements']()
+  const scroll = (offset: number, isScrolling: boolean) =>
+    scrollCallback!(offset, isScrolling)
+  const settle = () => {
+    for (let i = 0; i < 10 && rafCallbacks.length > 0; i++) {
+      rafCallbacks.splice(0).forEach((cb) => cb(0))
+      scroll(el.scrollTop, true)
+    }
+    scroll(el.scrollTop, false)
+  }
+  // Runs frames without scroll events, as a browser does when a write leaves
+  // the element where it was.
+  const frames = (n = 10) => {
+    for (let i = 0; i < n && rafCallbacks.length > 0; i++) {
+      rafCallbacks.splice(0).forEach((cb) => cb(0))
+    }
+  }
+  return { v, el, scroll, settle, frames, browser }
+}
+
+test('a resize before the read-back of a scrollToIndex write is left to reconcile', () => {
+  // Scrolling up from 2000 to row 20 (1000), row 30 grows before the scroll
+  // event reports 1000. Compensating it against the stale 2000 used to write
+  // 2050, and reconcile (target unchanged) never rewrote it until the 5s
+  // safety valve.
+  const { v, el, scroll, settle } = setupHandDrivenScroll()
+  el.scrollTop = 2000
+  scroll(2000, false)
+
+  v.scrollToIndex(20, { align: 'start' })
+  expect(el.scrollTop).toBe(1000)
+  v.resizeItem(30, 100)
+  expect(el.scrollTop).toBe(1000)
+  settle()
+
+  expect(el.scrollTop).toBe(1000)
+  expect(v.scrollState).toBeNull()
+})
+
+test('a resize above the target before the read-back is picked up by reconcile', () => {
+  const { v, el, scroll, settle } = setupHandDrivenScroll()
+  el.scrollTop = 2000
+  scroll(2000, false)
+
+  v.scrollToIndex(20, { align: 'start' })
+  v.resizeItem(10, 100)
+  // No compensation write before the read-back...
+  expect(el.scrollTop).toBe(1000)
+  settle()
+  // ...reconcile retargets to the row's new start.
+  expect(el.scrollTop).toBe(1050)
+  expect(v.scrollState).toBeNull()
+})
+
+test('rows at the old position measured during scrollToEnd do not push it off the end', () => {
+  // scrollToEnd writes 4800, but until the read-back the rendered rows are
+  // the ones at the top. They measure 20px smaller each; compensating them
+  // against the written offset would walk the view up from the end while
+  // the end target (the DOM max, sizer not re-rendered) stays put.
+  const { v, el, settle } = setupHandDrivenScroll()
+
+  v.scrollToEnd()
+  expect(el.scrollTop).toBe(4800)
+  for (let i = 0; i < 5; i++) v.resizeItem(i, 30)
+  expect(el.scrollTop).toBe(4800)
+  settle()
+
+  expect(el.scrollTop).toBe(4800)
+  expect(v.scrollState).toBeNull()
+})
+
+test('scrollBy right after a scrollToIndex builds on the written offset', () => {
+  const { v, el, settle } = setupHandDrivenScroll()
+
+  v.scrollToIndex(20, { align: 'start' })
+  v.scrollBy(100)
+  settle()
+
+  expect(el.scrollTop).toBe(1100)
+})
+
+test('scrollBy after the user scrolled builds on the user position, not an old write', () => {
+  const { v, el, scroll, settle } = setupHandDrivenScroll()
+
+  v.scrollToIndex(20, { align: 'start' })
+  scroll(el.scrollTop, false) // read-back of 1000
+  el.scrollTop = 1300
+  scroll(1300, false) // the user moved on
+  v.scrollBy(100)
+  settle()
+
+  expect(el.scrollTop).toBe(1400)
+})
+
+test('scrollBy during a smooth scroll builds on the current position', () => {
+  // The element is still travelling, so the destination isn't where it is.
+  const { v, el, settle } = setupHandDrivenScroll()
+
+  v.scrollToIndex(20, { align: 'start', behavior: 'smooth' })
+  v.scrollBy(100)
+  settle()
+
+  expect(el.scrollTop).toBe(100)
+})
+
+test('an anchorTo end list scrolled up by scrollToIndex is not pinned back by a resize before the read-back', () => {
+  // At the end, scrollToIndex(20) writes 1000. The last row grows before the
+  // read-back; against the stale offset the list still looks pinned, and the
+  // end-anchoring compensation used to write 4850 over the index scroll.
+  const { v, el, scroll, settle } = setupHandDrivenScroll({ anchorTo: 'end' })
+  el.scrollTop = 4800
+  scroll(4800, false)
+
+  v.scrollToIndex(20, { align: 'start' })
+  v.resizeItem(99, 100)
+  settle()
+
+  expect(el.scrollTop).toBe(1000)
+  expect(v.scrollState).toBeNull()
+})
+
+test('scrollBy past the end does not leave the next scrollBy building on an unreachable offset', () => {
+  // A write the browser clamps to where the element already is fires no
+  // scroll event, so nothing reads it back.
+  const { v, el, scroll, frames } = setupHandDrivenScroll()
+  el.scrollTop = 4800
+  scroll(4800, false)
+
+  v.scrollBy(100)
+  frames()
+  expect(el.scrollTop).toBe(4800)
+  v.scrollBy(-100)
+
+  expect(el.scrollTop).toBe(4700)
+})
+
+test('scrollBy past the start does not leave the next scrollBy building on an unreachable offset', () => {
+  const { v, el, frames } = setupHandDrivenScroll()
+
+  v.scrollBy(-100)
+  frames()
+  expect(el.scrollTop).toBe(0)
+  v.scrollBy(100)
+
+  expect(el.scrollTop).toBe(100)
+})
+
+test('scrollBy after the safety valve dropped an ignored write builds on the real position', () => {
+  const { v, el, scroll, frames, browser } = setupHandDrivenScroll()
+  el.scrollTop = 2000
+  scroll(2000, false)
+  let now = 0
+  v['now'] = () => now
+
+  browser.ignoreWrites = true
+  v.scrollToIndex(20, { align: 'start' })
+  now = 6000
+  frames()
+  expect(v.scrollState).toBeNull()
+  browser.ignoreWrites = false
+  v.scrollBy(100)
+
+  expect(el.scrollTop).toBe(2100)
+})
+
+test('scrollBy after cancelScroll dropped an ignored write builds on the real position', () => {
+  const { v, el, scroll, browser } = setupHandDrivenScroll()
+  el.scrollTop = 2000
+  scroll(2000, false)
+
+  browser.ignoreWrites = true
+  v.scrollToIndex(20, { align: 'start' })
+  v.cancelScroll()
+  browser.ignoreWrites = false
+  v.scrollBy(100)
+
+  expect(el.scrollTop).toBe(2100)
+})