fix(virtual-core): let a user scroll end a non-smooth scrollToIndex before it lands

.changeset/index-scroll-user-interrupt.md

@@ -0,0 +1,5 @@
+---
+'@tanstack/virtual-core': patch
+---
+
+fix(virtual-core): let a user scroll end a non-smooth `scrollToIndex` before reconcile has seen it land, so a later measurement no longer pulls the user back to the old target

packages/virtual-core/src/index.ts

@@ -406,6 +406,10 @@ type ScrollState = {
   stableFrames: number
   // Keep arrival across measurement-driven retargets so external scrolls can cancel.
   hasReachedTarget: boolean
+  // The command asked for a smooth scroll. Unlike `behavior`, which reconcile
+  // switches to 'auto' for the final approach, this stays set: the smooth
+  // animation can still deliver scroll events after that switch.
+  startedSmooth: 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
@@ -969,15 +973,29 @@ export class Virtualizer<
             offset = this._intendedScrollOffset
           } else if (
             this.scrollState?.index != null &&
-            this.scrollState.hasReachedTarget &&
+            // A non-smooth write lands at once, so any other scroll event is
+            // the user's, even before reconcile saw the target reached (the
+            // target may keep moving, or iOS ignored the write under a
+            // finger). A write the browser clamped because the sizer hasn't
+            // grown yet reads back at the max, which is also where the fresh
+            // target clamps, so the check below keeps it reconciling (#1294).
+            // Not when rows above the target shrink before the read-back and
+            // pull the target below the new max: the read-back then looks
+            // like the user and the scroll stops at the max. That needs the
+            // sizer re-rendered before the scroll event, which browsers have
+            // not been seen to do (pinned by a `.fails` test).
+            // A smooth scroll's own animation events look like a user's, so
+            // it only yields once it has reached the target.
+            (this.scrollState.hasReachedTarget ||
+              !this.scrollState.startedSmooth) &&
             !approxEqual(offset, this.scrollState.lastTargetOffset) &&
             !approxEqual(offset, this.getScrollStateTarget(this.scrollState)) &&
             (this._clampedAdjustment === null ||
               Math.abs(offset - this._clampedAdjustment.maxAtWrite) >= 1.5)
           ) {
-            // The index scroll reached its target, but an external scroll moved
-            // away while we waited for measurements. Our own measurement updates
-            // (including clamped writes) still need to reconcile.
+            // An external scroll moved away from the index scroll's target.
+            // Our own measurement updates (including clamped writes) still
+            // need to reconcile.
             this.scrollState = null
           }
           this._intendedScrollOffset = null
@@ -2063,6 +2081,7 @@ export class Virtualizer<
       lastTargetOffset: offset,
       stableFrames: 0,
       hasReachedTarget: false,
+      startedSmooth: behavior === 'smooth',
     }
 
     this._scrollToOffset(offset, { adjustments: undefined, behavior })
@@ -2098,6 +2117,7 @@ export class Virtualizer<
       lastTargetOffset: offset,
       stableFrames: 0,
       hasReachedTarget: approxEqual(offset, this.getScrollOffset()),
+      startedSmooth: behavior === 'smooth',
     }
 
     this._scrollToOffset(offset, { adjustments: undefined, behavior })
@@ -2126,6 +2146,7 @@ export class Virtualizer<
       lastTargetOffset: offset,
       stableFrames: 0,
       hasReachedTarget: false,
+      startedSmooth: behavior === 'smooth',
     }
 
     this._scrollToOffset(offset, { adjustments: undefined, behavior })
@@ -2150,6 +2171,7 @@ export class Virtualizer<
         lastTargetOffset: offset,
         stableFrames: 0,
         hasReachedTarget: approxEqual(offset, this.getScrollOffset()),
+        startedSmooth: behavior === 'smooth',
       }
 
       this._scrollToOffset(offset, { adjustments: undefined, behavior })

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

