feat(ui): Add SelectableList component#11
Merged
Conversation
SelectableList wraps List.Root to add visual selection indicators (checkboxes for multi-select, radios for single-select) while reusing all existing List selection infrastructure. Includes SelectAll, GroupSelectAll, SelectionSummary compound slots, 13 tests, and 9 stories.
📝 WalkthroughWalkthroughAdds a new SelectableList compound React component (checkbox/radio selection) with TypeScript types, CSS module styling, Storybook stories, and comprehensive tests; re-exports the component from component barrels. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as SelectableList.UI
participant Store as ListStore
participant Item as ItemIndicator
User->>UI: click item / click SelectAll / click GroupSelectAll
UI->>Store: dispatch toggle/selectAll/selectGroup/clearSelection
Store-->>UI: emit snapshot (selectedKeys, enabledKeys)
UI->>Item: read selection state via useSyncExternalStore
Item-->>UI: render indicator (checkbox/radio, checked/indeterminate/disabled)
UI-->>User: updated DOM and SelectionSummary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/selectable-list/SelectableList.module.css`:
- Line 96: The CSS keyword casing is incorrect: replace the background
declaration using the vendor-incorrect value "currentColor" with the lowercase
CSS keyword "currentcolor" in SelectableList.module.css (i.e., change
background: currentColor; to background: currentcolor;), and search for and
normalize any other occurrences of "currentColor" to "currentcolor" to satisfy
stylelint/CSS spec conventions.
In `@packages/base-ui/src/components/selectable-list/SelectableList.stories.tsx`:
- Around line 378-404: The items array in the LargeList story is recreated on
every render; wrap its creation in React.useMemo inside the render function of
the LargeList Story to memoize the 200-item array (reference the
LargeList.render and the items variable) so it only rebuilds when deps change
(e.g., none or specific props), keeping the rest of the JSX (SelectableList,
SelectableList.Viewport, SelectableList.Item) unchanged.
In `@packages/base-ui/src/components/selectable-list/SelectableList.test.tsx`:
- Around line 37-42: The test in SelectableList.test.tsx uses a brittle global
selector '[aria-hidden="true"]'; update the test to target the indicator wrapper
specifically (e.g., query for the CheckboxIndicator class or a data-testid) so
only selection indicators are counted — modify the render/assertion in the
'renders checkbox indicators by default (selectionMode=multiple)' test that
renders <ThreeItemList /> to use document.querySelectorAll('.CheckboxIndicator')
or getAllByTestId('selectable-indicator') (and add the data-testid to the
indicator element in the component if missing) and assert the length is >= 3.
In `@packages/base-ui/src/components/selectable-list/SelectableList.tsx`:
- Line 381: The children wrapper in GroupSelectAll is inconsistent with
SelectAll: SelectAll uses styles.SelectAllLabel but GroupSelectAll renders a
plain <span>, causing visual mismatch; update GroupSelectAll to wrap its
children with the same styles.SelectAllLabel (or extract a shared
component/utility used by both) so both SelectAll and GroupSelectAll use the
identical styled element when rendering {children}, ensuring consistent styling.
- Around line 352-384: GroupSelectAll uses a non-interactive div with
role="presentation" and onClick but lacks keyboard/accessibility support; update
the root element (in the GroupSelectAll component) to be focusable and expose
correct ARIA and keyboard behavior: set tabIndex={0}, change role to "checkbox"
when isCheckbox is true or "radio" otherwise, add aria-checked using
snapshot.allSelected (and use "mixed" when snapshot.someSelected &&
!snapshot.allSelected), and implement an onKeyDown handler that invokes
handleClick when Enter or Space is pressed; keep handleClick for mouse clicks
and ensure these attributes reference snapshot, isCheckbox, and handleClick.
- Around line 199-230: The SelectableList component returns an interactive div
with role="presentation" which breaks keyboard accessibility; change the wrapper
to a semantic focusable control (preferably a <button>) instead of the div,
remove role="presentation", ensure the element forwards ref, preserves className
and {...props}, and uses the existing handleClick as the onClick handler; if you
must keep a non-button element, add tabIndex={0}, handle keyDown for Enter/Space
to call handleClick, and include appropriate ARIA (e.g., aria-pressed or
aria-checked depending on isCheckbox) so screen readers receive the correct
state (use snapshot.allSelected/someSelected to set the ARIA state).
In `@packages/base-ui/src/components/selectable-list/types.ts`:
- Around line 25-26: The empty interface SelectableListItemIndicatorProps
extends HTMLAttributes<HTMLSpanElement> and ESLint flags it as equivalent to its
supertype; replace this interface with a type alias (e.g., type
SelectableListItemIndicatorProps = HTMLAttributes<HTMLSpanElement>) to remove
the redundant declaration while preserving the same exported type used by
consumers such as the SelectableListItemIndicator component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea69ff05-9304-477f-9ccb-ca326d00ccbb
📒 Files selected for processing (7)
packages/base-ui/src/components/index.tspackages/base-ui/src/components/selectable-list/SelectableList.module.csspackages/base-ui/src/components/selectable-list/SelectableList.stories.tsxpackages/base-ui/src/components/selectable-list/SelectableList.test.tsxpackages/base-ui/src/components/selectable-list/SelectableList.tsxpackages/base-ui/src/components/selectable-list/index.tspackages/base-ui/src/components/selectable-list/types.ts
- Normalize currentColor to currentcolor in CSS for spec compliance - Add keyboard/ARIA support to SelectAll and GroupSelectAll (role=checkbox, aria-checked with mixed state, tabIndex, Enter/Space handling) - Use styles.SelectAllLabel in GroupSelectAll for consistent styling - Replace empty interface with type alias for SelectableListItemIndicatorProps - Use specific class selector in checkbox indicator test instead of brittle [aria-hidden] query - Memoize large items array in LargeList story
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/base-ui/src/components/selectable-list/SelectableList.test.tsx (1)
352-369:⚠️ Potential issue | 🟡 MinorAvoid the global
[aria-hidden="true"]query here too.This reintroduces the same brittleness as the earlier indicator test: any decorative hidden element can satisfy the selector and hide a regression. Scope the query to the rendered option or target the indicator class directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/selectable-list/SelectableList.test.tsx` around lines 352 - 369, The test is using a global document.querySelector('[aria-hidden="true"]') which is brittle; scope the indicator lookup to the rendered item instead. Update the test in SelectableList.test.tsx to first locate the specific item (e.g., via getByRole('option', { name: /A/ }) or getByText('A') for SelectableList.Item) and then query within that item for the indicator (either the aria-hidden element or the '.CheckboxIndicator' class), asserting that the indicator inside that specific SelectableList.Item contains 'CheckboxIndicator' rather than using a global document selector.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/selectable-list/SelectableList.test.tsx`:
- Around line 271-293: The test "disabled items show disabled styling and cannot
be toggled" only asserts static attributes; change it to an async test, call
userEvent.setup(), await userEvent.click(itemB) (or userEvent.click on the
option element), and then re-assert that itemB does not become selected (e.g.,
expect(itemB).not.toHaveAttribute('aria-selected', 'true')); keep the existing
checks for aria-disabled and data-ov-disabled and ensure you reference the same
itemB variable and SelectableList rendering so the disabled click path is
actually exercised.
In `@packages/base-ui/src/components/selectable-list/SelectableList.tsx`:
- Around line 147-148: The component SelectableListSelectAll (and the other
affected components in the same file) currently spread consumer props after the
built-in accessibility attributes, allowing callers to override
role/aria/tabIndex/onKeyDown; update the function signature to destructure
onKeyDown (and any other event handlers you need to compose) from props,
merge/compose the consumer onKeyDown with the internal onKeyDown handler, spread
the remaining props before you set the fixed accessibility props (role,
aria-checked, tabIndex, onKeyDown) so the component always enforces its
accessibility contract, and apply the same pattern to the other affected blocks
(around the ranges noted) that use useListStoreContext and set keyboard/ARIA
behavior.
- Around line 186-192: The Select All toggle currently calls
actions.clearSelection() and actions.selectAll() which operate on the entire
list, ignoring the enabled/disabled filtering used to build snapshot; update
handleToggle so it mutates only the enabled-item subset (the same set derived
from registeredKeys minus disabledKeys that snapshot uses). Specifically,
compute the enabledKeys (or reuse snapshot.enabled/available keys) and call
actions.clearSelection(enabledKeys) or actions.selectAll(enabledKeys) (or the
equivalent methods that accept a key subset) instead of the global
actions.clearSelection() / actions.selectAll(); keep the function name
handleToggle and the dependency array ([snapshot.allSelected, actions]) intact.
---
Duplicate comments:
In `@packages/base-ui/src/components/selectable-list/SelectableList.test.tsx`:
- Around line 352-369: The test is using a global
document.querySelector('[aria-hidden="true"]') which is brittle; scope the
indicator lookup to the rendered item instead. Update the test in
SelectableList.test.tsx to first locate the specific item (e.g., via
getByRole('option', { name: /A/ }) or getByText('A') for SelectableList.Item)
and then query within that item for the indicator (either the aria-hidden
element or the '.CheckboxIndicator' class), asserting that the indicator inside
that specific SelectableList.Item contains 'CheckboxIndicator' rather than using
a global document selector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8edfe0e-1145-468c-8243-d79dba24aa36
📒 Files selected for processing (5)
packages/base-ui/src/components/selectable-list/SelectableList.module.csspackages/base-ui/src/components/selectable-list/SelectableList.stories.tsxpackages/base-ui/src/components/selectable-list/SelectableList.test.tsxpackages/base-ui/src/components/selectable-list/SelectableList.tsxpackages/base-ui/src/components/selectable-list/types.ts
Comment on lines
+147
to
+148
| >(function SelectableListSelectAll({ className, children, onClick, ...props }, ref) { | ||
| const store = useListStoreContext(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Don’t let consumer props override the built-in accessibility contract.
Both controls spread {...props} after role, aria-checked, tabIndex, and onKeyDown. That means a consumer can accidentally replace the keyboard behavior or ARIA state just by passing standard HTMLAttributes. Destructure onKeyDown, compose it inside the internal handler, and spread the remaining props before the fixed interaction props so these components keep their accessibility guarantees.
Also applies to: 202-228, 317-318, 377-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/selectable-list/SelectableList.tsx` around
lines 147 - 148, The component SelectableListSelectAll (and the other affected
components in the same file) currently spread consumer props after the built-in
accessibility attributes, allowing callers to override
role/aria/tabIndex/onKeyDown; update the function signature to destructure
onKeyDown (and any other event handlers you need to compose) from props,
merge/compose the consumer onKeyDown with the internal onKeyDown handler, spread
the remaining props before you set the fixed accessibility props (role,
aria-checked, tabIndex, onKeyDown) so the component always enforces its
accessibility contract, and apply the same pattern to the other affected blocks
(around the ranges noted) that use useListStoreContext and set keyboard/ARIA
behavior.
Comment on lines
+186
to
+192
| const handleToggle = useCallback(() => { | ||
| if (snapshot.allSelected) { | ||
| actions.clearSelection(); | ||
| } else { | ||
| actions.selectAll(); | ||
| } | ||
| }, [snapshot.allSelected, actions]); |
There was a problem hiding this comment.
Keep SelectAll mutations scoped to enabled items.
snapshot is derived from registeredKeys minus disabledKeys, but this toggle path switches to clearSelection() / selectAll(), which act on the whole list. If a disabled item is preselected, clicking Select all here will clear it too, and selectAll() can also reselect disabled rows depending on the List action semantics. Mutate the same enabled-key subset you used to compute the snapshot.
Possible fix
const handleToggle = useCallback(() => {
- if (snapshot.allSelected) {
- actions.clearSelection();
- } else {
- actions.selectAll();
- }
- }, [snapshot.allSelected, actions]);
+ const s = store.getSnapshot();
+ const enabled = s.registeredKeys.filter((k) => !s.disabledKeys.has(k));
+
+ if (snapshot.allSelected) {
+ for (const key of enabled) {
+ actions.deselect(key);
+ }
+ } else {
+ for (const key of enabled) {
+ actions.select(key);
+ }
+ }
+ }, [snapshot.allSelected, actions, store]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/selectable-list/SelectableList.tsx` around
lines 186 - 192, The Select All toggle currently calls actions.clearSelection()
and actions.selectAll() which operate on the entire list, ignoring the
enabled/disabled filtering used to build snapshot; update handleToggle so it
mutates only the enabled-item subset (the same set derived from registeredKeys
minus disabledKeys that snapshot uses). Specifically, compute the enabledKeys
(or reuse snapshot.enabled/available keys) and call
actions.clearSelection(enabledKeys) or actions.selectAll(enabledKeys) (or the
equivalent methods that accept a key subset) instead of the global
actions.clearSelection() / actions.selectAll(); keep the function name
handleToggle and the dependency array ([snapshot.allSelected, actions]) intact.
- SelectAll handleToggle now deselects only enabled keys (preserves disabled-but-selected items) instead of calling clearSelection() - Destructure and compose consumer onKeyDown in SelectAll and GroupSelectAll so callers can attach handlers - Spread consumer props before fixed a11y attrs (role, aria-checked, tabIndex, onKeyDown) to prevent accidental overrides - Disabled items test now exercises the click path with pointerEventsCheck disabled to verify the component-level guard - Scope indicator selector in checkBehavior override test to the specific item instead of global document query
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/selectable-list/SelectableList.test.tsx`:
- Around line 49-66: The test in SelectableList.test.tsx only asserts two items
exist but doesn't assert that radio indicators render for
selectionMode="single"; update the test that renders <SelectableList
selectionMode="single"> to also query for the indicator elements (e.g., by class
name "RadioIndicator" or by selecting <SelectableList.ItemIndicator /> nodes)
and assert they are present on each item (similar to the checkbox test).
Specifically, after rendering and getting items via
screen.getAllByRole('option'), add assertions that each item contains an element
with the RadioIndicator class (or use a more robust query matching the
ItemIndicator for single mode) to confirm indicators are rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e901b20f-ad7d-46da-955c-a8cd4ed573de
📒 Files selected for processing (2)
packages/base-ui/src/components/selectable-list/SelectableList.test.tsxpackages/base-ui/src/components/selectable-list/SelectableList.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/selectable-list/SelectableList.test.tsx`:
- Around line 36-378: Add keyboard interaction tests to cover accessibility
behaviors missing from SelectableList tests: (1) "Space toggles" — focus an item
(SelectableList.Item) and simulate Space to toggle aria-selected and indicator
data-checked, (2) "Shift+Arrow range select" — focus an item, hold Shift and
press ArrowDown/ArrowUp to expand selection range across SelectableList.Item
elements and assert aria-selected on the expected range, and (3) "Ctrl+A selects
all" — focus any item or the list, simulate Ctrl+A and assert all
SelectableList.Item elements become aria-selected (and SelectableList.SelectAll
reflects checked/indeterminate state if present); use userEvent.keyboard
(userEvent.setup()) and query items by role 'option' or by text to scope
assertions to the Item indicators (querySelector('[aria-hidden="true"]') for
data-checked/data-indeterminate).
- Around line 110-115: Replace the verbose multi-line comment above the
assertion checking itemA's aria-selected state with a concise one-liner that
summarizes the intent (e.g., "Ensure single selection with toggle behavior still
enforces only one selected item"). Update the comment near the variables itemA,
itemB and the selectionBehavior/selectionMode context in SelectableList.test.tsx
so it stays clear but short; do not change the assertion
expect(itemA).not.toHaveAttribute('aria-selected', 'true').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f532847-e953-4756-bdb6-e4d6cdf23115
📒 Files selected for processing (1)
packages/base-ui/src/components/selectable-list/SelectableList.test.tsx
Comment on lines
+36
to
+378
| describe('SelectableList', () => { | ||
| it('renders checkbox indicators by default (selectionMode=multiple)', () => { | ||
| renderWithTheme(<ThreeItemList />); | ||
| // Each item renders an indicator with the CheckboxIndicator class | ||
| const items = screen.getAllByRole('option'); | ||
| expect(items).toHaveLength(3); | ||
| for (const item of items) { | ||
| const indicator = item.querySelector('[aria-hidden="true"]'); | ||
| expect(indicator).toBeInTheDocument(); | ||
| expect(indicator?.className).toContain('CheckboxIndicator'); | ||
| } | ||
| }); | ||
|
|
||
| it('renders radio indicators for selectionMode=single', () => { | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="single"> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
| const items = screen.getAllByRole('option'); | ||
| expect(items).toHaveLength(2); | ||
| for (const item of items) { | ||
| const indicator = item.querySelector('[aria-hidden="true"]'); | ||
| expect(indicator).toBeInTheDocument(); | ||
| expect(indicator?.className).toContain('RadioIndicator'); | ||
| } | ||
| }); | ||
|
|
||
| it('click on item toggles checkbox — data-checked appears on indicator', async () => { | ||
| const user = userEvent.setup(); | ||
| renderWithTheme(<ThreeItemList />); | ||
|
|
||
| const itemA = screen.getByText('A').closest('[role="option"]')!; | ||
| await user.click(itemA); | ||
|
|
||
| expect(itemA).toHaveAttribute('aria-selected', 'true'); | ||
| const indicator = itemA.querySelector('[aria-hidden="true"]'); | ||
| expect(indicator).toHaveAttribute('data-checked', ''); | ||
| }); | ||
|
|
||
| it('click on item selects radio — previous radio unchecks', async () => { | ||
| const user = userEvent.setup(); | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="single"> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| const itemA = screen.getByText('A').closest('[role="option"]')!; | ||
| const itemB = screen.getByText('B').closest('[role="option"]')!; | ||
|
|
||
| await user.click(itemA); | ||
| expect(itemA).toHaveAttribute('aria-selected', 'true'); | ||
|
|
||
| await user.click(itemB); | ||
| expect(itemB).toHaveAttribute('aria-selected', 'true'); | ||
| // In single+toggle mode, clicking B toggles B on. A is still selected | ||
| // because toggle doesn't deselect others. But with single mode, List | ||
| // only allows one selection at a time. | ||
| // Actually, selectionBehavior='toggle' with selectionMode='single' | ||
| // still only allows one key. Let's verify: | ||
| expect(itemA).not.toHaveAttribute('aria-selected', 'true'); | ||
| }); | ||
|
|
||
| it('SelectAll checks all items', async () => { | ||
| const user = userEvent.setup(); | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="multiple"> | ||
| <SelectableList.SelectAll>Select all</SelectableList.SelectAll> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| await user.click(screen.getByText('Select all')); | ||
|
|
||
| const items = screen.getAllByRole('option'); | ||
| for (const item of items) { | ||
| expect(item).toHaveAttribute('aria-selected', 'true'); | ||
| } | ||
| }); | ||
|
|
||
| it('SelectAll shows data-indeterminate when partial selection', async () => { | ||
| const user = userEvent.setup(); | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="multiple"> | ||
| <SelectableList.SelectAll data-testid="select-all">Select all</SelectableList.SelectAll> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| // Select just one item | ||
| const itemA = screen.getByText('A').closest('[role="option"]')!; | ||
| await user.click(itemA); | ||
|
|
||
| const selectAll = screen.getByTestId('select-all'); | ||
| const indicator = selectAll.querySelector('[aria-hidden="true"]'); | ||
| expect(indicator).toHaveAttribute('data-indeterminate', ''); | ||
| }); | ||
|
|
||
| it('SelectAll clears when all selected', async () => { | ||
| const user = userEvent.setup(); | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="multiple"> | ||
| <SelectableList.SelectAll>Select all</SelectableList.SelectAll> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| const selectAllBtn = screen.getByText('Select all').closest('[role="checkbox"]')!; | ||
|
|
||
| // Select all | ||
| await user.click(selectAllBtn); | ||
| const items = screen.getAllByRole('option'); | ||
| for (const item of items) { | ||
| expect(item).toHaveAttribute('aria-selected', 'true'); | ||
| } | ||
|
|
||
| // Click again to clear | ||
| await user.click(selectAllBtn); | ||
| for (const item of items) { | ||
| expect(item).not.toHaveAttribute('aria-selected', 'true'); | ||
| } | ||
| }); | ||
|
|
||
| it('GroupSelectAll toggles only group items', async () => { | ||
| const user = userEvent.setup(); | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="multiple"> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Group> | ||
| <SelectableList.GroupSelectAll groupKeys={['a', 'b']}> | ||
| Group 1 | ||
| </SelectableList.GroupSelectAll> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Group> | ||
| <SelectableList.Group> | ||
| <SelectableList.GroupHeader>Group 2</SelectableList.GroupHeader> | ||
| <SelectableList.Item itemKey="c" textValue="C"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>C</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Group> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| await user.click(screen.getByText('Group 1')); | ||
|
|
||
| const itemA = screen.getByText('A').closest('[role="option"]')!; | ||
| const itemB = screen.getByText('B').closest('[role="option"]')!; | ||
| const itemC = screen.getByText('C').closest('[role="option"]')!; | ||
|
|
||
| expect(itemA).toHaveAttribute('aria-selected', 'true'); | ||
| expect(itemB).toHaveAttribute('aria-selected', 'true'); | ||
| expect(itemC).not.toHaveAttribute('aria-selected', 'true'); | ||
| }); | ||
|
|
||
| it('GroupSelectAll shows indeterminate state', async () => { | ||
| const user = userEvent.setup(); | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="multiple"> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Group> | ||
| <SelectableList.GroupSelectAll groupKeys={['a', 'b']} data-testid="group-sa"> | ||
| Group 1 | ||
| </SelectableList.GroupSelectAll> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Group> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| // Select just item A | ||
| const itemA = screen.getByText('A').closest('[role="option"]')!; | ||
| await user.click(itemA); | ||
|
|
||
| const groupSA = screen.getByTestId('group-sa'); | ||
| const indicator = groupSA.querySelector('[aria-hidden="true"]'); | ||
| expect(indicator).toHaveAttribute('data-indeterminate', ''); | ||
| }); | ||
|
|
||
| it('disabled items show disabled styling and cannot be toggled', async () => { | ||
| const user = userEvent.setup({ pointerEventsCheck: 0 }); | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="multiple" disabledKeys={['b']}> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| const itemB = screen.getByText('B').closest('[role="option"]')!; | ||
| // Disabled items have aria-disabled and data-ov-disabled | ||
| expect(itemB).toHaveAttribute('aria-disabled', 'true'); | ||
| expect(itemB).toHaveAttribute('data-ov-disabled', 'true'); | ||
|
|
||
| // Attempt to click disabled item — it should remain unselected | ||
| await user.click(itemB); | ||
| expect(itemB).not.toHaveAttribute('aria-selected', 'true'); | ||
| }); | ||
|
|
||
| it('controlled mode: selectedKeys prop drives indicator state', () => { | ||
| const selected = new Set(['a']); | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="multiple" selectedKeys={selected}> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| const itemA = screen.getByText('A').closest('[role="option"]')!; | ||
| const itemB = screen.getByText('B').closest('[role="option"]')!; | ||
|
|
||
| expect(itemA).toHaveAttribute('aria-selected', 'true'); | ||
| expect(itemB).not.toHaveAttribute('aria-selected', 'true'); | ||
|
|
||
| const indicatorA = itemA.querySelector('[aria-hidden="true"]'); | ||
| expect(indicatorA).toHaveAttribute('data-checked', ''); | ||
| }); | ||
|
|
||
| it('SelectionSummary shows correct count', async () => { | ||
| const user = userEvent.setup(); | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="multiple"> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="b" textValue="B"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>B</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| <SelectableList.Item itemKey="c" textValue="C"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>C</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Viewport> | ||
| <SelectableList.SelectionSummary /> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| expect(screen.getByText('0 of 3 selected')).toBeInTheDocument(); | ||
|
|
||
| const itemA = screen.getByText('A').closest('[role="option"]')!; | ||
| await user.click(itemA); | ||
|
|
||
| expect(screen.getByText('1 of 3 selected')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('checkBehavior override: force checkbox even with selectionMode=single', () => { | ||
| renderWithTheme( | ||
| <SelectableList selectionMode="single" checkBehavior="checkbox"> | ||
| <SelectableList.Viewport> | ||
| <SelectableList.Item itemKey="a" textValue="A"> | ||
| <SelectableList.ItemIndicator /> | ||
| <SelectableList.ItemLabel>A</SelectableList.ItemLabel> | ||
| </SelectableList.Item> | ||
| </SelectableList.Viewport> | ||
| </SelectableList>, | ||
| ); | ||
|
|
||
| // Scope indicator lookup to the rendered item | ||
| const item = screen.getByRole('option'); | ||
| const indicator = item.querySelector('[aria-hidden="true"]'); | ||
| expect(indicator).toBeInTheDocument(); | ||
| expect(indicator?.className).toContain('CheckboxIndicator'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for keyboard interactions.
The PR objectives mention keyboard interactions (Space toggles, Shift+Arrow range select, Ctrl+A selects all) as part of the UX requirements, but no keyboard tests are included. Consider adding tests for these accessibility-critical behaviors.
Would you like me to help draft test cases for keyboard interactions, or should I open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/selectable-list/SelectableList.test.tsx`
around lines 36 - 378, Add keyboard interaction tests to cover accessibility
behaviors missing from SelectableList tests: (1) "Space toggles" — focus an item
(SelectableList.Item) and simulate Space to toggle aria-selected and indicator
data-checked, (2) "Shift+Arrow range select" — focus an item, hold Shift and
press ArrowDown/ArrowUp to expand selection range across SelectableList.Item
elements and assert aria-selected on the expected range, and (3) "Ctrl+A selects
all" — focus any item or the list, simulate Ctrl+A and assert all
SelectableList.Item elements become aria-selected (and SelectableList.SelectAll
reflects checked/indeterminate state if present); use userEvent.keyboard
(userEvent.setup()) and query items by role 'option' or by text to scope
assertions to the Item indicators (querySelector('[aria-hidden="true"]') for
data-checked/data-indeterminate).
Comment on lines
+110
to
+115
| // In single+toggle mode, clicking B toggles B on. A is still selected | ||
| // because toggle doesn't deselect others. But with single mode, List | ||
| // only allows one selection at a time. | ||
| // Actually, selectionBehavior='toggle' with selectionMode='single' | ||
| // still only allows one key. Let's verify: | ||
| expect(itemA).not.toHaveAttribute('aria-selected', 'true'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying verbose inline comments.
The multi-line comment block explaining single-selection behavior is verbose. A brief one-liner would suffice.
Suggested simplification
await user.click(itemB);
expect(itemB).toHaveAttribute('aria-selected', 'true');
- // In single+toggle mode, clicking B toggles B on. A is still selected
- // because toggle doesn't deselect others. But with single mode, List
- // only allows one selection at a time.
- // Actually, selectionBehavior='toggle' with selectionMode='single'
- // still only allows one key. Let's verify:
+ // Single selection mode deselects previous item
expect(itemA).not.toHaveAttribute('aria-selected', 'true');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // In single+toggle mode, clicking B toggles B on. A is still selected | |
| // because toggle doesn't deselect others. But with single mode, List | |
| // only allows one selection at a time. | |
| // Actually, selectionBehavior='toggle' with selectionMode='single' | |
| // still only allows one key. Let's verify: | |
| expect(itemA).not.toHaveAttribute('aria-selected', 'true'); | |
| // Single selection mode deselects previous item | |
| expect(itemA).not.toHaveAttribute('aria-selected', 'true'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/selectable-list/SelectableList.test.tsx`
around lines 110 - 115, Replace the verbose multi-line comment above the
assertion checking itemA's aria-selected state with a concise one-liner that
summarizes the intent (e.g., "Ensure single selection with toggle behavior still
enforces only one selected item"). Update the comment near the variables itemA,
itemB and the selectionBehavior/selectionMode context in SelectableList.test.tsx
so it stays clear but short; do not change the assertion
expect(itemA).not.toHaveAttribute('aria-selected', 'true').
- Add Space toggle test (select/deselect via keyboard) - Add Shift+Arrow range selection test - Add Ctrl+A select-all test with SelectAll checkbox state assertion - Replace verbose multi-line comment with concise one-liner
This was referenced Mar 8, 2026
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
1 participant
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
SelectableListcompound component that wrapsList.Rootwith visual checkbox/radio selection indicatorsSelectAll,GroupSelectAll(with indeterminate state), andSelectionSummarycompound slotscheckBehaviorfromselectionMode(checkbox for multiple, radio for single), with manual override supportTest plan
pnpm typecheckpassesLists/SelectableListsection visible with all 9 storiesSummary by CodeRabbit