From aba5ab98b5a0f827387f173826abff01cb824840 Mon Sep 17 00:00:00 2001 From: Chris Lorenzo Date: Sun, 8 Nov 2026 12:46:42 -0500 Subject: [PATCH] fix(Virtual): place the row correctly for an initial or reactive selected Three bugs when VirtualRow/VirtualColumn is given `selected` (or scrollToIndex is called), each reproduced by the new contract tests: - scroll="auto": near the end of the list the window started at total - displaySize - bufferSize, so the focused item landed past the last visible slot (selected=11 of 12, displaySize 4: screen x 970). It now starts at total - displaySize, the window navigation reaches. - updateSelected saved targetPosition before shifting the row, so the first press afterwards moved the row one slot too far. It also shifted from the current position, so a reactive selected on a scrolled row moved it a second slot. The row now goes to originalPosition (minus one slot when the layout shifts) and targetPosition is set to where it rests. The non-animated shift path also updates targetPosition. - scroll="edge": the initial window started one item late, focusing item c + 1 (selected=7 focused v8). Two contract tests pinned the old first-press jump; their expectations are updated (one already called it a bug from before 1.7). Co-Authored-By: Claude Opus 5.5 --- src/primitives/Virtual.tsx | 29 ++++-- tests/contract-virtual.test.tsx | 166 +++++++++++++++++++++++++++++--- 2 files changed, 171 insertions(+), 24 deletions(-) diff --git a/src/primitives/Virtual.tsx b/src/primitives/Virtual.tsx index d81bb0e..1a2ed32 100644 --- a/src/primitives/Virtual.tsx +++ b/src/primitives/Virtual.tsx @@ -254,9 +254,12 @@ function createVirtual( } else { // Initial setup if (c > 0) { + // Near the end, the window navigation reaches there: it starts + // at total - displaySize. Subtracting bufferSize as well put + // the focused item past the last visible slot. start = Math.min( c - (scrollIndex() || 1), - total - props.displaySize - bufferSize(), + total - props.displaySize, ); selected = Math.max(scrollIndex() || 1, c - start); shiftBy = total - c < 3 ? c - total : -1; @@ -313,7 +316,8 @@ function createVirtual( if (delta === 0 && c > 0) { //initial setup selected = c > startScrolling ? startScrolling : c; - start = Math.max(0, c - startScrolling + 1); + // The window slot `selected` holds item c (start was one more). + start = c - selected; shiftBy = c > startScrolling ? -1 : 0; atStart = c < startScrolling; } else if (delta > 0) { @@ -492,6 +496,7 @@ function createVirtual( } else { view.lng[axis] = shiftPrevChildPos - active[axis] + childSize * slice.shiftBy; + targetPosition = view.lng[axis]; } } @@ -573,14 +578,20 @@ function createVirtual( viewRef.children[activeIndex]?.setFocus(); } - if (shiftBy === 0) return; - - const childSize = computeSize(slice.selected); // Original Position is offset to support scrollToIndex - originalPosition = originalPosition ?? viewRef.lng[axis]; - targetPosition = targetPosition ?? viewRef.lng[axis]; - - viewRef.lng[axis] = (viewRef.lng[axis] || 0) + childSize * -1; + originalPosition = originalPosition ?? viewRef.lng[axis] ?? 0; + // The window is laid out from scratch (delta 0), so the row goes to + // its original position, one slot further with a shift. Shifting from + // the current position moved an already scrolled row a second slot. + let position: number = originalPosition; + if (shiftBy !== 0) position -= computeSize(slice.selected); + if (cachedAnimationController?.state === 'running') { + cachedAnimationController.stop(); + } + viewRef.lng[axis] = position; + // The next press shifts from targetPosition: left at the position + // before the shift, it moved the row one slot too far. + targetPosition = position; }); }; diff --git a/tests/contract-virtual.test.tsx b/tests/contract-virtual.test.tsx index 46ed1b0..6b55b96 100644 --- a/tests/contract-virtual.test.tsx +++ b/tests/contract-virtual.test.tsx @@ -493,11 +493,11 @@ v.describe('VirtualRow: window and scroll modes', () => { const expected: Record = { plain: [50, 50, 50, 50, 50, 50, 50, 50, 50], wrap: [50, 50, 50, 50, 50, 50, 50, 50, 50], - // Not constant, with or without factorScale. The first press moving - // the focused item one slot right is a bug from before 1.7 with an - // initial `selected` (out of scope here). The fifth press is the - // end of the list: the window stops and the focus moves right. - selected: [50, 280, 280, 280, 280, 510, 510, 510, 510], + // The same with or without factorScale. The fifth press is the end + // of the list: the window stops and the focus moves right. (Before + // 1.7.2 the first press after an initial `selected` also moved the + // focused item one slot right.) + selected: [50, 50, 50, 50, 50, 280, 280, 280, 280], }; for (const factorScale of [true, false]) { for (const variant of ['plain', 'wrap', 'selected']) { @@ -756,25 +756,26 @@ v.describe('VirtualRow: onSelectedChanged and selected', () => { v.expect(rowStep(row)).toEqual(['v5', 1, 5, -180, '4,5,6,7,8,9']); v.expect(calls).toEqual([[1, 'v5', 1]]); - // Current behaviour: the first press after an initial selected puts x - // back to 50 as the window shifts, so the focused item sits one slot - // further right (screen x 280) than when the row starts at 0 (screen x - // 50, see the auto case). The position used is the one saved before the - // initial shift (Virtual.tsx:500-502). + // The presses after an initial selected keep x where it is, as when + // the row starts at 0 (see the auto case). Before 1.7.2 the first press + // put x back to 50, one slot too far. await press('ArrowRight'); - v.expect(rowStep(row)).toEqual(['v6', 1, 6, 50, '5,6,7,8,9,10']); + v.expect(rowStep(row)).toEqual(['v6', 1, 6, -180, '5,6,7,8,9,10']); await press('ArrowLeft', 'ArrowLeft'); - v.expect(rowStep(row)).toEqual(['v4', 1, 4, 50, '3,4,5,6,7,8']); + v.expect(rowStep(row)).toEqual(['v4', 1, 4, -180, '3,4,5,6,7,8']); calls.length = 0; + // A reactive change on a scrolled row places it as the initial value + // does (one slot, not one more on top of the current scroll). Near the + // end, the window is the one navigation reaches there. setSel(9); await flush(); - v.expect(rowStep(row)).toEqual(['v9', 3, 9, -180, '6,7,8,9,10,11']); + v.expect(rowStep(row)).toEqual(['v9', 1, 9, -180, '8,9,10,11']); v.expect(calls).toEqual([]); await press('ArrowRight'); - v.expect(rowStep(row)).toEqual(['v10', 3, 10, 50, '7,8,9,10,11']); - v.expect(calls).toEqual([[4, 'v10', 3]]); + v.expect(rowStep(row)).toEqual(['v10', 2, 10, -180, '8,9,10,11']); + v.expect(calls).toEqual([[2, 'v10', 1]]); }, ); @@ -1064,3 +1065,138 @@ v.describe('VirtualRow: each replaced with new items', () => { } } }); + +v.describe('VirtualRow: initial selected (scroll="auto")', () => { + // 12 items, displaySize 4, 200-wide children + gap 30: the visible slots + // are at screen x = row x + 0, 230, 460, 690. An initial `selected` places + // the window as navigation would, and the presses after it keep the row + // where it is. + const screenX = (row: lng.ElementNode) => row.x + lng.activeElement()!.x; + const cases: [x: number, selected: number, screen: number, window: string][] = + [ + [50, 11, 510, '8,9,10,11'], + [50, 10, 280, '8,9,10,11'], + [50, 9, 50, '8,9,10,11'], + [50, 5, 50, '4,5,6,7,8,9'], + [0, 11, 460, '8,9,10,11'], + [0, 5, 0, '4,5,6,7,8,9'], + ]; + for (const [x, selected, screen, window] of cases) { + v.it( + `x=${x}, selected=${selected}: the focused item is visible and keeps its screen x`, + async () => { + let row!: lng.ElementNode; + dispose = await mount(() => ( + + + {(item) => } + + + )); + await flush(); + v.expect([focusedId(), screenX(row), mounted(row)]).toEqual([ + `v${selected}`, + screen, + window, + ]); + const seen: [string | undefined, number][] = []; + for (const key of [ + 'ArrowLeft', + 'ArrowLeft', + 'ArrowLeft', + 'ArrowRight', + ]) { + await press(key); + seen.push([focusedId(), screenX(row)]); + } + v.expect(seen).toEqual([ + [`v${selected - 1}`, screen], + [`v${selected - 2}`, screen], + [`v${selected - 3}`, screen], + [`v${selected - 2}`, screen], + ]); + }, + ); + } +}); + +v.describe('VirtualRow: selected on a scrolled row', () => { + for (const scroll of ['auto', 'edge', 'always'] as const) { + v.it( + `scroll="${scroll}": a reactive selected places the row as the initial value does`, + async () => { + const place = async (initial: number | undefined, keys: string[]) => { + let row!: lng.ElementNode; + const [sel, setSel] = s.createSignal(initial); + const d = await mount(() => ( + + + {(item) => } + + + )); + await press(...keys); + if (initial === undefined) { + setSel(7); + await flush(); + } + await flush(); + const placed = [focusedId(), row.x + lng.activeElement()!.x]; + await press('ArrowRight'); + const next = [focusedId(), row.x + lng.activeElement()!.x]; + d(); + return [placed, next]; + }; + // selected={7} from the start vs. set to 7 after scrolling right. + const fromStart = await place(7, []); + const afterScroll = await place(undefined, [ + 'ArrowRight', + 'ArrowRight', + 'ArrowRight', + 'ArrowRight', + 'ArrowRight', + ]); + v.expect(fromStart[0]![0]).toBe('v7'); + v.expect(afterScroll).toEqual(fromStart); + }, + ); + } +}); + +v.describe('VirtualRow: initial selected (scroll="edge")', () => { + for (const selected of [1, 3, 7, 11]) { + v.it(`selected=${selected} focuses that item`, async () => { + dispose = await mount(() => ( + + + {(item) => } + + + )); + await flush(); + v.expect(focusedId()).toBe(`v${selected}`); + }); + } +});