fix(spatialHandleNavigation): don't reset selection at grid edges - #54
Merged
Conversation
When a cross-axis move (up/down in a row-wrapped container, left/right in a column-wrapped one) found no child in an adjacent row/column, the handler still called selectChild with closestIdx === -1. selectChild sets el.selected = -1 for an invalid index, so the failed move wiped the current selection. On the next key press the guard at the top of the handler saw selected === -1, treated it as "no selection", and fell back to the first focusable child — focus jumped to the top-left instead of staying put. Return false when no adjacent row/column exists so the current selection survives. The in-flex-direction branch already did this. Adds a regression test that drives a 3x2 wrapped grid through the focus manager with real key events. Co-Authored-By: Claude Opus 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.
What
spatialHandleNavigationreset the container'sselectedto-1whenever a cross-axis move failed at an edge, which made the next key press jump focus back to the first child.Why
In the cross-axis branch (up/down in a row-wrapped container, left/right in a column-wrapped one), the handler called
selectChild(el, closestIdx)unconditionally — including when the scan found nothing and leftclosestIdx === -1.selectChildtreats an invalid index as "clear the selection":So a no-op edge move silently wiped
selected. On the next press, the guard at the top of the handler sawselected === -1, treated it as "nothing selected", and fell back tofindFirstFocusableChildIdx→ the first child.Reported symptoms, both reproduced: holding Down on the bottom row bounces focus to the first item, and holding Up from a first-row item does the same.
The in-flex-direction branch already returned
falsewithout touchingselected, which is why only cross-axis moves were affected.The change
Return
falsewhen no adjacent row/column exists, beforeselectChildcan clobber the selection:Returning
falsealso lets the key bubble to a parent container, which is the desired behavior at a boundary.Notes for reviewers
I deliberately kept the guard local to
spatialHandleNavigationrather than removing theel.selected = -1reset insideselectChild. That reset is still correct fornavigableForwardFocusandspatialForwardFocus, which call it for the genuinely-no-focusable-children case.Tests
New
tests/spatialNavigation.test.tsxbuilds a 3×2 wrapped flex grid (0 1 2 / 3 4) and drives it through the real focus manager with dispatchedkeydownevents — it asserts the flex layout first, then that repeated Down on the bottom row and repeated Up on the top row hold their selection and still navigate correctly afterward.main(expected -1 to equal 3,expected -1 to equal 1) and pass with the fix.npm run tscclean;npm run lint0 errors (pre-existing warnings unchanged).One fixture note if these tests get extended:
spatialForwardFocusselects the child closest to the previously active element, so rendering a fresh grid per test inherits the prior grid's position. The suite renders once inbeforeAlland sets focus explicitly per case.🤖 Generated with Claude Code