Repository navigation
docs(spec): CommandPalette records its shipped contract as current - #9306
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 spec update needs an engineering owner's decision before it can be approved. The six CommandPalette records become current only with an owner's approval of this exact head, and that owner must also settle whether a caller's explicit role or aria-label may replace the listbox and option roles (List FR2 and Item FR1). INV5 says component-owned accessibility "cannot be accidentally overwritten", and the records' own List OQ2 and Item OQ3 leave this open. 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; fix the Empty verification map first, because a content change resets the approval.
CommandPaletteEmpty.spec.mdline 173 says message contrast below 4.5:1 fails the audit, butCommandPaletteEmptyAudit.a11y.chromium.spec.tsrecords contrast without failing on it, and it does check the FR4 320 px wrap that the record lists as having no focused test.
[Automated review]
| | ------------- | -------------------------------------------------------- | ---------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------- | | ||
| | FR1, FR5–FR6 | `CommandPaletteEmpty.test.tsx`; `themingTargets.test.ts` | text and element children; root inputs | A dropped input, a moved target, or lost children fails the focused assertions. | `audit:CommandPaletteEmpty/§2,§3,§4` | | ||
| | FR2, FR3, AR1 | `CommandPalette.test.tsx`; `CommandPaletteList.test.tsx` | bootstrap, search, rich content, modal and inline | Exposing a listbox, adding a role, or announcing other text than the rendered message fails assertions. | `audit:CommandPaletteEmpty/§1,§4` | | ||
| | FR4, AR2 | `CommandPaletteEmptyAudit.a11y.chromium.spec.ts` | Narrow and wide viewports; light and dark; forced colors; rich content | A case with horizontal overflow, or message text below 4.5:1 contrast outside forced colors, fails the audit manifest test. | `audit:CommandPaletteEmpty/accessibility` | |
There was a problem hiding this comment.
CommandPaletteEmptyAudit.a11y.chromium.spec.ts records message contrast as "Recorded, not gating" and leaves it out of passed, so contrast below 4.5:1 does not fail the manifest test. Consider saying overflow fails and contrast is recorded only. FR4's 320 px wrap is covered by the gated content cases, so it can come off the no-test list below.
| | ID | Invariant | Basis | | ||
| | --- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | ||
| | FR1 | Native props and `ref` reach the listbox root, and consumer `className` and `style` combine with the list target and base styles. Inside CommandPalette the root id is the context list id; standalone, a caller `id` is kept. | Current source and tests | | ||
| | FR2 | Standalone, and inside CommandPalette while at least one selectable result exists, the root exposes `role=listbox` with an accessible name from `label`, falling back to the translated default; a caller's explicit `role` or `aria-label` reaches the root (`architecture:public-component-api` INV5). Implementation gap: the component's role and name replace a caller's (Current-state impact; OQ2). Otherwise AR4 applies. | Current source and tests | |
There was a problem hiding this comment.
This reverses the earlier rule that consumer role and aria-label do not replace the listbox role and name. INV5 also says component-owned accessibility cannot be accidentally overwritten, and OQ2 below leaves the composite-role exemption open, so this needs an engineering owner's ruling before it becomes current.
df2e60c to
f7cb4f4
Compare
There was a problem hiding this comment.
This spec update still needs an engineering owner's decision before it can be approved. The six CommandPalette records become current only with an owner's approval of this exact head, and that owner must also settle whether a caller's explicit role or aria-label may replace the listbox and option roles (List FR2 and Item FR1), which INV5's "cannot be accidentally overwritten" and the records' own List OQ2 and Item OQ3 leave open. 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. The Empty verification row is now accurate; two CommandPalette rows (CommandPalette.spec.md lines 249 and 255) still name browser evidence their Verification column no longer lists, so make any wording fix before approving, because an edit resets the approval.
[Automated review]
| | ------------------- | -------------------------------------------------------------------------- | ---------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------- | | ||
| | FR1, AR1 | CommandPalette root and subcomponent suites | Default, grouped, empty, pending, rich Empty, and replaced slots | Exposing an empty listbox, leaving empty Input popup attributes, or nesting rich Empty controls under option semantics fails focused role and hosted axe assertions. | `audit:CommandPalette/anatomy` | | ||
| | FR2 | Focused subcomponent suites and `themingTargets.test.ts` | Seven local targets at their owned elements | Removing or moving a current target fails a focused placement assertion or the global inventory. | `audit:CommandPalette/theming` | | ||
| | FR3 | `CommandPaletteInput.test.tsx` and `CommandPaletteFooter.test.tsx` | Default Search glyph, pending spinner, and Footer Kbd | Replacing or removing the delegated Icon, Spinner, or Kbd instance fails a composed-owner assertion and changes the exact-head evidence inventory. | `audit:CommandPalette/theming` | |
There was a problem hiding this comment.
This row (and the Theming anatomy map row at line 255) still names browser evidence ("exact-head evidence inventory", "browser sensor counts") that the Verification column no longer lists. CommandPalette.a11y.chromium.spec.ts is still in verified_by, so naming it here, or dropping the browser clause, would keep the map consistent.
f7cb4f4 to
72f5c84
Compare
There was a problem hiding this comment.
This spec update still needs an engineering owner's decision before it can be approved. The six CommandPalette records become current only with an owner's approval of this exact head, and that owner must also settle whether a caller's explicit role or aria-label may replace the listbox and option roles (List FR2 and Item FR1), which INV5's "cannot be accidentally overwritten" and the records' own List OQ2 and Item OQ3 leave open. 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. The CommandPalette verification rows are now consistent; if the ruling goes against caller-wins, reword List FR2, Item FR1, CommandPalette AR1 and the readiness row at CommandPalette.spec.md:49 before approving, because an edit resets the approval.
[Automated review]
| | Behavior | Searchable commands in a Dialog: a combobox query field, a listbox of results only while selectable results exist, an empty message otherwise, and a keyboard-guidance footer. Modal Escape belongs to Dialog's shared dismissal. | | ||
| | End-user impact | Keyboard and screen-reader users search and run commands with native text editing kept intact, hear the empty message that is shown, and close nested layers one Escape at a time. | | ||
| | Builder impact | None. | | ||
| | Compatibility/readiness | Records shipped behavior; no public change. No implementation gap; one owner question stays open. | |
There was a problem hiding this comment.
This row says "No implementation gap", but AR1 (lines 143-148) now states that a caller's role or aria-label reaches List and Item, which today's parts do not meet. If caller-wins stays, name that gap here; if it goes, drop AR1's new sentence.
72f5c84 to
54e30e1
Compare
There was a problem hiding this comment.
This spec update needs an engineering owner's decision before it can be approved: the six CommandPalette records become current only with an owner's approval of this exact head. That owner also needs to settle whether a caller's explicit role or aria-label may replace the listbox and option roles (List FR2, Item FR1, CommandPalette AR1), which INV5's "cannot be accidentally overwritten", the records' own List OQ2 and Item OQ3, and the current CommandPaletteInput AR3, which keeps the combobox role over a caller's inside the same palette, leave open. 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. If the ruling goes against caller-wins, reword those rules and the List and Item review checks before approving, because an edit resets the approval.
[Automated review]
| | FR5 | Children provide the option's label and supporting presentation, without nested interactive descendants. | Option semantics and `spec:AST-020/FR1,FR2` | Documented; exact-head a11y remains a gate | | ||
| | ID | Invariant | Basis | | ||
| | --- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------- | | ||
| | FR1 | Native props and `ref` reach the option root, and a caller's explicit `role` reaches it (`architecture:public-component-api` INV5). CommandPalette context owns registered-item id, selected state, disabled state, and value marker; standalone caller id persists. Implementation gap: `role="option"` replaces a caller's role (Current-state impact; OQ3). | Current source and tests; `architecture:public-component-api` | |
There was a problem hiding this comment.
FR1 drops role from what CommandPalette context owns and lets a caller's role replace option even inside the palette. The current CommandPaletteInput record reads INV5 the other way for the same composite: its AR3 says that with CommandPalette context the field owns role and the popup attributes, "and these replace the same native passthrough props". The OQ3 ruling should cover both, or FR1 keeps role under context ownership until it does.
e5ac990 to
dec2203
Compare
There was a problem hiding this comment.
This spec update needs an engineering owner's decision before it can be approved. The six CommandPalette records become current only once an engineering owner approves this exact head by commenting /approve-spec <full head SHA> on the pull request; the author may do so as an engineering owner. That approval settles three rules that no current record states: List FR2's caller aria-label naming the List, Group FR1's caller role replacing group, and standalone listbox and option role ownership in List FR2 and Item FR1. Two record fixes are needed either way:
- List FR2 and Item FR1 cite
component:CommandPaletteInputAR3 for the standalone case, but AR3 covers only the in-palette field and keeps standalone caller roles intact, so state the standalone rule as the record's own. - List FR4's Basis says "Current source and tests", but the same record lists FR4 as read from source with no focused test.
[Automated review]
| | ID | Invariant | Basis | | ||
| | --- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | ||
| | FR1 | Native props and `ref` reach the listbox root, and consumer `className` and `style` combine with the list target and base styles. Inside CommandPalette the root id is the context list id; standalone, a caller `id` is kept. | Current source and tests | | ||
| | FR2 | Standalone, and inside CommandPalette while at least one selectable result exists, the root exposes `role=listbox`, which a caller's `role` does not replace (composite semantics are component-owned and replace passthrough, as `component:CommandPaletteInput` AR3 records), named by a caller's explicit `aria-label`, else `label`, else the translated default (`architecture:public-component-api` INV5). Implementation gap: the component's name replaces a caller's `aria-label` (Current-state impact). Otherwise AR4 applies. | Current source and tests | |
There was a problem hiding this comment.
component:CommandPaletteInput AR3 covers only the field "With CommandPalette context" and says "Standalone caller-owned roles and relationships remain intact." Citing it for the standalone List's listbox role points readers at the opposite rule. Scope the AR3 citation to the in-palette case and state the standalone rule as this record's own.
| | FR1 | Native props and `ref` reach the listbox root, and consumer `className` and `style` combine with the list target and base styles. Inside CommandPalette the root id is the context list id; standalone, a caller `id` is kept. | Current source and tests | | ||
| | FR2 | Standalone, and inside CommandPalette while at least one selectable result exists, the root exposes `role=listbox`, which a caller's `role` does not replace (composite semantics are component-owned and replace passthrough, as `component:CommandPaletteInput` AR3 records), named by a caller's explicit `aria-label`, else `label`, else the translated default (`architecture:public-component-api` INV5). Implementation gap: the component's name replaces a caller's `aria-label` (Current-state impact). Otherwise AR4 applies. | Current source and tests | | ||
| | FR3 | Inside CommandPalette, the query field's `aria-controls` names the listbox id. | Current source and tests | | ||
| | FR4 | The root lays out children in one column with spacing-scale padding and gaps, grows within a flex parent, caps its height at the parent's height, and scrolls overflowing children inside itself. | Current source and tests | |
There was a problem hiding this comment.
The Basis says "Current source and tests", but this record lists FR4 under "Read from source, with no focused test yet" and CommandPaletteList.test.tsx has no layout or scrolling test. Suggest "Current source".
| | FR5 | Children provide the option's label and supporting presentation, without nested interactive descendants. | Option semantics and `spec:AST-020/FR1,FR2` | Documented; exact-head a11y remains a gate | | ||
| | ID | Invariant | Basis | | ||
| | --- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------- | | ||
| | FR1 | Native props and `ref` reach the option root. The `option` role, selected state, and disabled state replace a caller's (composite semantics are component-owned and replace passthrough, as `component:CommandPaletteInput` AR3 records). CommandPalette context owns the registered-item id and value marker; a standalone caller id persists. | Current source and tests; `architecture:public-component-api` | |
There was a problem hiding this comment.
FR1 applies the protected option role to standalone items too (the "protects owned option semantics" test renders one without CommandPalette), but component:CommandPaletteInput AR3 governs only the in-palette field and keeps standalone caller roles intact. State the standalone rule here as this record's own instead of attributing it to AR3.
dec2203 to
6e0fde3
Compare
There was a problem hiding this comment.
This spec update needs an engineering owner's decision before it can be approved: the six CommandPalette records become current only once an engineering owner approves this exact head by commenting /approve-spec <full head SHA> on the pull request, and the author may do so as an engineering owner. That approval settles three rules no current record states yet: List FR2's caller aria-label naming the List, Group FR1's caller role replacing group, and the standalone listbox and option protection in List FR2 and Item FR1. Both record fixes from the previous round are in. Two optional wording fixes remain; make them before approving, because a new commit resets the approval:
CommandPalette.spec.md:254says the FR1/AR1 mutations fail "hosted axe assertions", but its Verification column no longer lists any axe check.CommandPaletteEmpty.spec.md:151-152says no anatomy map is recorded, while CommandPalette's aggregate map already maps Empty; point at it as the Group and Footer records do.
[Automated review]
| | Theming anatomy map | `scripts/check-knowledge.mjs` and exact-head Chromium evidence | Canonical anatomy and seven current local targets | Missing, extra, prefixed, stale, alias-backed, or visually absent mappings fail repository validation or browser sensor counts. | `audit:CommandPalette/theming` | | ||
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | ------------------- | -------------------------------------------------------------------------- | ---------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------- | | ||
| | FR1, AR1 | CommandPalette root and subcomponent suites | Default, grouped, empty, pending, rich Empty, and replaced slots | Exposing an empty listbox, leaving empty Input popup attributes, or nesting rich Empty controls under option semantics fails focused role and hosted axe assertions. | `audit:CommandPalette/anatomy` | |
There was a problem hiding this comment.
The Verification column now lists only the unit suites, but this row still says the mutations fail "hosted axe assertions", and none of those suites runs axe. Name the Storybook accessibility audit here, or end the sentence at "focused role assertions". Optional; make it before approving, since a new commit resets the approval.
| No component-local design decision is recorded. No anatomy-theming map is | ||
| recorded yet; the block is optional during migration, so the map is deferred. |
There was a problem hiding this comment.
CommandPalette's aggregate map already maps Empty to command-palette-empty. Suggest the wording the Group and Footer records use: "The aggregate CommandPalette anatomy map, owned by component:CommandPalette, maps the Empty part." Optional.
6e0fde3 to
ef25c7e
Compare
There was a problem hiding this comment.
This spec update needs an engineering owner's decision before it can be approved: the six CommandPalette records become current only once an engineering owner approves this exact head by commenting /approve-spec <full head SHA> on the pull request, and the author may do so as an engineering owner. That approval settles three rules no current record states yet: List FR2's caller aria-label naming the List, Group FR1's caller role replacing group, and the listbox and option protection in List FR2 and Item FR1. Both wording fixes from the previous round are in. One optional consistency fix remains; make it before approving, because a new commit resets the approval:
CommandPaletteList.spec.mdstill names onlylabeland the translated default as name sources at lines 42, 80-82, 103 and 126, while FR2, ORD1 and AR1 put a caller'saria-labelfirst; align them with whichever rule the owner keeps.
[Automated review]
| | ID | Invariant | Basis | | ||
| | --- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | ||
| | FR1 | Native props and `ref` reach the listbox root, and consumer `className` and `style` combine with the list target and base styles. Inside CommandPalette the root id is the context list id; standalone, a caller `id` is kept. | Current source and tests | | ||
| | FR2 | Standalone, and inside CommandPalette while at least one selectable result exists, the root exposes `role=listbox`, which a caller's `role` does not replace: inside CommandPalette because composite semantics are component-owned and replace passthrough, as `component:CommandPaletteInput` AR3 records, and standalone by this record's own rule, named by a caller's explicit `aria-label`, else `label`, else the translated default (`architecture:public-component-api` INV5). Implementation gap: the component's name replaces a caller's `aria-label` (Current-state impact). Otherwise AR4 applies. | Current source and tests | |
There was a problem hiding this comment.
Optional: FR2 puts a caller's aria-label first, but lines 42, 80-82, 103 (the Accessible name concept's closed values) and 126 still name only label and the translated default. Align them with whichever rule the owner keeps, before approving, since a new commit resets the approval.
ef25c7e to
589168e
Compare
There was a problem hiding this comment.
The six CommandPalette records describe what the components ship today, with the shortfalls named as gaps, and I found no defects, but approval waits on an engineering owner's decision because no current record yet states three of the rules they introduce. The rules to settle: List FR2 lets a builder's aria-label name the List (recording today's replacement as a gap), List FR2 and Item FR1 keep listbox and option protected from a builder's role (inside the palette by analogy with CommandPaletteInput AR3, which covers only the query field), and Group FR1 and FR4 let a builder's role replace group. 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; otherwise, say which of those rules should change.
[Automated review]
| | FR2, AR3 | `CommandPaletteItem.test.tsx` | enabled; disabled; consumer-cancelled; contextual | Disabled consumer activation, ignored cancellation, missing local report, selection, or close fails tests. | `audit:CommandPaletteItem/behavior` | | ||
| | FR4, PR2 | `CommandPaletteItem.test.tsx`, `CommandPalette.test.tsx` | standalone; inline initial; changed highlight | Lost nearest scrolling, duplicate contextual scrolling, or initial inline scroll fails Effect assertions. | `audit:CommandPaletteItem/behavior` | | ||
| | Theme/RTL | focused class assertion, `themingTargets.test.ts`, RTL verified N/A | all visual states | Target loss fails inventory/DOM tests; any new directional behavior requires a non-empty RTL dimension. | `audit:CommandPaletteItem/theming` | | ||
| | FR5, AR4 | Source review | Item children | Gap: no check fails if item children contain a nested interactive descendant. | `audit:CommandPaletteItem/accessibility` | |
There was a problem hiding this comment.
Optional: CommandPaletteItemAudit.a11y.chromium.spec.ts (lines 273-275) already fails when an option in its stories contains a button, a or [tabindex], though it runs only for Item audit pull requests. Naming it here, scoped to the stories, would make this row exact; caller content stays unchecked.
|
/approve-spec 589168e |
github-actions
Bot
removed
the
needs:spec-owner-review
Exact decision needed
Promote
component:CommandPalette,component:CommandPaletteList,component:CommandPaletteItem, andcomponent:CommandPaletteEmptyfrom draft to current, and add new current records forcomponent:CommandPaletteGroupandcomponent:CommandPaletteFooter.component:CommandPaletteInputis already current and unchanged. No behavior, prop, default, target, or export changes.Existing authority searched
family:overlay-dismissal,spec:AST-056,spec:AST-017,spec:AST-020,architecture:interaction-modality,architecture:public-component-api, andarchitecture:component-theming-surfacegovern the records;component:Dialog,component:Icon,component:Spinner, andcomponent:Kbdown delegated parts.Decision
All six
authority: current, approved by cixzhang. Drafts keep their content and owners with draft and audit wording removed. Behavior that does not meet a current rule is recorded as a gap:aria-labelwith its own name (architecture:public-component-apiINV5: a caller's explicit name wins).The
listboxandoptionroles and option state are component-owned and replace a caller's, which the currentcomponent:CommandPaletteInputAR3 already records for the same composite; List and Item cite it, and the Item test that protects option semantics verifies that contract. Group lets a caller'sroleoraria-labelwin, which is conformant. CommandPalette OQ2 asks whether INV5 should state the composite-semantics reading family-wide. These follow existing authority; no new decision.CommandPalette's "Does not own" list now points at the Group, Footer, and Empty records.
Rejected alternatives
None.
Claim scope and non-goals
Open questions on the query-field target, the standalone keyboard model, and whether INV5 states the composite-semantics reading family-wide stay with their owners. Claims read from source without a focused test are listed under each verification map.
Evidence and implementation relationship
CommandPalette.test.tsx,CommandPaletteInput.test.tsx,CommandPaletteList.test.tsx,CommandPaletteItem.test.tsx,CommandPaletteEmpty.test.tsx,CommandPaletteGroup.test.tsx,CommandPaletteFooter.test.tsx,CommandPaletteFooter.a11y.chromium.spec.ts,CommandPaletteEmptyAudit.a11y.chromium.spec.ts, andthemingTargets.test.ts.Writing rubric results
Scope
Validation
pnpm check:knowledge: alignedgit diff --check: clean