@@ -543,6 +543,13 @@ test.each(['auto', 'smooth'] as const)(
     virtualizer._willUpdate()
     virtualizer.getVirtualItems()
     virtualizer.scrollToIndex(99, { align: 'end', behavior })
+    // The scroll max shrinks to 2000 before the read-back, so the browser
+    // reports 2000 instead of the 4400 we wrote. That is where the fresh
+    // target clamps too, so it isn't mistaken for the user scrolling away.
+    Object.defineProperty(mockScrollElement, 'scrollHeight', {
+      value: 2600,
+      configurable: true,
+    })
     onScroll(2000, true)
 
     expect(virtualizer['scrollState']).not.toBeNull()
@@ -2551,46 +2558,40 @@ test('scrollToIndex (#1307): a resize classified against the stale offset does n
   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',
-  () => {
-    // Seen in iOS Safari: a scrollToIndex issued while a finger is dragging is
-    // ignored by the browser, so the offset never gets near the target.
-    const h = makeDeferralHarness()
-    h.run(() => {
-      h.v.scrollToIndex(50, { align: 'start' })
-      h.scroll(true, 20)
-      h.frame()
-      h.scroll(true, 60)
-      h.frame()
-      // A row measures and moves the target; the user is still at 60.
-      h.v.resizeItem(30, 100)
-      h.frame()
-      expect(h.el.scrollTop).toBe(60)
-    })
-  },
-)
-
-test.fails(
-  'scrollToIndex (#1307): a scroll away from a target that moved before landing is not pulled back, non-iOS',
-  () => {
-    // Not specific to iOS ignoring writes under a finger: a target that moves
-    // before reconcile sees the scroll land leaves the state alive too.
-    const h = makeDeferralHarness({ ios: false })
+test('scrollToIndex (#1307): a drag that never reached the target is not pulled back when the target moves', () => {
+  // Seen in iOS Safari: a scrollToIndex issued while a finger is dragging is
+  // ignored by the browser, so the offset never gets near the target.
+  const h = makeDeferralHarness()
+  h.run(() => {
     h.v.scrollToIndex(50, { align: 'start' })
-    h.scroll(true)
-    h.v.resizeItem(10, 100)
+    h.scroll(true, 20)
     h.frame()
-    h.scroll(true)
-    expect(h.el.scrollTop).toBe(2550)
-    // The user scrolls away, then a row above them measures.
-    h.scroll(true, 1000)
+    h.scroll(true, 60)
     h.frame()
-    h.v.resizeItem(5, 100)
+    // A row measures and moves the target; the user is still at 60.
+    h.v.resizeItem(30, 100)
     h.frame()
-    expect(h.el.scrollTop).toBe(1050)
-  },
-)
+    expect(h.el.scrollTop).toBe(60)
+  })
+})
+
+test('scrollToIndex (#1307): a scroll away from a target that moved before landing is not pulled back, non-iOS', () => {
+  // Not specific to iOS ignoring writes under a finger: a target that moves
+  // before reconcile sees the scroll land leaves the state alive too.
+  const h = makeDeferralHarness({ ios: false })
+  h.v.scrollToIndex(50, { align: 'start' })
+  h.scroll(true)
+  h.v.resizeItem(10, 100)
+  h.frame()
+  h.scroll(true)
+  expect(h.el.scrollTop).toBe(2550)
+  // The user scrolls away, then a row above them measures.
+  h.scroll(true, 1000)
+  h.frame()
+  h.v.resizeItem(5, 100)
+  h.frame()
+  expect(h.el.scrollTop).toBe(1050)
+})
 
 test('iOS deferral: a deferred correction follows the viewport through momentum', () => {
   const h = makeDeferralHarness({ start: 1000 })
@@ -5474,9 +5475,10 @@ function setupHandDrivenScroll(
   v['getMeasurements']()
   const scroll = (offset: number, isScrolling: boolean) =>
     scrollCallback!(offset, isScrolling)
+  const frame = () => rafCallbacks.splice(0).forEach((cb) => cb(0))
   const settle = () => {
     for (let i = 0; i < 10 && rafCallbacks.length > 0; i++) {
-      rafCallbacks.splice(0).forEach((cb) => cb(0))
+      frame()
       scroll(el.scrollTop, true)
     }
     scroll(el.scrollTop, false)
@@ -5488,7 +5490,7 @@ function setupHandDrivenScroll(
       rafCallbacks.splice(0).forEach((cb) => cb(0))
     }
   }
-  return { v, el, scroll, settle, frames, browser }
+  return { v, el, scroll, frame, settle, frames, browser }
 }
 
 test('a resize before the read-back of a scrollToIndex write is left to reconcile', () => {
@@ -5649,3 +5651,52 @@ test('scrollBy after cancelScroll dropped an ignored write builds on the real po
 
   expect(el.scrollTop).toBe(2100)
 })
+
+test('a smooth scrollToIndex switched to auto near the end is not cancelled by a late animation event', () => {
+  // Reconcile switches a smooth scroll to 'auto' for the final approach when
+  // the target moves within a viewport. WebKit can still deliver a scroll
+  // event from the smooth animation after that instant write; it is not the
+  // user and must not end the scroll.
+  const { v, el, scroll, frame, settle } = setupHandDrivenScroll()
+
+  v.scrollToIndex(50, { align: 'start', behavior: 'smooth' })
+  el.scrollTop = 2450 // the animation is almost there
+  scroll(2450, true)
+  v.resizeItem(10, 100) // the target moves to 2550, within a viewport
+  frame() // reconcile switches to 'auto' and writes 2550
+  expect(el.scrollTop).toBe(2550)
+
+  el.scrollTop = 2470 // a late event from the smooth animation
+  scroll(2470, true)
+  expect(v.scrollState).not.toBeNull()
+
+  el.scrollTop = 2550
+  scroll(2550, true)
+  settle()
+  expect(el.scrollTop).toBe(2550)
+  expect(v.scrollState).toBeNull()
+})
+
+test.fails(
+  'a scrollToIndex write clamped by a sizer that shrank below it still reaches the fresh target',
+  () => {
+    // Not seen in a real browser: the sizer has to re-render before the
+    // scroll event reports the write. Rows above the target shrink by 600px,
+    // so the max drops from 4800 to 4200 and the target from 4500 to 3900.
+    // The read-back at the new max is neither our write nor the target, so it
+    // reads as a user scroll and the index scroll stops at 4200.
+    const { v, el, scroll, settle } = setupHandDrivenScroll()
+
+    v.scrollToIndex(90, { align: 'start' })
+    expect(el.scrollTop).toBe(4500)
+    for (let i = 0; i < 20; i++) v.resizeItem(i, 20)
+    // The re-render that shrinks the sizer also rebuilds the measurements.
+    v.getVirtualItems()
+    el.scrollHeight = 4400
+    el.scrollTop = 4200
+    scroll(4200, true)
+    settle()
+
+    expect(el.scrollTop).toBe(3900)
+  },
+)