feat(focus): add createFocusGroup - #1004
Conversation
🦋 Changeset detectedLatest commit: 3238a1a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds ChangesFocus group primitive
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Consumer
participant createFocusGroup
participant RootElement
participant getFocusableTreeWalker
participant FocusTarget
Consumer->>createFocusGroup: create group with root and options
createFocusGroup->>RootElement: attach keydown listener
RootElement->>createFocusGroup: receive navigation key
createFocusGroup->>getFocusableTreeWalker: resolve focus candidates
getFocusableTreeWalker-->>createFocusGroup: return filtered candidates
createFocusGroup->>FocusTarget: focus selected element
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
packages/focus/src/focusGroup.ts (4)
108-112: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
fromis never passed to the walker, so the subtree-reject branch is dead for these callers.
getFocusableTreeWalkercontains a filter branch that rejects nodes insideopts.from(lines 320-323), and it also setswalker.currentNode = opts.from(lines 358-360).focusNextandfocusPreviousomitfromfrom the options object and assigncurrentNodethemselves. As a result, iffromis a container that itself holds focusable descendants,focusNextmoves focus into that subtree instead of past it. The upstream Kobalte implementation passesfromto the walker to avoid this.Either pass
fromthrough, or drop the reject branch and thecurrentNodeassignment insidegetFocusableTreeWalkerso the helper has one clear contract.♻️ Option: pass `from` through
- const walker = getFocusableTreeWalker(root, { tabbable, accept }); - - if (from && root.contains(from)) { - walker.currentNode = from; - } + const walker = getFocusableTreeWalker( + root, + from && root.contains(from) ? { tabbable, accept, from } : { tabbable, accept }, + );Also applies to: 142-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/src/focusGroup.ts` around lines 108 - 112, Update the focusNext and focusPrevious walker construction to pass the from value in the options supplied to getFocusableTreeWalker, and remove their manual walker.currentNode assignments so the helper applies its existing from-based positioning and subtree rejection consistently.
312-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument or remove the undocumented
scopeparameter.
getFocusableTreeWalkeris exported frompackages/focus/src/index.ts, soscopeis public API. No caller in this PR passes it, no test covers it, and the JSDoc does not mention it. Add a@param scopedescription, or drop the parameter andisElementInScopeuntil a consumer needs them.Also applies to: 348-348
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/src/focusGroup.ts` around lines 312 - 316, Update the exported getFocusableTreeWalker API by either documenting the scope parameter in its JSDoc with its intended behavior, or removing scope and the associated isElementInScope logic until it has a consumer. Keep the implementation and public signature consistent with the chosen option.
242-253: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider skipping arrow handling for text-entry targets.
The listener sits on the group root and
keydownbubbles. So an<input type="text">or<textarea>inside the group loses caret movement: in horizontal orientationArrowLeftandArrowRightmove focus instead of the caret, andHomeandEndmove focus instead of jumping within the value. Menus and toolbars often contain a filter input.A target check keeps the group usable with text fields.
const isTextEntry = (el: Element | undefined): boolean => el instanceof HTMLTextAreaElement || (el instanceof HTMLInputElement && !/^(button|checkbox|radio|submit|reset)$/.test(el.type)) || (el instanceof HTMLElement && el.isContentEditable);This is a behavior choice, so treat it as optional. An alternative is to document the limitation in the README.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/src/focusGroup.ts` around lines 242 - 253, Optionally update the keydown handling around focusNext, focusPrevious, focusFirst, and focusLast to detect text-entry targets such as text inputs, textareas, and content-editable elements, then bypass arrow, Home, and End focus handling for those targets while preserving existing behavior elsewhere.
38-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit per-call options from group-level options.
FocusGroupOptionscarries both per-call keys (from,tabbable,wrap,accept) and group-level reactive keys (orientation,textDirection,handleTab,keyboardNavigation). TheFocusGroupmethods accept the same type, so a caller can passkeyboardNavigationororientationtofocusNext, where the implementation ignores them. Two types make the contract explicit and prevent silent no-ops.Also, the
acceptdoc states the callback "determines whether the given element is focused". The callback determines whether an element is a focus candidate.♻️ Proposed type split
-export interface FocusGroupOptions { +export interface FocusOptions { /** The element to start searching from. The currently focused element by default. */ from?: Element; /** Whether to only include tabbable elements, or all focusable elements. */ tabbable?: boolean; /** Whether focus should wrap around when it reaches the end of the scope. */ wrap?: boolean; - /** A callback that determines whether the given element is focused. */ + /** A callback that determines whether the given element is a focus candidate. */ accept?: (node: Element) => boolean; +} +export interface FocusGroupOptions extends FocusOptions { /** The orientation of the focus group. `@default` "vertical" */ orientation?: MaybeAccessor<Orientation>; @@ keyboardNavigation?: MaybeAccessor<boolean>; }Then type the
FocusGroupmethods withFocusOptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/src/focusGroup.ts` around lines 38 - 65, Split FocusGroupOptions into group-level configuration and a new FocusOptions type containing from, tabbable, wrap, and accept; update FocusGroup methods to accept FocusOptions so orientation, textDirection, handleTab, and keyboardNavigation cannot be passed per call. Correct the accept documentation to describe whether an element is a focus candidate.packages/focus/test/index.test.tsx (3)
741-746: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pressnever uses itscontainerparameter.The body calls only
flush()andtarget.dispatchEvent(event). Every call site passes a container that the helper discards. Remove the parameter so the helper signature matches its behavior.♻️ Proposed fix
/** Flush pending effects so `createFocusGroup` has attached its keydown listener, then dispatch `event` on `target`. */ - const press = (container: HTMLElement, target: Element, event: KeyboardEvent) => { + const press = (target: Element, event: KeyboardEvent) => { flush(); target.dispatchEvent(event); return event; };Then update the call sites to
press(buttons[0]!, key("ArrowDown")).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/test/index.test.tsx` around lines 741 - 746, Remove the unused container parameter from the press helper and update every call site to pass only the target element and keyboard event, preserving the existing flush and dispatch behavior.
633-636: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap these
createFocusGroupcalls increateRoot.
createFocusGroupcallscreateEffectinternally. Every test in this describe block calls it outside a reactive root. Solid logs a warning for computations created outsidecreateRoot, and the effect is never disposed, so each test leaks akeydownlistener onto its container. ThecreateFocusGroup keyboard navigationblock at line 737 already usescreateRootcorrectly.A shared helper keeps the change small.
♻️ Proposed helper
const withRoot = <T,>(fn: () => T): T => { let result!: T; createRoot(dispose => { result = fn(); onTestFinished(dispose); }); return result; };Then call
withRoot(() => createFocusGroup(() => container))in each test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/test/index.test.tsx` around lines 633 - 636, Wrap every createFocusGroup invocation in the createFocusGroup describe block with a shared withRoot helper that creates and disposes a Solid root via onTestFinished. Update the affected tests to call withRoot(() => createFocusGroup(() => container)), preserving the existing createFocusGroup behavior and the already-correct keyboard-navigation test.
703-734: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for radio-group filtering and visibility filtering.
The tests cover
tabbableandaccept, but two filter paths ingetFocusableTreeWalkerhave no coverage:
- The radio branch at
packages/focus/src/focusGroup.tslines 325-343, together withgetRadiosInGroupandisTabbableRadioat lines 365-395. This is the most intricate logic in the PR. It has form and non-form paths, aRadioNodeListbranch, a single-element branch, and a checked/unchecked rule.- The
isElementVisiblefilter. No test asserts that ahiddenelement or adisplay: noneelement is skipped.The
handleTab: falseoption and thefocusPreviousbranch for afromoutside the root (lines 146-152) are also untested.Do you want me to generate these test cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/test/index.test.tsx` around lines 703 - 734, Expand the focus-group tests to cover radio filtering in getFocusableTreeWalker, including form and non-form groups, RadioNodeList and single-element paths, and checked/unchecked behavior via getRadiosInGroup and isTabbableRadio. Add visibility cases proving hidden and display:none elements are skipped through isElementVisible, and cover handleTab:false plus focusPrevious when from is outside the root.packages/focus/src/tabbable.ts (2)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a comment explaining the
joinsuffix pattern.
Array.prototype.joininserts the separator between entries only. So the suffix:not([hidden])is not applied to the last entry offocusableElements, and the suffix:not([hidden]):not([tabindex="-1"])is not applied to the last entry oftabbableElements. Both selectors are correct today because the trailing literal inFOCUSABLE_ELEMENT_SELECTORand the last entry oftabbableElementscarry their own guards. A future entry appended to either array would silently lose its guards.A short comment above each constant records the constraint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/src/tabbable.ts` around lines 35 - 41, Add concise comments above FOCUSABLE_ELEMENT_SELECTOR and TABBABLE_ELEMENT_SELECTOR documenting that join separators apply guards only between entries, so the final array entry must include its own hidden/tabindex exclusions and future entries must preserve those guards.
49-56: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
isElementVisiblewalks to the document root for every candidate.The function recurses through every ancestor and calls
getComputedStyleat each level.getFocusableTreeWalkercalls it once per candidate node. For a group with many items inside a deeply nested tree, this producescandidates × depthforced style resolutions per navigation keypress.This matches the upstream react-spectrum implementation, so it is acceptable for the initial release. If a consumer reports slow arrow navigation in large lists, an ancestor-result cache scoped to one traversal removes the repeated work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/src/tabbable.ts` around lines 49 - 56, Keep isElementVisible unchanged for this initial release; the comment documents a potential performance optimization rather than a required fix. If optimization becomes necessary, add an ancestor-result cache scoped to a single getFocusableTreeWalker traversal and reuse it across candidate visibility checks.packages/focus/stories/focus.stories.tsx (1)
336-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the listbox ARIA pattern in the story.
The container declares
role="listbox"and the children declarerole="option". The pattern is incomplete:
- An element with
role="option"requiresaria-selected. Screen readers announce the state.role="option"on a<button>overrides the native button role, so the element no longer exposes button semantics.- The standard listbox pattern makes the container the single tab stop and manages the active option with
aria-activedescendantor rovingtabindex. Here every option is separately tabbable.Stories serve as reference usage, so an incomplete pattern gets copied. Either complete the pattern, or drop the
listboxandoptionroles and present the example as a generic focus group.Also applies to: 362-372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/focus/stories/focus.stories.tsx` around lines 336 - 337, Update the focus story’s listbox example around the container and option elements to use a complete accessible listbox pattern: add option selection state, avoid overriding native button semantics, and make the container the single tab stop with active-option management via aria-activedescendant or roving tabindex. Alternatively, remove the listbox/option roles and present the example as a generic focus group.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/public-crabs-joke.md:
- Around line 1-5: Update the changeset for `@solid-primitives/focus` from patch
to minor because the exported createFocusGroup primitive adds a new public API;
leave the existing summary unchanged.
In `@packages/focus/README.md`:
- Around line 244-259: Update the README options documentation to distinguish
per-call traversal options (from, tabbable, wrap, and accept) from group-level
keyboard options (orientation, textDirection, handleTab, and
keyboardNavigation). Revise the introductory text and table
headings/descriptions so they no longer claim every option can be passed to
individual methods; do not change implementation behavior.
In `@packages/focus/src/focusGroup.ts`:
- Line 178: Update the TreeWalker result handling in focusFirst so nextNode()
converts null to undefined, matching its HTMLElement | undefined return contract
and the existing focusNext behavior. Preserve the HTMLElement typing while
ensuring empty focus groups return undefined.
- Around line 238-261: Update handleKeyDown so the next, previous, Home, and End
navigation branches only run when no ctrlKey, metaKey, or altKey modifier is
pressed. Preserve the existing Tab handling, including its shiftKey behavior,
and continue preventing default only for unmodified navigation keys.
- Around line 365-387: Update getRadiosInGroup to immediately return [element]
when element.name is empty, before the form and document query branches.
Preserve the existing grouped-radio behavior for non-empty names.
- Around line 325-343: Update the radio check in the walker filtering logic to
use the HTMLInputElement.type property consistently instead of
getAttribute("type"). Preserve the existing isTabbableRadio and same-name
radio-group filtering behavior, including support for case-insensitive or
normalized radio types.
In `@packages/focus/src/tabbable.ts`:
- Around line 84-91: Add an inert-attribute check to isAttributeVisible so
elements with inert are rejected alongside hidden elements. Keep the existing
DETAILS/SUMMARY visibility logic unchanged; ancestor recursion in
isElementVisible will cover descendants of inert containers.
In `@packages/focus/stories/focus.stories.tsx`:
- Line 352: Replace the BoolRow usage for the focused item in the focus story
with StatRow, passing focusedLabel() as its string value so the row uses the
correct type and presentation.
---
Nitpick comments:
In `@packages/focus/src/focusGroup.ts`:
- Around line 108-112: Update the focusNext and focusPrevious walker
construction to pass the from value in the options supplied to
getFocusableTreeWalker, and remove their manual walker.currentNode assignments
so the helper applies its existing from-based positioning and subtree rejection
consistently.
- Around line 312-316: Update the exported getFocusableTreeWalker API by either
documenting the scope parameter in its JSDoc with its intended behavior, or
removing scope and the associated isElementInScope logic until it has a
consumer. Keep the implementation and public signature consistent with the
chosen option.
- Around line 242-253: Optionally update the keydown handling around focusNext,
focusPrevious, focusFirst, and focusLast to detect text-entry targets such as
text inputs, textareas, and content-editable elements, then bypass arrow, Home,
and End focus handling for those targets while preserving existing behavior
elsewhere.
- Around line 38-65: Split FocusGroupOptions into group-level configuration and
a new FocusOptions type containing from, tabbable, wrap, and accept; update
FocusGroup methods to accept FocusOptions so orientation, textDirection,
handleTab, and keyboardNavigation cannot be passed per call. Correct the accept
documentation to describe whether an element is a focus candidate.
In `@packages/focus/src/tabbable.ts`:
- Around line 35-41: Add concise comments above FOCUSABLE_ELEMENT_SELECTOR and
TABBABLE_ELEMENT_SELECTOR documenting that join separators apply guards only
between entries, so the final array entry must include its own hidden/tabindex
exclusions and future entries must preserve those guards.
- Around line 49-56: Keep isElementVisible unchanged for this initial release;
the comment documents a potential performance optimization rather than a
required fix. If optimization becomes necessary, add an ancestor-result cache
scoped to a single getFocusableTreeWalker traversal and reuse it across
candidate visibility checks.
In `@packages/focus/stories/focus.stories.tsx`:
- Around line 336-337: Update the focus story’s listbox example around the
container and option elements to use a complete accessible listbox pattern: add
option selection state, avoid overriding native button semantics, and make the
container the single tab stop with active-option management via
aria-activedescendant or roving tabindex. Alternatively, remove the
listbox/option roles and present the example as a generic focus group.
In `@packages/focus/test/index.test.tsx`:
- Around line 741-746: Remove the unused container parameter from the press
helper and update every call site to pass only the target element and keyboard
event, preserving the existing flush and dispatch behavior.
- Around line 633-636: Wrap every createFocusGroup invocation in the
createFocusGroup describe block with a shared withRoot helper that creates and
disposes a Solid root via onTestFinished. Update the affected tests to call
withRoot(() => createFocusGroup(() => container)), preserving the existing
createFocusGroup behavior and the already-correct keyboard-navigation test.
- Around line 703-734: Expand the focus-group tests to cover radio filtering in
getFocusableTreeWalker, including form and non-form groups, RadioNodeList and
single-element paths, and checked/unchecked behavior via getRadiosInGroup and
isTabbableRadio. Add visibility cases proving hidden and display:none elements
are skipped through isElementVisible, and cover handleTab:false plus
focusPrevious when from is outside the root.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e45826b-d886-4ac5-8994-c4a198175ac9
📒 Files selected for processing (9)
.changeset/public-crabs-joke.mdpackages/focus/README.mdpackages/focus/package.jsonpackages/focus/src/focusGroup.tspackages/focus/src/index.tspackages/focus/src/tabbable.tspackages/focus/stories/focus.stories.tsxpackages/focus/test/index.test.tsxpackages/focus/test/server.test.ts
2 participants
Ports FocusManager from Kobalte to focus.
Also handles keyboard navigation within the focus group.
Summary by CodeRabbit