Repository navigation
docs(spec): MultiSelector records its shipped contract as current - #9133
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This review is on hold for a spec decision. Promoting MultiSelector.spec.md to current adds requirements no current record states yet: FR9's create-row rules from the pending DEC-2, plus FR12–FR14, ORD1 and PR1. An engineering owner records approval by commenting /approve-spec <full head SHA>, and the author may do so as an engineering owner. Three record fixes are needed either way, and a new commit resets approval, so please push them before that comment:
- FR12 says Delete or Backspace on the trigger always clears the value, but
hooks.tsonly does so withhasClearand withouthasSearch. - The record should cite
spec:AST-055rather than keep its own copy of therenderTriggerdecision in DEC-1, because AST-055 names MultiSelector as a record that cites it. - The
option actionrow lost its default, owner and invalid-value cells (nullmeans none; an unnamed control warns) and belongs in Public concepts, withspec:AST-058AR1–AR4 restored in FR10's basis.
[Automated review]
| | option action | `action` absent, present | The option's secondary control as one node; the popup is a grid of rows pairing option and action | | ||
| | FR10 | Once any declared option — at any depth of sections, before any query filters it — carries an `action`, the popup is a `role="grid"` and stays one while mounted; the trigger advertises `aria-haspopup="grid"` (the bottom sheet keeps `dialog`). Each option, the select-all row included, is a `role="row"` carrying the option's id, `aria-selected` and `aria-disabled`, with exactly two `gridcell` children: the option's content, then its action or an empty cell. A section is a `rowgroup` named by its title. Pressing the option cell toggles; pressing the action fires the caller's control and nothing else. Up/Down move rows onto the option cell; the inline-end arrow moves to the action cell and the inline-start arrow back, following visual direction under RTL; Enter on the action cell activates the control. `aria-activedescendant` names the row or the action cell. No control inside the popup is a tab stop. | `spec:AST-058` FR1–FR8 | | ||
| | FR11 | No surface renders native `disabled`. A disabled trigger keeps its tab stop with `aria-disabled`; no press or key opens it or reaches a caller or ancestor handler; it submits no value; a `disabledMessage` Tooltip is linked from the trigger through `aria-describedby`; and only `isDisabled` takes disabled paint. While busy (`isLoading` or a pending `changeAction`) the trigger shows a Spinner and `aria-busy`, keeps its ordinary paint, and announces each busy start once. | `spec:AST-085` FR1–FR6; `spec:AST-086` FR8; `family:input-fields` FR4–FR6 | | ||
| | FR12 | ArrowDown on the focused trigger opens the list; ArrowDown and ArrowUp move the highlight; Home and End jump to the first and last option without search, while with search they move the caret and PageDown and PageUp jump the highlight; Enter toggles the highlighted option except during input-method composition; Escape closes; Tab closes and moves focus on; Delete or Backspace on the focused trigger clears every value when one is selected. | Current source and tests | |
There was a problem hiding this comment.
The Delete/Backspace clause is broader than the shipped behavior. hooks.ts clears only when !hasSearch && !isOpen && onClear != null && hasValue, and MultiSelector.tsx passes onClear only with hasClear. Suggest: "With hasClear and without hasSearch, Delete or Backspace on the focused, closed trigger clears every value when one is selected."
| | FR5 | Pointer popup and Touch sheet are separate surface anatomy rows, but both host the same `panelContent` tree; the Touch sheet additionally renders its Heading. Changing presentation does not create a second search, option, divider, section, or empty-state content model. | Current source and tests | | ||
| | FR6 | Clear glyphs delegate exclusively to the shared `input-clear-icon` target; removed per-component aliases do not claim anatomy. | Current public target metadata | | ||
| | FR7 | While `isReadOnly` is true, selected values remain focusable and form-submittable, while the selection surface, clear action, disclosure indicator, and every value-change path are unavailable. | `spec:AST-011` DEC-1; current docs and tests | | ||
| | FR8 | With `renderTrigger`, the caller-rendered control receives `ref`, `id`, `onClick`, `onKeyDown`, `onFocus`, `aria-haspopup`, `aria-expanded`, `aria-controls`, and `aria-busy`; the panel is anchored to it, focus returns to it on close, the listbox is named by `label`, and the listbox takes focus and owns the keyboard on open. No Field, status, clear action, spinner, or indicator renders. `handleRef` exposes `open`, `close`, `toggle`, `isOpen`; `onOpenChange` fires on every open and close. | DEC-1; current docs and tests | |
There was a problem hiding this comment.
spec:AST-055 (current) owns the caller-rendered trigger and names MultiSelector as a record that cites it instead of keeping a private copy. Please add it to system_specs and to this row's basis, and narrow DEC-1 to the handleRef-over-isOpen choice.
| | FR7 | While `isReadOnly` is true, selected values remain focusable and form-submittable, while the selection surface, clear action, disclosure indicator, and every value-change path are unavailable. | `spec:AST-011` DEC-1; current docs and tests | | ||
| | FR8 | With `renderTrigger`, the caller-rendered control receives `ref`, `id`, `onClick`, `onKeyDown`, `onFocus`, `aria-haspopup`, `aria-expanded`, `aria-controls`, and `aria-busy`; the panel is anchored to it, focus returns to it on close, the listbox is named by `label`, and the listbox takes focus and owns the keyboard on open. No Field, status, clear action, spinner, or indicator renders. `handleRef` exposes `open`, `close`, `toggle`, `isOpen`; `onOpenChange` fires on every open and close. | DEC-1; current docs and tests | | ||
| | FR9 | With `hasSearch` and `hasCreate`, once options have loaded, a `Create "<query>"` row leads the list when the trimmed query equals no option label under the search's own case-insensitive matching. Picking it, or Enter with nothing highlighted, calls `onChange` with the query appended to the value and a `{type: 'create', query}` descriptor, clears the search, and announces the creation; every other change passes no descriptor. The row has no checkbox, is never selected, is excluded from select-all and from `renderOption`, and is announced instead of the empty message when it is the only result. While `isLoading`, no row is offered and Enter commits nothing. `hasCreate` without `hasSearch` warns in development and offers nothing. | Current behavior; `spec:AST-056` FR5 and DEC-3 | | ||
| | option action | `action` absent, present | The option's secondary control as one node; the popup is a grid of rows pairing option and action | |
There was a problem hiding this comment.
This concept row lost its Availability, Default, Owner, Stability and Invalid-value cells (null is none; a control without a name warns in development). Please move it into Public concepts with all eight cells, and restore spec:AST-058 AR1–AR4 in FR10's basis.
…s, and restores the option-action concept
There was a problem hiding this comment.
This review is on hold for a spec decision: promoting MultiSelector.spec.md to current adds requirements no current record states yet (FR9's create-row rules from the removed DEC-2, FR12, FR13's control-box press, FR14's selection announcements, and PR1). An engineering owner records approval by commenting /approve-spec <full head SHA>, and the author may do so as an engineering owner. FR12, the AST-055 citation and the option-action row now read correctly, but two record fixes remain and a new commit resets approval, so please push them before that comment:
- FR13 says a caller
onKeyDownruns after the built-in keyboard handling, but it never runs for a key that handling consumes: the arrows, Enter, Escape, Home/End and PageUp/PageDown. - FR14 ties "All selected" and "Selection cleared" to select-all and clear, but any toggle that selects every option, or leaves none, announces them too.
[Automated review]
| | FR10 | Once any declared option — at any depth of sections, before any query filters it — carries an `action`, the popup is a `role="grid"` and stays one while mounted; the trigger advertises `aria-haspopup="grid"` (the bottom sheet keeps `dialog`). Each option, the select-all row included, is a `role="row"` carrying the option's id, `aria-selected` and `aria-disabled`, with exactly two `gridcell` children: the option's content, then its action or an empty cell. A section is a `rowgroup` named by its title. Pressing the option cell toggles; pressing the action fires the caller's control and nothing else. Up/Down move rows onto the option cell; the inline-end arrow moves to the action cell and the inline-start arrow back, following visual direction under RTL; Enter on the action cell activates the control. `aria-activedescendant` names the row or the action cell. No control inside the popup is a tab stop. | `spec:AST-058` FR1–FR8, AR1–AR4 | | ||
| | FR11 | No surface renders native `disabled`. A disabled trigger keeps its tab stop with `aria-disabled`; no press or key opens it or reaches a caller or ancestor handler; it submits no value; a `disabledMessage` Tooltip is linked from the trigger through `aria-describedby`; and only `isDisabled` takes disabled paint. While busy (`isLoading` or a pending `changeAction`) the trigger shows a Spinner and `aria-busy`, keeps its ordinary paint, and announces each busy start once. | `spec:AST-085` FR1–FR6; `spec:AST-086` FR8; `family:input-fields` FR4–FR6 | | ||
| | FR12 | ArrowDown on the focused trigger opens the list; ArrowDown and ArrowUp move the highlight; Home and End jump to the first and last option without search, while with search they move the caret and PageDown and PageUp jump the highlight; Enter toggles the highlighted option except during input-method composition; Escape closes; Tab closes and moves focus on; with `hasClear` and without `hasSearch`, Delete or Backspace on the focused, closed trigger clears every value when one is selected. | Current source and tests | | ||
| | FR13 | The field has no root styling props: `control*` reaches the bordered control box around the trigger and `container*` the Field box (`family:input-fields` FR10); a press on the control box outside the trigger opens it; a caller `onClick` runs after the built-in toggle and a caller `onKeyDown` after the built-in keyboard model; a caller `aria-describedby` joins the field description and a caller `aria-labelledby` follows the field's own name. With `renderTrigger`, `container*` and `control*` have no effect and warn. The trigger height is the shared element size for its size and MUST grow with a set text size (`spec:AST-102` FR4). Implementation gap: the height is fixed. | `family:input-fields` FR10; `spec:AST-102` FR4; current tests | |
There was a problem hiding this comment.
A caller onKeyDown never runs for a key the built-in model handles. composeEventHandlers stops at the first handler that calls preventDefault (utils/composeEventHandlers.ts:36-41), and hooks.ts prevents default on the arrows, Enter, Home/End, PageUp/PageDown, Escape while open, and the clear keys when they clear. Suggest: "a caller onKeyDown runs after the built-in keyboard model, and only for a key that model leaves unhandled".
| | FR11 | No surface renders native `disabled`. A disabled trigger keeps its tab stop with `aria-disabled`; no press or key opens it or reaches a caller or ancestor handler; it submits no value; a `disabledMessage` Tooltip is linked from the trigger through `aria-describedby`; and only `isDisabled` takes disabled paint. While busy (`isLoading` or a pending `changeAction`) the trigger shows a Spinner and `aria-busy`, keeps its ordinary paint, and announces each busy start once. | `spec:AST-085` FR1–FR6; `spec:AST-086` FR8; `family:input-fields` FR4–FR6 | | ||
| | FR12 | ArrowDown on the focused trigger opens the list; ArrowDown and ArrowUp move the highlight; Home and End jump to the first and last option without search, while with search they move the caret and PageDown and PageUp jump the highlight; Enter toggles the highlighted option except during input-method composition; Escape closes; Tab closes and moves focus on; with `hasClear` and without `hasSearch`, Delete or Backspace on the focused, closed trigger clears every value when one is selected. | Current source and tests | | ||
| | FR13 | The field has no root styling props: `control*` reaches the bordered control box around the trigger and `container*` the Field box (`family:input-fields` FR10); a press on the control box outside the trigger opens it; a caller `onClick` runs after the built-in toggle and a caller `onKeyDown` after the built-in keyboard model; a caller `aria-describedby` joins the field description and a caller `aria-labelledby` follows the field's own name. With `renderTrigger`, `container*` and `control*` have no effect and warn. The trigger height is the shared element size for its size and MUST grow with a set text size (`spec:AST-102` FR4). Implementation gap: the height is fixed. | `family:input-fields` FR10; `spec:AST-102` FR4; current tests | | ||
| | FR14 | Toggling an option announces the selection count politely, select-all that selects everything announces the all-selected message, and clearing announces it; once a person searches, the match count (or the empty-results message, or the create row's label when it is the only result) is announced politely, and nothing is announced while `isLoading`. With `htmlName` the field submits one entry per selected value. | `spec:AST-086`; current tests | |
There was a problem hiding this comment.
announceSelection (MultiSelector.tsx:1290-1309) serves toggle, select-all and clear alike. It says "All selected" whenever every option ends selected, "Selection cleared" whenever none does, and the count otherwise, so toggling the last unselected option says "All selected". Suggest: "Each toggle, select-all or clear announces politely: the all-selected message when every option ends selected, the cleared message when none does, and otherwise the selection count". Also scope "nothing is announced while isLoading" to search results, since FR11 announces the busy start.
There was a problem hiding this comment.
This review is on hold for a spec decision: promoting MultiSelector.spec.md to current adds requirements no current record states yet (FR9's create-row rules from the removed DEC-2, FR12, FR13's control-box press and caller-key rule, FR14's selection announcements, and PR1). An engineering owner records approval by commenting /approve-spec <full head SHA>, and the author may do so as an engineering owner. FR14 now reads as shipped, but five clauses still describe more than the component does, and a new commit resets approval, so please fix them before that comment:
- FR12 says Tab closes the list and FR10 says no control in the popup is a tab stop, but with a typed query forward Tab reaches the search clear button and keeps the list open, as
MultiSelector.test.tsxasserts; FR10 can say "no action control", as AST-058 FR5 does. - FR11 says no key on a disabled trigger reaches an ancestor handler, but a keydown still bubbles to an ancestor; only presses are stopped.
- FR13 should say the caller's
onKeyDownis skipped for keys the built-in model handles and cancels, as the prop's JSDoc does, because it still receives Tab and typeahead letters. - FR8 and the Accessibility contract say the listbox takes focus with a caller trigger, but with
hasSearchthe search input does.
[Automated review]
| | FR5 | Pointer popup and Touch sheet are separate surface anatomy rows, but both host the same `panelContent` tree; the Touch sheet additionally renders its Heading. Changing presentation does not create a second search, option, divider, section, or empty-state content model. | Current source and tests | | ||
| | FR6 | Clear glyphs delegate exclusively to the shared `input-clear-icon` target; removed per-component aliases do not claim anatomy. | Current public target metadata | | ||
| | FR7 | While `isReadOnly` is true, selected values remain focusable and form-submittable, while the selection surface, clear action, disclosure indicator, and every value-change path are unavailable. | `spec:AST-011` DEC-1; current docs and tests | | ||
| | FR8 | With `renderTrigger`, the caller-rendered control receives `ref`, `id`, `onClick`, `onKeyDown`, `onFocus`, `aria-haspopup`, `aria-expanded`, `aria-controls`, and `aria-busy`; the panel is anchored to it, focus returns to it on close, the listbox is named by `label`, and the listbox takes focus and owns the keyboard on open. No Field, status, clear action, spinner, or indicator renders. `handleRef` exposes `open`, `close`, `toggle`, `isOpen`; `onOpenChange` fires on every open and close. | `spec:AST-055` FR1–FR7, AR1–AR4; DEC-1; current docs and tests | |
There was a problem hiding this comment.
With hasSearch, opening from a caller trigger focuses the search input, not the listbox (MultiSelector.tsx:1432-1443; the search branch's listbox has no tabIndex). Suggest: "on open the search input takes focus with hasSearch, and otherwise the listbox, which then owns the keyboard", and the same in the Accessibility contract's caller-trigger paragraph.
| | FR7 | While `isReadOnly` is true, selected values remain focusable and form-submittable, while the selection surface, clear action, disclosure indicator, and every value-change path are unavailable. | `spec:AST-011` DEC-1; current docs and tests | | ||
| | FR8 | With `renderTrigger`, the caller-rendered control receives `ref`, `id`, `onClick`, `onKeyDown`, `onFocus`, `aria-haspopup`, `aria-expanded`, `aria-controls`, and `aria-busy`; the panel is anchored to it, focus returns to it on close, the listbox is named by `label`, and the listbox takes focus and owns the keyboard on open. No Field, status, clear action, spinner, or indicator renders. `handleRef` exposes `open`, `close`, `toggle`, `isOpen`; `onOpenChange` fires on every open and close. | `spec:AST-055` FR1–FR7, AR1–AR4; DEC-1; current docs and tests | | ||
| | FR9 | With `hasSearch` and `hasCreate`, once options have loaded, a `Create "<query>"` row leads the list when the trimmed query equals no option label under the search's own case-insensitive matching. Picking it, or Enter with nothing highlighted, calls `onChange` with the query appended to the value and a `{type: 'create', query}` descriptor, clears the search, and announces the creation; every other change passes no descriptor. The row has no checkbox, is never selected, is excluded from select-all and from `renderOption`, and is announced instead of the empty message when it is the only result. While `isLoading`, no row is offered and Enter commits nothing. `hasCreate` without `hasSearch` warns in development and offers nothing. | Current behavior; `spec:AST-056` FR5 and DEC-3 | | ||
| | FR10 | Once any declared option — at any depth of sections, before any query filters it — carries an `action`, the popup is a `role="grid"` and stays one while mounted; the trigger advertises `aria-haspopup="grid"` (the bottom sheet keeps `dialog`). Each option, the select-all row included, is a `role="row"` carrying the option's id, `aria-selected` and `aria-disabled`, with exactly two `gridcell` children: the option's content, then its action or an empty cell. A section is a `rowgroup` named by its title. Pressing the option cell toggles; pressing the action fires the caller's control and nothing else. Up/Down move rows onto the option cell; the inline-end arrow moves to the action cell and the inline-start arrow back, following visual direction under RTL; Enter on the action cell activates the control. `aria-activedescendant` names the row or the action cell. No control inside the popup is a tab stop. | `spec:AST-058` FR1–FR8, AR1–AR4 | |
There was a problem hiding this comment.
The search row's clear button is a tab stop inside the popup while a query is typed, grid or not. spec:AST-058 FR5 says "No action control is in the sequential tab order"; suggest the same here: "No action control is a tab stop."
| | FR8 | With `renderTrigger`, the caller-rendered control receives `ref`, `id`, `onClick`, `onKeyDown`, `onFocus`, `aria-haspopup`, `aria-expanded`, `aria-controls`, and `aria-busy`; the panel is anchored to it, focus returns to it on close, the listbox is named by `label`, and the listbox takes focus and owns the keyboard on open. No Field, status, clear action, spinner, or indicator renders. `handleRef` exposes `open`, `close`, `toggle`, `isOpen`; `onOpenChange` fires on every open and close. | `spec:AST-055` FR1–FR7, AR1–AR4; DEC-1; current docs and tests | | ||
| | FR9 | With `hasSearch` and `hasCreate`, once options have loaded, a `Create "<query>"` row leads the list when the trimmed query equals no option label under the search's own case-insensitive matching. Picking it, or Enter with nothing highlighted, calls `onChange` with the query appended to the value and a `{type: 'create', query}` descriptor, clears the search, and announces the creation; every other change passes no descriptor. The row has no checkbox, is never selected, is excluded from select-all and from `renderOption`, and is announced instead of the empty message when it is the only result. While `isLoading`, no row is offered and Enter commits nothing. `hasCreate` without `hasSearch` warns in development and offers nothing. | Current behavior; `spec:AST-056` FR5 and DEC-3 | | ||
| | FR10 | Once any declared option — at any depth of sections, before any query filters it — carries an `action`, the popup is a `role="grid"` and stays one while mounted; the trigger advertises `aria-haspopup="grid"` (the bottom sheet keeps `dialog`). Each option, the select-all row included, is a `role="row"` carrying the option's id, `aria-selected` and `aria-disabled`, with exactly two `gridcell` children: the option's content, then its action or an empty cell. A section is a `rowgroup` named by its title. Pressing the option cell toggles; pressing the action fires the caller's control and nothing else. Up/Down move rows onto the option cell; the inline-end arrow moves to the action cell and the inline-start arrow back, following visual direction under RTL; Enter on the action cell activates the control. `aria-activedescendant` names the row or the action cell. No control inside the popup is a tab stop. | `spec:AST-058` FR1–FR8, AR1–AR4 | | ||
| | FR11 | No surface renders native `disabled`. A disabled trigger keeps its tab stop with `aria-disabled`; no press or key opens it or reaches a caller or ancestor handler; it submits no value; a `disabledMessage` Tooltip is linked from the trigger through `aria-describedby`; and only `isDisabled` takes disabled paint. While busy (`isLoading` or a pending `changeAction`) the trigger shows a Spinner and `aria-busy`, keeps its ordinary paint, and announces each busy start once. | `spec:AST-085` FR1–FR6; `spec:AST-086` FR8; `family:input-fields` FR4–FR6 | |
There was a problem hiding this comment.
A disabled trigger stops presses (MultiSelector.tsx:2490-2494) and drops the caller's handlers, but a keydown still bubbles to an ancestor onKeyDown: Enter, ArrowDown and letters all arrive. Suggest: "no press opens it or reaches a caller or ancestor handler, and no key opens it or reaches a caller handler".
| | FR9 | With `hasSearch` and `hasCreate`, once options have loaded, a `Create "<query>"` row leads the list when the trimmed query equals no option label under the search's own case-insensitive matching. Picking it, or Enter with nothing highlighted, calls `onChange` with the query appended to the value and a `{type: 'create', query}` descriptor, clears the search, and announces the creation; every other change passes no descriptor. The row has no checkbox, is never selected, is excluded from select-all and from `renderOption`, and is announced instead of the empty message when it is the only result. While `isLoading`, no row is offered and Enter commits nothing. `hasCreate` without `hasSearch` warns in development and offers nothing. | Current behavior; `spec:AST-056` FR5 and DEC-3 | | ||
| | FR10 | Once any declared option — at any depth of sections, before any query filters it — carries an `action`, the popup is a `role="grid"` and stays one while mounted; the trigger advertises `aria-haspopup="grid"` (the bottom sheet keeps `dialog`). Each option, the select-all row included, is a `role="row"` carrying the option's id, `aria-selected` and `aria-disabled`, with exactly two `gridcell` children: the option's content, then its action or an empty cell. A section is a `rowgroup` named by its title. Pressing the option cell toggles; pressing the action fires the caller's control and nothing else. Up/Down move rows onto the option cell; the inline-end arrow moves to the action cell and the inline-start arrow back, following visual direction under RTL; Enter on the action cell activates the control. `aria-activedescendant` names the row or the action cell. No control inside the popup is a tab stop. | `spec:AST-058` FR1–FR8, AR1–AR4 | | ||
| | FR11 | No surface renders native `disabled`. A disabled trigger keeps its tab stop with `aria-disabled`; no press or key opens it or reaches a caller or ancestor handler; it submits no value; a `disabledMessage` Tooltip is linked from the trigger through `aria-describedby`; and only `isDisabled` takes disabled paint. While busy (`isLoading` or a pending `changeAction`) the trigger shows a Spinner and `aria-busy`, keeps its ordinary paint, and announces each busy start once. | `spec:AST-085` FR1–FR6; `spec:AST-086` FR8; `family:input-fields` FR4–FR6 | | ||
| | FR12 | ArrowDown on the focused trigger opens the list; ArrowDown and ArrowUp move the highlight; Home and End jump to the first and last option without search, while with search they move the caret and PageDown and PageUp jump the highlight; Enter toggles the highlighted option except during input-method composition; Escape closes; Tab closes and moves focus on; with `hasClear` and without `hasSearch`, Delete or Backspace on the focused, closed trigger clears every value when one is selected. | Current source and tests | |
There was a problem hiding this comment.
With hasSearch and a typed query, forward Tab from the search input moves to the search row's clear button and keeps the list open (MultiSelector.tsx:2044-2048, asserted by MultiSelector.test.tsx:3192-3219). Suggest: "Tab closes and moves focus on, except that while a query shows the search's clear button, forward Tab moves to that button and keeps the list open".
| | FR10 | Once any declared option — at any depth of sections, before any query filters it — carries an `action`, the popup is a `role="grid"` and stays one while mounted; the trigger advertises `aria-haspopup="grid"` (the bottom sheet keeps `dialog`). Each option, the select-all row included, is a `role="row"` carrying the option's id, `aria-selected` and `aria-disabled`, with exactly two `gridcell` children: the option's content, then its action or an empty cell. A section is a `rowgroup` named by its title. Pressing the option cell toggles; pressing the action fires the caller's control and nothing else. Up/Down move rows onto the option cell; the inline-end arrow moves to the action cell and the inline-start arrow back, following visual direction under RTL; Enter on the action cell activates the control. `aria-activedescendant` names the row or the action cell. No control inside the popup is a tab stop. | `spec:AST-058` FR1–FR8, AR1–AR4 | | ||
| | FR11 | No surface renders native `disabled`. A disabled trigger keeps its tab stop with `aria-disabled`; no press or key opens it or reaches a caller or ancestor handler; it submits no value; a `disabledMessage` Tooltip is linked from the trigger through `aria-describedby`; and only `isDisabled` takes disabled paint. While busy (`isLoading` or a pending `changeAction`) the trigger shows a Spinner and `aria-busy`, keeps its ordinary paint, and announces each busy start once. | `spec:AST-085` FR1–FR6; `spec:AST-086` FR8; `family:input-fields` FR4–FR6 | | ||
| | FR12 | ArrowDown on the focused trigger opens the list; ArrowDown and ArrowUp move the highlight; Home and End jump to the first and last option without search, while with search they move the caret and PageDown and PageUp jump the highlight; Enter toggles the highlighted option except during input-method composition; Escape closes; Tab closes and moves focus on; with `hasClear` and without `hasSearch`, Delete or Backspace on the focused, closed trigger clears every value when one is selected. | Current source and tests | | ||
| | FR13 | The field has no root styling props: `control*` reaches the bordered control box around the trigger and `container*` the Field box (`family:input-fields` FR10); a press on the control box outside the trigger opens it; a caller `onClick` runs after the built-in toggle and a caller `onKeyDown` after the built-in keyboard model, and only for a key that model leaves unhandled; a caller `aria-describedby` joins the field description and a caller `aria-labelledby` follows the field's own name. With `renderTrigger`, `container*` and `control*` have no effect and warn. The trigger height is the shared element size for its size and MUST grow with a set text size (`spec:AST-102` FR4). Implementation gap: the height is fixed. | `family:input-fields` FR10; `spec:AST-102` FR4; current tests | |
There was a problem hiding this comment.
The caller still receives Tab and typeahead letters after the model acts on them, because hooks.ts:279-283 and :335-358 do not cancel them; only cancelled keys are skipped (utils/composeEventHandlers.ts:36-41). The prop's JSDoc already states the rule, "is skipped for keys that model handles and cancels" (MultiSelector.tsx:670-673); suggest that wording.
…eys, and open focus to what ships
There was a problem hiding this comment.
This review is on hold for a spec decision: promoting MultiSelector.spec.md to current adds requirements no current record states yet (FR9's create-row rules from the removed DEC-2, FR12, FR13's control-box press and caller-key rule, FR14's selection announcements, and PR1). An engineering owner records approval by commenting /approve-spec <full head SHA>, and the author may do so as an engineering owner. FR8, FR10, FR11, FR12 and FR13 now read as shipped, but two record fixes remain and a new commit resets approval, so please push them before that comment:
- FR2, FR9 and FR14 treat
isLoadingas "options have not arrived" (no empty message, no create row, no search announcement), whichspec:AST-001FR4 forbids; please cite AST-001 and record these as an implementation gap, as FR13 does for the trigger height. - The Accessibility contract calls the create row a
role="option", but in a grid it is arole="row"with an empty action cell, asspec:AST-058FR3 requires.
[Automated review]
| | FR6 | Clear glyphs delegate exclusively to the shared `input-clear-icon` target; removed per-component aliases do not claim anatomy. | Current public target metadata | | ||
| | FR7 | While `isReadOnly` is true, selected values remain focusable and form-submittable, while the selection surface, clear action, disclosure indicator, and every value-change path are unavailable. | `spec:AST-011` DEC-1; current docs and tests | | ||
| | FR8 | With `renderTrigger`, the caller-rendered control receives `ref`, `id`, `onClick`, `onKeyDown`, `onFocus`, `aria-haspopup`, `aria-expanded`, `aria-controls`, and `aria-busy`; the panel is anchored to it, focus returns to it on close, the listbox is named by `label`, and on open the search input takes focus with `hasSearch`, and otherwise the listbox, which then owns the keyboard. No Field, status, clear action, spinner, or indicator renders. `handleRef` exposes `open`, `close`, `toggle`, `isOpen`; `onOpenChange` fires on every open and close. | `spec:AST-055` FR1–FR7, AR1–AR4; DEC-1; current docs and tests | | ||
| | FR9 | With `hasSearch` and `hasCreate`, once options have loaded, a `Create "<query>"` row leads the list when the trimmed query equals no option label under the search's own case-insensitive matching. Picking it, or Enter with nothing highlighted, calls `onChange` with the query appended to the value and a `{type: 'create', query}` descriptor, clears the search, and announces the creation; every other change passes no descriptor. The row has no checkbox, is never selected, is excluded from select-all and from `renderOption`, and is announced instead of the empty message when it is the only result. While `isLoading`, no row is offered and Enter commits nothing. `hasCreate` without `hasSearch` warns in development and offers nothing. | Current behavior; `spec:AST-056` FR5 and DEC-3 | |
There was a problem hiding this comment.
FR9 offers the create row "once options have loaded" and none while isLoading (the concept row at line 119 says "options loaded"), and FR2 (line 127) and FR14 (line 139) also withhold the empty message and search announcements while loading. spec:AST-001 FR4 says option-source pending "MUST NOT be inferred from zero options or input isLoading", and its Current-state impact names this suppression as the gap to remove. Suggest citing spec:AST-001 in system_specs and the governing rules, and recording these three behaviors as an implementation gap against AST-001 FR4, as FR13 does for spec:AST-102 FR4, with a Current-state impact line.
| typeahead keys while open, so an anchor that cannot take focus still leaves a | ||
| keyboard path. | ||
|
|
||
| The create row is a `role="option"` with `aria-selected="false"` and no checkbox |
There was a problem hiding this comment.
In a grid this row is a role="row" with an empty action cell (MultiSelector.tsx:2131; the "create row inside a grid" test in MultiSelector.test.tsx), as spec:AST-058 FR3 requires. Suggest: "The create row is a role="option", or a role="row" with an empty action cell in a grid (FR10), with aria-selected="false" and no checkbox indicator."
…inst AST-001 and the grid create row
There was a problem hiding this comment.
This review is on hold for a spec decision: promoting MultiSelector.spec.md to current adds requirements no current record states yet (FR9's create-row rules from the removed DEC-2, FR12, FR13's control-box press and caller-key rule, FR14's selection announcements, and PR1). An engineering owner records approval by commenting /approve-spec <full head SHA>, and the author may do so as an engineering owner. The isLoading gap and the grid create row now read correctly, but one record fix remains and a new commit resets approval, so please push it before that comment:
- FR2, the Empty state design row and the FR1/FR2 verification row require the "No options" panel a MultiSelector with no options opens today, while
spec:AST-001FR3 and FR6 call for a read-only no-choice state; please record it as an implementation gap against AST-001 FR3 and FR6, as FR2 already does for loading.
[Automated review]
| | ID | Invariant | Basis | | ||
| | ---- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------- | | ||
| | FR1 | The current standalone presentation contains a Field and painted Trigger; the supported InputGroup path renders the same Trigger without a Field. Optional start content, clear action, status icon, or Indicator icon follows supplied props and state. | Current source, docs, and tests | | ||
| | FR2 | The shared panel content contains the optional Search row, zero or more Option rows, optional public Option dividers and Section headings, and an Empty state when the applicable result set is empty and the value is not loading. Option-source pending MUST be supplied independently and MUST NOT be inferred from input `isLoading` (`spec:AST-001` FR4). Implementation gap: `isLoading` is read as options not yet arrived, so it withholds the Empty state, the create row (FR9), and search announcements (FR14). | Current source, docs, and tests; `spec:AST-001` FR4 | |
There was a problem hiding this comment.
With options={[]} and no isLoading, the shipped trigger opens a listbox showing emptyText ("No options"; MultiSelector.tsx:2262-2272). This row, the Empty state design row at line 226 and the FR1/FR2 verification row at line 318 make that a requirement, but spec:AST-001 FR3 and FR6 call for a read-only no-choice state "without requiring a person to activate an unavailable popup", and AST-001's Current-state impact names this gap. Please add it to this row's implementation gap (citing AST-001 FR3 and FR6), add a Current-state impact line, and limit the design row's "no-options" content to that gap.
…ST-001 FR3 and FR6
There was a problem hiding this comment.
This spec promotion is on hold only for an engineering-owner decision: making MultiSelector.spec.md current adds requirements no current record states yet (FR9's create-row rules from the removed DEC-2, FR12, FR13's control-box press and caller-key rule, FR14's selection announcements, and PR1). The zero-option panel is now recorded as a gap against spec:AST-001 FR3 and FR6, and no other record fix remains. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; a new commit resets that approval.
[Automated review]
|
/approve-spec 4badbfe |
github-actions
Bot
removed
the
needs:spec-owner-review
Exact decision needed
Promote
component:MultiSelectorfromdrafttocurrent, recording its shipped behavior. No behavior, API, default, or DOM change.Existing authority searched
pnpm specs:for packages/core/src/MultiSelector/MultiSelector.tsx→ current records that govern MultiSelector:family:input-fields,family:overlay-dismissal,spec:AST-001,spec:AST-011DEC-1,spec:AST-056,spec:AST-058,spec:AST-079DEC-1,spec:AST-085,spec:AST-086,spec:AST-102,architecture:component-theming-surface,architecture:layer-runtime,architecture:public-component-api, and the composed Field, CheckboxInput, Divider, Icon, Text, and BottomSheet records.MultiSelector.spec.mdexisted as adraft.MultiSelector.spec.md: fix(Selector, MultiSelector): a query change no longer keeps the highlight from the previous list #8871 rebases on top of this record (query change resets the highlight). feat(core): in Chrome on Android a Tall BottomSheet rides Gboard's slides #8566, fix(MultiSelector): a search status the next keystroke replaces is withdrawn, not joined #8640, fix(Selector, MultiSelector): a closed list opens on the selected option, never a disabled one #8868, fix(MultiSelector): translate the default count text #9017, and fix(InputGroup): an error status on the group marks its members invalid #9030 touch MultiSelector source and rebase on top.Decision
MultiSelector.spec.mdbecomes current (approved_by: cixzhang,approved_at: 2026-10-31), template 7, written from main today with existing content and attribution kept; DEC-1 now citesspec:AST-055for the caller-rendered trigger, as AST-055 asks, and keeps only thehandleRef-over-isOpenchoice; the misplacedoption actionconcept row moves to Public concepts. It adds the shipped disabled and busy rules (FR11), keyboard (FR12),container*/control*and handler composition (FR13), and announcements and form submission (FR14), and records the fixed trigger height as "MUST … Implementation gap".*.spec.md:docs/families/input-fields.mdloses MultiSelector's row from the FR10 "Prop targets: current components do not yet meet FR10" gap table, a current-state line about this lane's component that is now stale (MultiSelector shipscontainer*andcontrol*with no root styling,MultiSelector.fr10.test.tsx). No rule, invariant, matrix, or decision is edited; Prettier re-flowed the table's column widths.Rejected alternatives
None.
Claim scope and non-goals
Shipped behavior only, traced from the MultiSelector sources, docs, and tests. No code is fixed.
Evidence and implementation relationship
MultiSelector.test.tsx,MultiSelector.fr10.test.tsx,MultiSelector.focusableDisabled.test.tsx,CreateRow.a11y.chromium.spec.ts,OptionActions.a11y.chromium.spec.ts,BusyOwnerAnnouncement.test.tsx, andthemingTargets.test.ts; each verification row names its cases and lists its evidence gaps.Current-state impact:
isLoadingwithholds the Empty state, the create row, and search announcements, againstspec:AST-001FR4; a field with zero options opens a "No options" list instead of a read-only no-choice state, against FR3 and FR6.spec:AST-102FR4.Writing rubric results
Scope
Validation
pnpm check:knowledge: alignedgit diff --check: clean