Repository navigation
fix(Virtual): place the row correctly for an initial or reactive selected - #83
Merged
Merged
Conversation
…cted 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three bugs that show up when
VirtualRow/VirtualColumngets aselectedvalue (initially or reactively) or whenscrollToIndexis called. All three are present in 1.7.1, and the new contract tests reproduce each one.The first two match what a downstream app worked around with a
pnpm patchon 1.6.6 (initialselectedat the last item of anautocarousel, then Back). This PR fixes them here, so that patch can be removed after a release.Bugs and fixes
1.
scroll="auto": an initialselectednear the end puts the focused item off-screen.The initial-setup branch started the window at
total - displaySize - bufferSize. With 12 items,displaySize={4}andselected={11}, the focused item landed at screen x 970, the 5th slot. The window now starts attotal - displaySize, the same window navigation reaches at the end (8,9,10,11, focused item in slot 3).2. Row ends up one slot off after
selected/scrollToIndex.updateSelectedsettargetPositionbefore shifting the row. The next key press shifts fromtargetPosition, so the first press after any initialselectedmoved the row one slot too far. The B14 contract test already called this "a bug from before 1.7 … (out of scope here)".updateSelectedalso shifted from the row's current position, so a reactiveselectedchange on a row that was already scrolled moved it a second slot (x -410 instead of -180).updateSelectednow places the row atoriginalPosition, minus one slot when the layout shifts. It setstargetPositionto where the row actually rests and stops a shift animation that is still running. The non-animated shift path now also updatestargetPosition, so it never goes stale once animations are off.Fixing only the first part would have made the reactive case worse. The old first-press jump happened to return the row to its base position, which hid the second-slot drift.
3.
scroll="edge": an initialselectedfocuses the next item.The initial window started at
c - startScrolling + 1, one item too far (selected={7}focusedv8). It now starts atc - selected.Behaviour changes for reviewers
selectedvariant, the focused item's screen x is now[50, 50, 50, 50, 50, 280, …]instead of[50, 280, …, 510, …].selected/ reactive test, x stays at -180, andsetSel(9)gives the end-of-list window8,9,10,11.selectedchange now also moves the row back tooriginalPositionwhen the new layout needs no shift. Before, the row stayed where it was, soselected={0}on a scrolled row could leave item 0 off-screen.displaySizeitems still take the non-windowed path (total <= displaySize). The downstream patch changed that to<, which makesautorows scroll the first item off-screen andalwaysrows stall before the last item. It's left as is here.Tests
New contract tests in
tests/contract-virtual.test.tsx:autowith an initialselectedof 11, 10, 9 and 5, at x=50 and x=0: the focused item is visible and keeps its screen x over Left×3 and Right.auto/edge/always: a reactiveselectedon a scrolled row places the row exactly as the same initialselecteddoes.edgewith an initialselectedof 1, 3, 7 and 11 focuses that item.Without the source changes, 12 tests fail. With them, the full suite passes: 755 passed, 13 skipped. The branch is rebased on #82.
Local
npm run tscstill reports 4 errors indomRenderer.ts(atlasGroup,rtlonIRendererNodeProps). They come from the locally installed renderer version, appear on main as well, and are unrelated to this change.🤖 Generated with Claude Code