fix: remove auto-select on focusin from GridView#519
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the auto-select-on-focus behavior from GridView._onFocusIn, aligning GridView with the earlier TreeView fix from #518 so external consumers can fully control selection state without focus-driven side effects.
Changes:
- Simplifies
GridView._onFocusInto only update the roving-tabindex active item (_setActiveItem), removing selection mutation on focus-in. - Updates the GridView unit test to assert that focusing an item does not auto-select it.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/GridView/index.ts | Removes focus-in auto-selection logic; focus-in now only updates the active (tabIndex=0) item. |
| test/components/gridview.mjs | Updates the test expectation for focus-in behavior to ensure focus does not modify selection state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| strictEqual(item1.selected, false); | ||
|
|
||
| // Simulate keyboard Tab-in by focusing and dispatching focusin. | ||
| // jsdom's :focus-visible heuristic is unreliable across tests, | ||
| // so we stub matches() to return true for the duration. | ||
| const origMatches = item1.dom.matches.bind(item1.dom); | ||
| item1.dom.matches = (sel) => sel === ':focus-visible' ? true : origMatches(sel); | ||
| item1.focus(); | ||
| item1.dom.matches = origMatches; | ||
|
|
||
| strictEqual(item1.selected, true); | ||
| strictEqual(item1.selected, false); |
There was a problem hiding this comment.
This updated test no longer forces :focus-visible (or a focusin-from-outside scenario), so it will likely pass even if the old auto-select-on-focus logic were still present (jsdom often reports matches(':focus-visible') === false). To make this regression-proof, keep stubbing matches(':focus-visible') to true (or explicitly dispatch a focusin with an external relatedTarget) and still assert that selected remains false.
Summary
Follow-up to #518 — applies the same
_onFocusInfix toGridViewthat was applied toTreeView.GridView._onFocusInto only update the roving tabindex active item (_setActiveItem), removing the auto-selection logic that checked:focus-visibleand modified selection state on focus-inMotivation
The
GridView._onFocusInhandler contained the same auto-selection logic that caused the race condition fixed inTreeViewby #518. External consumers (like the PlayCanvas Editor) manage selection state themselves, so havingfocusinsilently select items creates conflicts and visual desync between the component's selection state and the application's selection state.Test Plan
npm test)