Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: 2 accessibility violation(s) found — 2 serious. Radio group pattern - 1 issue(s)
RadioList - 1 issue(s)
Visual Regression46 of 496 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 26 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR18 and DEC-13 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so the DropdownMenu owner needs to approve or reject that contract, including label taking precedence over a visible title and single radio sets no longer using a DropdownMenuGroup wrapper. Whatever that decision, three fixes are needed, and the branch needs a rebase because main now conflicts in DropdownMenu.spec.md and DropdownMenuSelectable.test.tsx:
- Pass a caller's
aria-labelthrough whenlabelis omitted (DropdownMenuRadioGroup.tsxline 151 replaces it withundefined), and count it in the warning condition. - Describe the result in the changeset and the story comment as one group named by its heading, or name the NVDA + Chrome and VoiceOver + Safari check and the release block an announcement claim needs.
- Add a titled
DropdownMenuRadioGroupstate to the shared radio-group binding inventory (RadioGroup.a11y.states.ts).
[Automated review]
| {...rest} | ||
| role="group" | ||
| aria-label={label} | ||
| aria-label={hasLabel ? label : undefined} |
There was a problem hiding this comment.
With label now optional, <DropdownMenuRadioGroup aria-label="Sort by" …> type-checks, because BaseProps accepts aria-*. However, this line overwrites the spread aria-label with undefined, so the group is unnamed and the dev warning asks for a label or title. Suggest aria-label={hasLabel ? label : rest['aria-label']}, and counting rest['aria-label'] as a name in the warning condition above.
There was a problem hiding this comment.
Fixed in 6f2f825: label wins, otherwise a caller's aria-label names the group (over the title too), and either one counts as a name for the warning. New case "names the group by a caller's aria-label when label is omitted, title or not" fails at the previous head.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `DropdownMenuRadioGroup` (also exported as `ContextMenuRadioGroup`) takes a `title`: a visible heading above its radios, drawn like a `DropdownMenuGroup` heading, that names the group. With a title, `label` is optional. Give a menu section that holds one radio set this title instead of wrapping the set in a `DropdownMenuGroup`, so a screen reader announces one group with one name rather than two nested groups with the same name. Without `title` nothing changes. |
There was a problem hiding this comment.
This promises what a screen reader announces, but the tests prove the accessibility tree (one group named by its heading). Either phrase it as that exposure, or back the announcement with NVDA + Chrome and VoiceOver + Safari checks. The story comment above RadioGroupWithTitle makes the same promise.
There was a problem hiding this comment.
Reworded in 6f2f825, here and in the story comment: the menu exposes one group named by its heading. The PR description now says the same.
0b10c07 to
6f2f825
Compare
|
Rebased onto
|
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR18 and DEC-11 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so a maintainer (the DropdownMenu owner) needs to approve or reject that contract as the governing spec, including whether label or a caller's aria-label may override a visible title (with label="Ordering" and title="Sort by", the group is named "Ordering" under the heading "Sort by"). Whatever the decision, CI's test-node job fails because label is now optional: delete the DropdownMenuRadioGroup.label required_mismatch line (line 26) from packages/cli/test/core-prop-docs.known-drift.json. Once the owner rules on that precedence, add the matching state to the radio-group binding inventory (RadioGroup.a11y.states.ts), or record why it is omitted. The aria-label, wording and titled-state fixes from the last round are in.
[Automated review]
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR18 and DEC-11 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so the DropdownMenu owner needs to approve or reject that contract, including whether label or a caller's aria-label may override a visible title (with label="Ordering" and title="Sort by", the group is named "Ordering" under the heading "Sort by"). The drift-list fix from the last round is in, and CI is green. Whatever the owner decides, the shared radio-group binding inventory (RadioGroup.a11y.states.ts) still needs the naming state the ruling keeps, either bound or recorded with its reason in RADIO_GROUP_EXCLUSIONS.
[Automated review]
Five open pull requests propose rules in this record: facebook#7319 (FR18, DEC-11), facebook#7352 (FR19, DEC-12), facebook#7530 (FR20, AR7, DEC-13), facebook#7532 (FR21, DEC-14) and facebook#7541 (FR22, DEC-15). Each takes its own number, so none changes when another merges first. Record wording only.
Five open pull requests propose rules in this record: facebook#7319 (FR18, DEC-11), facebook#7352 (FR19, DEC-12), facebook#7530 (FR20, AR7, DEC-13), facebook#7532 (FR21, DEC-14) and facebook#7541 (FR22, DEC-15). Each takes its own number, so none changes when another merges first. Record wording only.
Five open pull requests propose rules in this record: facebook#7319 (FR18, DEC-11), facebook#7352 (FR19, DEC-12), facebook#7530 (FR20, AR7, DEC-13), facebook#7532 (FR21, DEC-14) and facebook#7541 (FR22, DEC-15). Each takes its own number, so none changes when another merges first. Record wording only.
Five open pull requests propose rules in this record: facebook#7319 (FR18, DEC-11), facebook#7352 (FR19, DEC-12), facebook#7530 (FR20, AR7, DEC-13), facebook#7532 (FR21, DEC-14) and facebook#7541 (FR22, DEC-15). Each takes its own number, so none changes when another merges first. Record wording only.
7a054e2 to
0c600e5
Compare
Five open pull requests propose rules in this record: facebook#7319 (FR18, DEC-11), facebook#7352 (FR19, DEC-12), facebook#7530 (FR20, AR7, DEC-13), facebook#7532 (FR21, DEC-14) and facebook#7541 (FR22, DEC-15). Each takes its own number, so none changes when another merges first. Record wording only.
Five open pull requests propose rules in this record: facebook#7319 (FR18, DEC-11), facebook#7352 (FR19, DEC-12), facebook#7530 (FR20, AR7, DEC-13), facebook#7532 (FR21, DEC-14) and facebook#7541 (FR22, DEC-15). Each takes its own number, so none changes when another merges first. Record wording only.
Five open pull requests propose rules in this record: facebook#7319 (FR18, DEC-11), facebook#7352 (FR19, DEC-12), facebook#7530 (FR20, AR7, DEC-13), facebook#7532 (FR21, DEC-14) and facebook#7541 (FR22, DEC-15). Each takes its own number, so none changes when another merges first. Record wording only.
…#7530) > Data items' `header` and `footer`, and the bottom sheet's level regions, follow once this merges. ## User need A sub-menu picking from a long list needs a filter field above its rows, often a "Create" button below. #7344 gave the root menu `header` and `footer`; a sub-menu has neither, so the field goes among the rows (DEC-8 forbids it) or the picker leaves the menu. ## Problem and solution fit `DropdownMenuSubMenu` takes `header` and `footer` under the root's rules (DEC-8, DEC-10). With either, the sub-menu's surface (the flyout, or the drilled-in view on a phone) is a modal dialog named by its row, holding the header, the sub-menu's `role="menu"` with rows only, and the footer. A flyout renders inside its parent menu, so `DropdownMenu` renders a host beside its menu for the dialog. A drilled-in view with regions replaces the root's menu and regions until popped, by a style rule that reads the host. ```tsx <DropdownMenuSubMenu label="Labels" header={<TextInput label="Filter labels" … />} footer={<Button label="Create label" … />}> {visibleLabelRows} </DropdownMenuSubMenu> ``` ## Current specification `component:DropdownMenu` (draft) gains a concept row, FR20, AR7 and DEC-13, all proposed and pending owner review. Other proposals in open pull requests: #7319 FR18/DEC-11, #7352 FR19/DEC-12, #7532 FR21/DEC-14, #7541 FR22/DEC-15. `module:DropdownMenu/useMenuPress` (current) gains a proposed ORD1a: a press released on a row of that dialog's menu acts, as FR1 requires for any flyout; a picker placed in its header or footer stays its own press root. ## Public delta and caller burden - `header` and `footer` on `DropdownMenuSubMenu`. - With either, the row says `aria-haspopup="dialog"`; Tab walks header, rows as one stop, footer, and wraps; an Escape nothing inside handled returns to the row; a pointer leaving the flyout keeps it open until focus leaves. Rendered beside the root's menu, such a flyout closes with the flyout holding its row. - Phone drill-in: a shown view follows its regions and caps at the root's `menuMaxHeight`. - `ContextMenu` renders no host: there the sub-menu shows its rows alone and warns in development; parity follows once this merges. - Without them, nothing changes. ## Design and alternatives Rejected: regions inside the flyout (inside the parent menu); a separate `DropdownMenu` per level; closing on pointer-leave, losing typed text. A drilled-in view's header sits above Back. ## Writing rubric results - R1 narration: none. - R2 research in a record: none. - R3 evidence naming a change: none. - R4 rejected alternatives over one line or outside a decision: none. - R5 pull request or issue references: none. - R6 ruled open questions listing options: none. ## Evidence See Testing. ## Scope - [x] One capability is added. - [x] Unrelated fixes, cleanup, and policy changes were removed or split. - [x] Consumer docs, focused tests, a story and a changeset ship with the feature. - [x] Public text and artifacts contain no internal Meta context. ## Testing Review round 8 (rebased onto `main` b74cf21): `pr-a11y` gets a "Run DropdownMenu sub-menu header and footer contract" step beside "Run DropdownMenu header and footer contract". It runs `SubMenuHeaderFooter.a11y.chromium.spec.ts` with `--project chromium`, so FR20's real-engine checks gate every head. That exact command passes locally against a Storybook built from this head (6 cases). 15 files, +1867/−121 (1,988 lines). DropdownMenu, ContextMenu, Layer, MoreMenu, RadioList, `useMenuPress` and doc-prop suites: 739 tests; header and footer Chromium specs: 16; `.github/scripts` workflow tests: 100; typecheck, `lint:strict`, `check:knowledge`, Storybook build: clean. Review round 7 (rebased onto `main` 863e167): `surfaceHost`, `surfaceOwnerId` and `menuMaxHeight` leave the exported `DropdownMenuContextValue` for `DropdownMenuSurfaceContext`, which the package does not export, layer-scoped and re-provided by each flyout as the menu context is; the public type matches `main`. A new `DropdownMenuContext.public.test.ts` pins its four members (it fails typecheck with the three back) and checks the internal context is not exported. Data-mode parity (data items' `header` and `footer`, `renderDropdownItems`, the bottom sheet's warning and their tests) moves to the follow-up, and the three pointer-leave cases share one setup: 14 files, +1856/−121 (1,977 lines). DropdownMenu, ContextMenu, Layer, MoreMenu, RadioList, `useMenuPress` and doc-prop suites: 739 tests; header and footer Chromium specs: 16; typecheck, `lint:strict` (no warnings), `check:knowledge`, Storybook build: clean. Rebased onto `main` d0f9afe, no conflicts or changes. The previous head's `pr-a11y` and `smoke-test` reds came from CI: main's #7802 added a `pr-a11y` step whose spec file the older head lacked, and jsdelivr was still propagating core@0.7.2. DropdownMenu, ContextMenu, RadioList, docProp* suites: 477 tests; typecheck, `check:knowledge`: clean. Review round 6: a flyout the pointer has left now decides that focus left once the move has completed, so an arrow key in a header radio group, where the browser moves focus itself, no longer closes it. A jsdom case replays the browser's order (focusout, a microtask checkpoint, focusin) and fails on the previous head; a Chromium case on the new "Submenu / header with a choice" story presses ArrowDown after the pointer leaves, and fails against a Storybook built from the previous head, where the flyout closes. DropdownMenu, ContextMenu, Layer, MoreMenu, RadioList, `useMenuPress` and doc-prop suites: 741 tests; header and footer Chromium specs: 16; typecheck, `lint:strict`, `check:knowledge`, Storybook build: clean. Review round 5 (rebased onto `main` 2c3ed99; the earlier `test` and `test-node` reds were `main`'s own, a PostCSS fixture that #7797 fixed): an Escape a control in a region already handled (a header menu closing, a field clearing itself) closes nothing more, and the new case fails on the previous head. The contract takes FR20, AR7 and DEC-13 so open proposals in this record stay distinct. DropdownMenu, ContextMenu, Layer, MoreMenu, `useMenuPress` and doc-prop suites: 646 tests; header and footer Chromium specs: 15; typecheck, `lint:strict`, `check:knowledge`, Storybook build: clean. Rebased onto `main` 577a62e after #7737 merged: one commit, 15 files, +1733/−109. DropdownMenu, ContextMenu, Layer, MoreMenu, `useMenuPress` and doc-prop suites: 645 tests; header and footer Chromium specs: 15; typecheck, `lint:strict`, `check:knowledge`, Storybook build: clean. Review round 4 (amended into the last commit): the surface host is owned through a ref, with only a key in state that changes when a host attaches (it moves when the root's header or footer comes or goes); a drilled-in view with regions caps at the root's `menuMaxHeight`. New cases: the view cap (fails on the previous head) and the host move (a guard). Gates below are for this head. Round 3, then stacked on #7737: the root steps aside for a view by a style rule over its surface host instead of a drill-in record kept in an Effect; a flyout with regions closes from the `onHide` of the flyout holding its row instead of an Effect; the bottom sheet's level regions moved to a follow-up. Each change has a case that fails with it reverted. - `DropdownMenuSubMenu.test.tsx` "header and footer" suite: 20 cases, plus one in `ContextMenu.test.tsx` (rows alone, with the warning). They cover the dialog's role, modality, name and row wiring; regions outside every menu; the Tab walk and wrap from a field, a row and a deeper flyout; field keys; Escape, an IME Escape and Escapes a region handled; a press from a menu row released on a dialog row (acts) or its field (outside); a menu placed in a region keeping its own press; pointer-leave with focus inside, focus moving within (a header radio's arrow included), then focus leaving; a nested flyout closing with its parent; the phone drill-in, the root stepping aside, a shown view following its regions and its height cap. - `__tests__/SubMenuHeaderFooter.a11y.chromium.spec.ts`, real Chromium: six cases (laptop structure, keyboard walk, picks, press-drag pick, a header radio after the pointer leaves; phone drill-in with the root stepped aside). - iOS 26.2 simulator (Mobile Safari), earlier head: phone drill-in as described above; on `main` the level has no field or button.
0c600e5 to
e60d696
Compare
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR22 and DEC-15 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so the DropdownMenu owner needs to approve or change that contract, including whether label, aria-label or aria-labelledby may name a group whose title is visible (with label="Ordering" and title="Sort by", the group is named "Ordering" under the heading "Sort by"). Whatever the owner decides, two fixes are needed:
- Make FR22's naming text match the code, since a group named only by
aria-labelis named and does not warn and a caller'saria-labelledbyalso replaces the title, and add the untitledaria-labeltest case the existing test name promises. - Once the owner rules, give the radio-group binding inventory (
RadioGroup.a11y.states.ts) the naming state the ruling keeps, with the heading passed as its visible label so the label-in-name check runs, or remove the code that allows it.
[Automated review]
| | FR16 | `DropdownMenuCheckboxItem` MUST expose one `menuitemcheckbox` row whose `aria-checked` equals its controlled value; enabled activation reports `!value`, stays open by default, and composes consumer click and pointer-move handlers with owned activation and hover focus. Disabled activation calls no consumer activation handler. | Released subcomponent API; `architecture:public-component-api/INV5–INV8`; `DropdownMenuSelectable.test.tsx` | Verified current behavior; consumer handlers are preserved | | ||
| | FR17 | Disabled menu rows SHOULD remain keyboard-discoverable while activation stays blocked. Current shared `useListFocus` deliberately skips every `aria-disabled` row, including checkbox items. | WAI-ARIA Menu pattern; current shared navigation source/tests | Objective mismatch; shared mechanism remains undecided in OQ1 | | ||
| | FR20 | With `header` or `footer` on a sub-menu (`DropdownMenuSubMenu`), its surface is a modal dialog named by its row (AR7), holding the header, the sub-menu's `role="menu"` (rows only), then the footer. It sits in a host `DropdownMenu` renders beside its menu, never inside a menu; under a root with no such host (`ContextMenu`) the sub-menu shows its rows alone. On a fine pointer the dialog is the flyout; on a compact touch display, the drilled-in view, which replaces the root's menu and regions until popped. A region keeps its own keys and presses. The rows are one Tab stop: Tab and Shift+Tab walk the header's controls, the rows, then the footer's, and wrap; from a deeper flyout, that flyout closes and the walk goes on from the rows. An open lands as for any sub-menu (FR11, FR12), not on a region. Escape in the dialog, outside an IME composition and unless a control there handled it, returns focus to the row and leaves the root open. A pointer leaving the flyout keeps it open until focus leaves, and it closes with the flyout holding its row. Without either prop nothing changes. | Proposed DEC-13; FR15; AR5 | Proposed; verified in jsdom and Chromium, pending owner review | | ||
| | FR22 | A `DropdownMenuRadioGroup` with `title` renders the title first inside its `role="group"`, as plain text carrying `dropdown-menu-section-heading` that roving focus and typeahead skip, and the group is named by it through `aria-labelledby`. `label` is then optional; `label`, or a caller's `aria-label`, names the group instead when given. A menu section that holds one radio set takes the set's `title` rather than a `DropdownMenuGroup` around it, so the set is one group with one name. Without `title` the group renders as before; with no `label`, `title` or `aria-labelledby` it is unnamed and warns in development. | Proposed DEC-15; FR14 | Proposed; verified in jsdom and Chromium, pending owner review | |
There was a problem hiding this comment.
FR22's closing clause leaves out aria-label: a group given only aria-label is named and does not warn (DropdownMenuRadioGroup.tsx line 148). Its overrides also leave out a caller's aria-labelledby, which replaces the title as well (line 157); Chromium then names the group by the referenced element. Please state the naming order the code follows, or the one the owner rules.
There was a problem hiding this comment.
FR22 now states the order the code follows, in e565c4b: the name is the first given of a caller's aria-labelledby, label, a caller's aria-label, and title, and the group is unnamed, with the development warning, only with none of the four. The caller's aria-label case now runs with and without a title, and a new case pins aria-labelledby over the title. The binding inventory (RadioGroup.a11y.states.ts) follows once the owner rules which of those may name a titled group.
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR22 and DEC-15 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so the DropdownMenu owner needs to approve or change that contract, including whether a caller's aria-labelledby, label or aria-label may name a group whose title is visible (with label="Ordering" and title="Sort by", the group is named "Ordering" under the heading "Sort by"). Whatever the owner decides, the radio-group binding inventory in RadioGroup.a11y.states.ts needs to cover the naming state the ruling keeps, with the heading passed as its visible label so the label-in-name check compares the right words, or the code that allows that state should go. FR22's naming text now matches the code, and the title-only state passes the shared radio-group checks in Chromium.
[Automated review]
e565c4b to
354ce7e
Compare
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR22 and DEC-15 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so the DropdownMenu owner needs to approve or change that contract as the governing spec, including dropping a label or aria-label given beside a visible title. The titled-and-labelled state is now bound in the radio-group inventory and passes the shared label-in-name check in Chromium, so nothing else is needed at this head. If FR22 is approved, please move the DropdownMenuGroupWithSelectable showcase block, which still wraps one radio set in a DropdownMenuGroup, to the set's title, and have FR22 say what a title that renders no text does.
[Automated review]
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR22 and DEC-15 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so the DropdownMenu owner needs to approve or change that contract as the governing spec, including the new rule that a title rendering no text counts as no title. Whatever that decision, one fix is needed: the heading's text is re-read only when the radio group itself re-renders, so a title component that changes its own text after the menu opens stays hidden, or leaves an empty heading over an unnamed group; observing the heading with a MutationObserver (or narrowing the rule to the values isRenderable rejects) and adding a case where the title updates itself would close it. If FR22 is approved, please also move the DropdownMenuGroupWithSelectable showcase block to the set's title.
[Automated review]
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR22 and DEC-15 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so the DropdownMenu owner needs to approve or change that contract as the governing spec, including the rule that a title rendering no text counts as no title. The fix that re-reads a title changing its own text is in, and nothing else blocks. Optionally, make the two development warnings match the state: a label equal to an element title's text still warns, and a given title that renders no text is told to add "a label or a title". If FR22 is approved, please also move the DropdownMenuGroupWithSelectable showcase block to the set's title.
[Automated review]
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR22 and DEC-15 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so the DropdownMenu owner needs to approve or change that contract as the governing spec, including the rules that a title rendering no text counts as no title and that a label beside a visible title is ignored. The two warning fixes from the last round are in, and nothing else blocks. If FR22 is approved, please also move the DropdownMenuGroupWithSelectable showcase block's title onto its radio set.
[Automated review]
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR22 and DEC-15 rules it adds to the draft component:DropdownMenu record are not yet approved spec, so the DropdownMenu owner, or another engineering owner on this exact head, needs to approve or change that contract as the governing spec. That decision includes the rules that a title rendering no text counts as no title (which also hides a title that is only an icon) and that a label beside a visible title is ignored. The SYNC-note fix from the last round is in, and nothing else blocks. If FR22 is approved, please also move the DropdownMenuGroupWithSelectable showcase block's title onto its radio set, and consider having FR22 say that title content stays non-interactive, as DEC-6 does for DropdownMenuGroup.
[Automated review]
…g is one named group DropdownMenuRadioGroup takes the title a DropdownMenuGroup takes: a visible heading, drawn and targeted like a group heading, that names the group. A visible title always names the group (a caller's aria-labelledby is read after it; label and aria-label are ignored, with a development warning when they differ from its text), so the name holds the text a speech-input user reads; a title that renders no text counts as no title. The shipped selectable-rows block titles its radio set itself.
…ropdownMenuRadioGroup Main now has the component's own record, component:DropdownMenuRadioGroup (draft). The title rules this change proposed as component:DropdownMenu FR22 and DEC-15, and its Radio set heading concept, move there as proposed FR7-FR9, DEC-1 and a Title concept, and the record's statements this change alters follow: FR1 and the Accessible name concept (a visible title names the group first; label becomes optional beside one), the ownership and compatibility notes, AR1 and AR3, a titled representative state, the title heading in the design relationships, a verification row, and PR1, which now names the title-text read (a layout read after each commit and one MutationObserver on the heading, only while a title is given). component:DropdownMenu is back to main's text.
3cb8b5e to
b1352e9
Compare
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR7–FR9 and DEC-1 rules this pull request adds to the draft component:DropdownMenuRadioGroup record are not yet approved spec, so the record's owner, or another engineering owner on this exact head, needs to approve or change that contract as the governing spec. That decision includes the rules that a title rendering no text counts as no title (which also hides a title that is only an icon) and that a label beside a visible title is ignored. Nothing else blocks: the shipped block now titles its radio set, and the focusable-title case is in. Optionally, have FR1 say again that a caller's aria-label names an untitled group when label is omitted, and update the radio group's SYNC note, which still describes that block as wrapping its radio set.
[Automated review]
…the SYNC note says the block titles its set FR1 had dropped what the code, the docs and the untitled aria-label case do: without a title, a caller's aria-label names the group when label is omitted. It says so again (the cell stays within its column). The header's SYNC note still described the shipped selectable-rows block as wrapping its radio set in a DropdownMenuGroup; this change makes the block title the set itself, and the note now says so. Record and comment only.
There was a problem hiding this comment.
Approval is on hold for a maintainer decision: the new title prop and the FR7–FR9 and DEC-1 rules this pull request adds to the draft component:DropdownMenuRadioGroup record are not yet approved spec, so the record's owner, or another engineering owner on this exact head, needs to approve or change that contract as the governing spec. That decision includes the rules that a title rendering no text counts as no title (which also hides a title that is only an icon) and that a label beside a visible title is ignored. The FR1 and SYNC-note follow-ups from the last round are in, and nothing else blocks. Optionally, carry DropdownMenuGroup's "keep the title non-interactive" sentence into the title docs, since the arrow keys cannot reach a control placed in the heading.
[Automated review]
| name: 'title', | ||
| type: 'ReactNode', | ||
| description: | ||
| 'A visible heading above the radios, with the same treatment as a DropdownMenuGroup heading, that names the group through aria-labelledby; an aria-labelledby passed through base props is read after it. With a title, label is optional. A title that renders no text (true, an empty fragment, a component that returns null) counts as no title: no heading shows and the group is named as without one. Give a menu section that holds one radio set this title instead of wrapping the set in a DropdownMenuGroup, so the set is one group with one name rather than two nested groups with the same name.', |
There was a problem hiding this comment.
Not blocking: DropdownMenuGroup.doc.mjs tells builders "Keep title content non-interactive and place controls in DropdownMenu header or footer", because a control in the heading is outside the arrow-key order, and proposed FR7 and DEC-1 say the same holds here. Adding that sentence to this description (and to the Chinese, dense and JSDoc copies) keeps builders from putting a control keyboard users cannot reach into a radio set's title.
User need
A menu often titles one section that holds a single radio set ("Sort by: Newest / Oldest"). Today the heading comes from wrapping the set in a
DropdownMenuGroup, andDropdownMenuRadioGrouprequires its ownlabel. That puts two nestedrole="group"elements with the same name in the accessibility tree around one radio set, and no public API avoids it: the outer group always names itself by its title, and the radio group's label is required.Problem and solution fit
DropdownMenuRadioGrouptakes thetitlethatDropdownMenuGroupalready takes. The title renders as the same section heading (same treatment, samedropdown-menu-section-headingtarget) as the group's first child, and names the radio group througharia-labelledby. The section becomes one group with one name, and nothing wraps it.Current specification
component:DropdownMenuRadioGroup(draft) gains a Title concept, FR7–FR9 and DEC-1, all proposed and pending owner review; its FR1, PR1, AR1 and ownership notes follow them. The heading reusescomponent:DropdownMenuFR14 and DEC-6. Other proposals in open pull requests: #7319 FR10 here andcomponent:DropdownMenuFR18/DEC-11, #7352 FR19/DEC-12, #7532 FR21/DEC-14; #7839 adds FR23, and with #7840 amends FR20.Public delta and caller burden
title?: ReactNodeonDropdownMenuRadioGroup(also exported asContextMenuRadioGroup).labelbecomes optional when atitleis given. The title then names the group, so the name holds the heading: a caller'saria-labelledbyis read after it, andlabeloraria-labelis ignored, with a development warning when they differ from its text. A title rendering no text counts as none. An unnamed group warns in development.title, the render is unchanged. Callers who wrapped a single set in aDropdownMenuGroupcan drop the wrapper and move its title onto the set.Design and alternatives
titlereuses the heading prop its sibling already ships. Rejected: aDropdownMenuGroupthat stops naming itself when its only child is a named set, which needs it to inspect its children; and anaria-hiddenheading beside the radio group'slabel, which lets the visible text and the name drift apart.Writing rubric results
Evidence
See Testing: red on
main, green here.Scope
Testing
Review round (16955a2): FR1 says again that, without a title, a caller's
aria-labelnames the group whenlabelis omitted, as the code, docs and untitled case do; the header's SYNC note says the shipped block titles its radio set. Record and comment only; typecheck and the title suite pass.Review round (b1352e9, rebased onto
mainda1020b as two commits):mainaddedcomponent:DropdownMenuRadioGroup([Component Audit] DropdownMenuRadioGroup: let builders reach the group root #7911), so the title rules move there fromcomponent:DropdownMenu(which is back tomain's text) as FR7–FR9 and DEC-1, and the record's FR1, PR1 (the title-text read), AR1, AR3 and ownership notes follow. [Component Audit] DropdownMenuRadioGroup: let builders reach the group root #7911'srefmerges besidetitle. Also from the 13:37Z review: FR7 and DEC-1 say a focusable node in the title is not reachable by the arrow keys, as DEC-6 does for a group heading, with a case; and the shipped "Selectable rows" block titles its radio set itself (changeset for@astryxdesign/cli). DropdownMenu, ContextMenu, Breadcrumbs, RadioList, CLI and doc-prop suites: 812 tests; radio group title, radio-group pattern and the DropdownMenuRadioGroup audit Chromium specs: 40; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (3cb8b5e): the header's SYNC note now names
blocks/components/DropdownMenuGroup/DropdownMenuGroupWithSelectable.tsx, the shipped block that wraps a single radio set in aDropdownMenuGroup, beside the menu's showcase directory thatcheck:syncrequires. Comment only;check:repo, typecheck and the title suite pass.Review round (2372e5d): the label-beside-title warning compared the label with the title node, so a node title with the same text still warned; it now compares with the heading's rendered text (whitespace collapsed). A title that renders no text gets its own unnamed-group warning ("The title renders no text, so it does not name the group…"); no title keeps the old one. Node-title cases with the same text, and the empty-fragment and null-component cases' new warning, fail with the previous component. DropdownMenu, ContextMenu, Breadcrumbs, RadioList and doc-prop suites: 544 tests; CLI node project: 260; radio group title and radio-group pattern Chromium specs: 31; typecheck,
lint:strict,check:knowledge, Storybook build: clean.Review round (a56da4a): whether the title's heading has text was read only after the radio group's own commits, so a title component that changes its text through its own state, a context or a store was never re-read. A
MutationObserveron the heading (childList,characterData,subtree), disconnected in the effect's cleanup, now reads it again on each change; the per-commit read stays, so the group's own renders settle before paint. A case where the title component updates itself, text arriving and then going away, fails when the observer does not re-read. DropdownMenu, ContextMenu, Breadcrumbs, RadioList and doc-prop suites: 541 tests; radio group title and radio-group pattern Chromium specs: 31; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (95f5cf3): a
titleoftrue, an empty fragment or a component that returnsnullstill counted as a title, leaving the group unnamed with no warning. A non-renderable title now counts as none, and a renderable one counts only while its heading renders text, read from the DOM after each commit before paint; a heading without text shows nothing and the group is named as without a title. Cases for each of the three titles fail with the previous component, a title that starts rendering text names the group (red when the heading is read once), and the hidden heading is asserted on its computed style. FR22, the docs (English and Chinese) and the Changeset say so. DropdownMenu, ContextMenu, Breadcrumbs, RadioList and doc-prop suites: 540 tests; radio group title and radio-group pattern Chromium specs: 31; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (354ce7e, rebased onto
mainbe42333): with alabelor a caller'saria-labelbeside atitle, the group was named by the invisible label, failing the radio-group pattern's required label-in-name rule. A visible title now always names the group (a caller'saria-labelledbyis read after it;labelandaria-labelare ignored, warning when they differ). The title suite's cases for a label, an aria-label and an aria-labelledby beside a title fail with the previous component, and the shared pattern binds a titled set given a label (menu-group-titled-labelled), which fails in Chromium with the previous component. FR22, DEC-15, the docs (English and Chinese) and the Changeset say so. DropdownMenu, ContextMenu, Breadcrumbs, RadioList and doc-prop suites: 536 tests; radio group title and radio-group pattern Chromium specs: 31; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round: FR22 states the naming order the code follows (the first given of a caller's
aria-labelledby,label, a caller'saria-label, andtitle; with none, unnamed and a development warning), and the changeset says the same. The caller'saria-labelcase now runs with and without a title, as its name promised, and a new case pinsaria-labelledbyover the title; both guard the code, which is unchanged. DropdownMenu, ContextMenu and doc-prop suites: 389 tests;RadioGroupTitleChromium spec: 1; typecheck,lint:strict,check:knowledge, Storybook build: clean.Rebased onto
maincb489ff after feat(DropdownMenuSubMenu): header and footer around a sub-menu's rows #7530 merged, as one commit: the record conflict keeps both rule sets (FR22/DEC-15 after FR20/DEC-13; the concepts sentence counts five).pr-a11ygets a "Run DropdownMenu radio group title contract" step forRadioGroupTitle.a11y.chromium.spec.ts, after main's new TopNav step. DropdownMenu, ContextMenu and doc-prop suites: 387 tests; CLI node project: 244;.github/scriptsworkflow tests: 100;RadioGroupTitleChromium spec: 1 (on b7860bc); typecheck,lint:strict,check:knowledge, Storybook build: clean.Rebased onto
main2c3ed99. The rule takes FR22 and DEC-15 so the five open proposals in this record stay distinct (record wording only). DropdownMenu suite: 283 tests; typecheck,check:knowledge, prettier: clean.Rebased onto main 22c592a (one commit). Main's record now has FR16–FR17 and DEC-10, so the rule stays FR18 and the decision is proposed DEC-11.
DropdownMenuSelectable.test.tsx, "DropdownMenuRadioGroup title" suite, 6 cases. A titled set is the menu's only group, named by its heading (aria-labelledby), with the section-heading target as its first child. The heading is skipped by arrows and typeahead. With a label too, the label names the group. A caller'saria-labelnames it whenlabelis omitted, with no warning. Without a title nothing changes. No name at all warns. Onmain4 fail; the two unchanged-path guards pass on both. Thearia-labelcase also fails at the previous head. All 6 pass here.Shared radio-group binding inventory (
RadioGroup.a11y.states.ts): newmenu-group-titledstate, a menu-radio group named by its visible title, with its story. Real Chromium (RadioGroup.a11y.chromium.spec.ts) fails it against amainbuild and passes here; the jsdom binding suite passes.__tests__/RadioGroupTitle.a11y.chromium.spec.tsagainst the "Radio group with a title" story, real Chromium. The accessibility tree shows one group "Hidden files" holding its heading and both radios. ArrowDown goes from the checkbox row to the first radio, and a pick works. It fails against amainbuild and passes here.DropdownMenu, ContextMenu, RadioList, BreadcrumbMenu and doc-prop suites (460 tests), plus every DropdownMenu and RadioList Chromium spec (87): green. Core typecheck,
lint:strict,check:knowledgeand the Storybook build: clean.