Repository navigation
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:design-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This change needs a maintainer decision before it can be approved: the new indicator prop on DropdownMenuRadioGroup adds public API and a new visible behavior, and no current spec covers it yet, because the DropdownMenu record is still a draft and the proposed concept row, FR15 and DEC-8 cannot settle it on their own. A maintainer needs to provide or identify the governing spec, deciding whether the prop is admitted, where the check sits, how the check glyph maps to theme targets, and whether ContextMenuRadioGroup, which re-exports this component, carries it too. Two small fixes apply either way: the FR15 evidence cell says the CheckIndicator record names menu rows as a host, but that record only describes a generic host, and the changeset and description should mention that ContextMenuRadioGroup gains the prop.
[Automated review]
| | FR12 | In the pointer menu ArrowDown on the last enabled row wraps to the first and ArrowUp on the first to the last; PageDown and PageUp move to the last and first fully visible enabled row and, pressed there again, one viewport further without wrapping; ArrowUp on the trigger opens with the last enabled row highlighted; the key that opened the menu and its auto-repeats do not activate; typeahead matches the row's label element alone and ignores Control/Command chords and input-method composition. | Proposed in this change; `DropdownMenu.test.tsx` keyboard suite, `useListFocus.test.tsx`, `useTypeahead.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR13 | On a mouse, a nested flyout stays open while the pointer is inside the triangle from where it left its row to the flyout's near edge — including while the pointer is paused there — and closes after the existing delay once the pointer has left both the row and that triangle. The row's click toggle and its guard window are unchanged. | Proposed in this change; `DropdownMenuSubMenu.test.tsx` safe-triangle suite, `useMenuHover.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR14 | A compound `DropdownMenuGroup` is one `role="group"` named by its heading through `aria-labelledby`; the heading carries `dropdown-menu-section-heading`, is plain text rather than a menu item, and is skipped by roving focus and typeahead. An untitled group is an unnamed `role="group"`. | DEC-1, docs, and tests | Proposed; awaiting owner approval | | ||
| | FR15 | A `DropdownMenuRadioGroup` with `indicator="check"` MUST draw the theme's `check` indicator, carrying `dropdown-menu-radio`, at the inline end of its chosen row and no mark on its other rows; every row keeps `role="menuitemradio"` and `aria-checked`. The default `radio` draws the radio circle on every row. | Proposed DEC-8; `component:CheckIndicator` names menu rows as a host; Selector's `indicatorPosition` defaults to `end` | Proposed; verified in jsdom and Chromium, pending owner review | |
There was a problem hiding this comment.
The CheckIndicator record doesn't name menu rows as a host: it describes a generic host and lists "Row placement and logical-edge layout" as owned by the host component. Menu rows are named in the CheckIndicator.tsx header comment and the Indicator docs, not in the record. Consider dropping this citation or restating what the record says.
There was a problem hiding this comment.
Fixed in b624158. The citation was wrong: the CheckIndicator record names no particular host. FR15's basis and the PR description now say what the record does say, that row placement and logical-edge layout belong to the host component. That makes the inline-end placement this record's own decision, recorded in DEC-8 and left for the owner to settle.
| * stay `menuitemradio` with `aria-checked` either way. | ||
| * @default 'radio' | ||
| */ | ||
| indicator?: DropdownMenuRadioIndicator; |
There was a problem hiding this comment.
ContextMenuRadioGroup re-exports this component, so ContextMenu callers get indicator too. The description and changeset only name DropdownMenuRadioGroup; stating the ContextMenu exposure lets the maintainer decision cover both names.
There was a problem hiding this comment.
Done in b624158. The prop's JSDoc, the component doc and the changeset now say that ContextMenuRadioGroup is this component under another name and takes indicator too. The PR description's public delta names it as well, so the decision covers both names.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression46 of 494 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.
This change still needs a maintainer decision before it can be approved: the DropdownMenu record is a draft, so a maintainer must provide or identify the governing spec for the new indicator prop, settling whether it is admitted, the inline-end placement, how the check glyph maps to theme targets, and what unchosen rows show when a theme replaces the check indicator. The earlier citation and ContextMenu fixes are in, and two small fixes apply either way:
BreadcrumbMenuRadioGroupre-exports the same component, so the changeset, JSDoc and docs should name it next toContextMenuRadioGroup.- FR15 and the docs say unchosen rows draw nothing, which holds only for the default check indicator: a theme that swaps
checkfor one that draws an unchecked state, such asRadioIndicator, shows a mark on every row.
[Automated review]
| * stay `menuitemradio` with `aria-checked` either way. `ContextMenuRadioGroup` | ||
| * is this component under another name, so it takes the prop too. |
There was a problem hiding this comment.
BreadcrumbMenuRadioGroup re-exports this component too (Breadcrumbs/index.ts), so Breadcrumbs menus also get indicator. Naming it next to ContextMenuRadioGroup here, in the changeset, and in the en and zh docs lets the maintainer decision cover every public name.
There was a problem hiding this comment.
Done in ab9cfab. BreadcrumbMenuRadioGroup is now named next to ContextMenuRadioGroup in the prop JSDoc, the changeset, the en, zh and dense docs, FR15 and the PR description, so the decision covers all three public names.
| | FR12 | In the pointer menu ArrowDown on the last enabled row wraps to the first and ArrowUp on the first to the last; PageDown and PageUp move to the last and first fully visible enabled row and, pressed there again, one viewport further without wrapping; ArrowUp on the trigger opens with the last enabled row highlighted; the key that opened the menu and its auto-repeats do not activate; typeahead matches the row's label element alone and ignores Control/Command chords and input-method composition. | Proposed in this change; `DropdownMenu.test.tsx` keyboard suite, `useListFocus.test.tsx`, `useTypeahead.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR13 | On a mouse, a nested flyout stays open while the pointer is inside the triangle from where it left its row to the flyout's near edge — including while the pointer is paused there — and closes after the existing delay once the pointer has left both the row and that triangle. The row's click toggle and its guard window are unchanged. | Proposed in this change; `DropdownMenuSubMenu.test.tsx` safe-triangle suite, `useMenuHover.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR14 | A compound `DropdownMenuGroup` is one `role="group"` named by its heading through `aria-labelledby`; the heading carries `dropdown-menu-section-heading`, is plain text rather than a menu item, and is skipped by roving focus and typeahead. An untitled group is an unnamed `role="group"`. | DEC-1, docs, and tests | Proposed; awaiting owner approval | | ||
| | FR15 | A `DropdownMenuRadioGroup` with `indicator="check"` MUST draw the theme's `check` indicator, carrying `dropdown-menu-radio`, at the inline end of its chosen row and no mark on its other rows; every row keeps `role="menuitemradio"` and `aria-checked`. The default `radio` draws the radio circle on every row. | Proposed DEC-8; `component:CheckIndicator` leaves row placement to its host; Selector's `indicatorPosition` defaults to `end` | Proposed; verified in jsdom and Chromium, pending owner review | |
There was a problem hiding this comment.
"No mark on its other rows" holds only for the default CheckIndicator, which renders nothing when unchecked. The row renders the theme's check indicator on every row, so a theme that swaps in one that draws an unchecked state (the documented RadioIndicator swap) shows it at the inline end of every row, as Selector's AV1 allows for Selector. Once that behavior is settled, this row and the docs should say what actually renders.
There was a problem hiding this comment.
Fixed in ab9cfab. FR15, DEC-8, the concept row, the docs, the story and the changeset now say what renders. Every row renders the theme's check indicator at its inline end, in that row's state. The default check draws on the chosen row only. A theme whose check draws an unchecked state, like the documented RadioIndicator swap, shows it on every row, as Selector's AV1 allows. A new unit case renders the group under a theme with indicators: {check: RadioIndicator} and checks that every row draws a radio carrying the menu target and only the chosen one is filled.
There was a problem hiding this comment.
This change needs a maintainer decision before it can be approved: the DropdownMenu record is still a draft, so a maintainer must provide or identify the governing spec for the new indicator prop on DropdownMenuRadioGroup, which ContextMenuRadioGroup and BreadcrumbMenuRadioGroup also expose. That decision needs to settle whether the prop is admitted, the inline-end placement on every pointer type, how the check glyph maps to theme targets, and whether unchosen rows show a replacement check indicator's unchecked state. The earlier disclosure and wording fixes are in and the implementation matches its description, including an unchanged default radio rendering, so nothing else is needed from you until that decision lands.
[Automated review]
|
Decision (ENGOWNER), settling the four open questions: |
There was a problem hiding this comment.
This change still needs a maintainer to record its governing spec before it can be approved. The ENGOWNER decision posted on this PR states the intended direction for the new indicator prop, but DropdownMenu.spec.md is still a draft, so FR15, the Radio mark concept and DEC-8 are not yet current authority. A maintainer needs to commit that decision as current in the owning record, or point to an existing current record that covers it; the record's "Radio indicator" anatomy row and FR2 still describe only the radio circle and should then also describe the check glyph that carries dropdown-menu-radio. The implementation matches the PR description and the default radio rendering is unchanged, so no code change is needed while that decision is recorded.
[Automated review]
| | FR12 | In the pointer menu ArrowDown on the last enabled row wraps to the first and ArrowUp on the first to the last; PageDown and PageUp move to the last and first fully visible enabled row and, pressed there again, one viewport further without wrapping; ArrowUp on the trigger opens with the last enabled row highlighted; the key that opened the menu and its auto-repeats do not activate; typeahead matches the row's label element alone and ignores Control/Command chords and input-method composition. | Proposed in this change; `DropdownMenu.test.tsx` keyboard suite, `useListFocus.test.tsx`, `useTypeahead.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR13 | On a mouse, a nested flyout stays open while the pointer is inside the triangle from where it left its row to the flyout's near edge — including while the pointer is paused there — and closes after the existing delay once the pointer has left both the row and that triangle. The row's click toggle and its guard window are unchanged. | Proposed in this change; `DropdownMenuSubMenu.test.tsx` safe-triangle suite, `useMenuHover.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR14 | A compound `DropdownMenuGroup` is one `role="group"` named by its heading through `aria-labelledby`; the heading carries `dropdown-menu-section-heading`, is plain text rather than a menu item, and is skipped by roving focus and typeahead. An untitled group is an unnamed `role="group"`. | DEC-1, docs, and tests | Proposed; awaiting owner approval | | ||
| | FR15 | A `DropdownMenuRadioGroup` with `indicator="check"` MUST render the theme's `check` indicator, carrying `dropdown-menu-radio`, at the inline end of every row, in that row's checked or unchecked state; the default `check` draws only on the chosen row, and a theme whose `check` draws an unchecked state (a radio) shows it on every row, as Selector's AV1 allows. Every row keeps `role="menuitemradio"` and `aria-checked`. The default `radio` draws the radio circle on every row. `ContextMenuRadioGroup` and `BreadcrumbMenuRadioGroup` are the same component. | DEC-8; `component:CheckIndicator` leaves row placement to its host; Selector's `indicatorPosition` defaults to `end` | Accepted; verified in jsdom and Chromium | |
There was a problem hiding this comment.
FR15 now reads "Accepted", but this record is still authority: draft with approved_by: null, so FR15 is not current authority yet. Once the decision is committed as current, FR2 ("every target remains on its current painted element") and the "Radio indicator" anatomy row should also describe the check glyph that carries dropdown-menu-radio in check mode.
There was a problem hiding this comment.
Updated FR2 and the Radio indicator anatomy row in a17e4d9: with indicator="check" the dropdown-menu-radio target sits on the check glyph, and both rows now say so and point at FR15. On authority, FR15's "Accepted" records the engineering-owner decision posted on this PR, the same way the draft ContextMenu record marks its accepted FR6. Promoting the whole DropdownMenu record to current would also turn its other proposed rows into authority. That is the record owner's step, outside this PR's one intent, so this PR leaves the record draft.
There was a problem hiding this comment.
This change still needs a maintainer to record its governing spec before it can be approved. The ENGOWNER decision is now written into DropdownMenu.spec.md, but that record is still a draft, so the Radio mark concept, FR15 and DEC-8 are not yet current authority. A maintainer needs to make that decision current in the owning record, which also means settling the record's other proposed rows, or point to an existing current record that covers the indicator prop; at that point, the design-relationships sentence saying the radio element carries Indicator's shared radio target should also describe the check glyph. The implementation matches its description and the default radio menu renders unchanged, so you don't need to change any code in the meantime.
[Automated review]
| | Caller-rendered item start content | Presents arbitrary React content directly in an action-row start slot. | Caller-supplied content | Context-dependent | FR1, FR3 | | ||
| | Checkbox indicator | Draws the decorative checkbox state for a checkbox action row. | `component:Indicator` | Supporting | FR1, FR3 | | ||
| | Radio indicator | Draws shared radio chrome with the current menu-specific radio target. | Current source and Indicator | Supporting | FR1, FR2, FR3 | | ||
| | Radio indicator | Radio chrome, or with `indicator="check"` the check glyph; menu radio target. | Current source and Indicator | Supporting | FR1–FR3, FR15 | |
There was a problem hiding this comment.
In check mode this part is the glyph CheckIndicator renders through Icon, so it also carries astryx-icon. The sentence under this table still says "The radio element also carries Indicator's shared radio target". When the decision is made current, that sentence should describe the check glyph too.
There was a problem hiding this comment.
Done in 0a9a14c. The note under the anatomy table now says that in a radio group with indicator="check" the part is the check glyph, which carries Icon's astryx-icon rather than Indicator's shared radio target. Making the record current is still the owner's step, outside this PR.
There was a problem hiding this comment.
This change still needs a maintainer to make its governing spec current before it can be approved: the ENGOWNER decision is written into DropdownMenu.spec.md (the Radio mark concept, FR15, DEC-8 and the FR2 and anatomy amendments), but that record is still a draft, so the new indicator prop on DropdownMenuRadioGroup, ContextMenuRadioGroup and BreadcrumbMenuRadioGroup has no current authority yet. A maintainer needs to commit that decision as current in the owning record, which for this record also means settling its other proposed rows, or point to an existing current record that covers the prop. The implementation matches its description and the default radio rendering is unchanged; the one small follow-up is the "Radio indicator" anatomy entry in DropdownMenu.doc.mjs, which should also describe the check glyph that carries dropdown-menu-radio in check mode.
[Automated review]
0a9a14c to
c123b0a
Compare
There was a problem hiding this comment.
Approval of this change is waiting on a maintainer rather than on code changes: the new indicator prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) still has no current governing spec, because the engineering-owner decision is recorded only in the DropdownMenu record, which stays a draft. A maintainer needs to commit that decision as current (the Radio mark concept, FR15, the FR2 and anatomy amendments, and DEC-8) or point to an existing current record that already covers the prop. The implementation matches its description and leaves the default radio rendering unchanged. Once the decision is current, the "Radio indicator" anatomy entry in DropdownMenu.doc.mjs should also describe the check glyph that carries dropdown-menu-radio in check mode.
[Automated review]
There was a problem hiding this comment.
Owner ruling, cixzhang: this doesn't match Selector's existing prop, so the spec needs updating before it can be approved.
A single-choice menu and Selector both mark one chosen option, but they would describe that mark differently:
- Selector takes
indicatorPosition?: 'start' | 'end'. It chooses where the selection mark sits. The mark itself is always the theme'scheckindicator, which a theme can replace; no prop picks it. - This PR adds
indicator?: 'radio' | 'check'to the radio group. It chooses which mark is drawn and fixes where it goes (radio at the start, check at the end), under a different name and on a different axis.
A product picking between a Selector and a radio-group menu would meet two vocabularies for the same thing.
What the spec update should settle, in component:DropdownMenu, replacing the proposed Radio mark concept, FR15 and DEC-8:
- If the need is the mark's position, use Selector's
indicatorPosition: 'start' | 'end'with the same meaning. - If the need is a check mark instead of a radio circle, say why the theme's
radioindicator replacement can't express it. Selector's mark comes from the theme, not from a prop.
Code changes can follow once the spec settles that.
c123b0a to
dbc86bb
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.
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.
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.
bf4c3ed to
dc76473
Compare
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but the DropdownMenu spec that would govern it is still a draft, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether unset stays pointer-dependent instead of Selector's end, whether a row whose mark draws nothing reserves no space (with the documented check recipe at start, only the chosen row's label is indented), and whether checkbox rows keep today's placement, which puts checkbox and radio marks on opposite edges of a mixed menu. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, and menus without the prop render as before; the pr-a11y failure comes from a newer step on main and clears with a rebase.
[Automated review]
dc76473 to
9a0d1b1
Compare
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but the DropdownMenu spec that would govern it is still a draft, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether unset stays pointer-dependent instead of Selector's end, whether a row whose mark draws nothing reserves no space (with the documented check recipe at start, only the chosen row's label is indented), and whether checkbox rows keep today's placement, which puts checkbox and radio marks on opposite edges of a mixed menu. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, menus without the prop render as before, and CI is green on this head, so nothing else is needed from you until that decision lands.
[Automated review]
9a0d1b1 to
5181913
Compare
|
Two older review threads, addressed earlier on this branch (now 1acf14e after a rebase):
Resolving both. The two non-blocking points are answered in their threads (5181913). |
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but the DropdownMenu spec that would govern it is still a draft, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether an unset group keeps its pointer-dependent edge instead of Selector's end, accept or reject FR18's new rule that a mark that draws nothing takes no room (so with the check recipe at start only the chosen row's label is indented), and say whether checkbox rows keep today's placement. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, the new touch case closes the earlier test gap, menus without the prop render as before, and CI is green, so nothing else is needed from you until that decision lands.
[Automated review]
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but the DropdownMenu spec that would govern it is still a draft, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether an unset group keeps its pointer-dependent edge instead of Selector's end, whether FR18's rule that a mark that draws nothing takes no room stands (with the check recipe at start, only the chosen row's label is indented), and whether checkbox rows keep today's placement. The Mark position row now reads proposed, and in Chromium the change does what it describes under a mouse, under touch and in right-to-left while menus without the prop render as before, so nothing else is needed from you until that decision lands.
[Automated review]
…, as Selector's does
`DropdownMenuRadioGroup` takes `indicatorPosition` (`'start' | 'end'`),
Selector's prop and `IndicatorPosition` type with the same meaning: the
logical edge of each row that carries the radio mark, on every pointer.
Unset, the mark keeps today's placement, at the start under a fine pointer
and at the end under a coarse one. The mark stays the theme's `radio`
indicator; a menu that wants a check instead is wrapped in a nested Theme
that extends the product theme with `indicators: {radio: CheckIndicator}`.
ContextMenuRadioGroup and BreadcrumbMenuRadioGroup, the same component under
other names, take the prop too.
The draft DropdownMenu record proposes the Mark position concept, FR18 and
DEC-11 for the owner. pr-a11y runs the radio mark position Chromium contract.
Rebased onto main after facebook#7530 as one commit; review rounds are folded in
unchanged.
…er a coarse pointer, and FR18 states Selector's FR3 The Chromium start case ran under a fine pointer, where start and an unset group place the mark alike, so it passed with the start pin dropped. A case in a touch-emulating context (pointer: coarse) checks that an unset group puts the mark after the label and that start keeps every mark before it; with the start pin dropped it fails. FR18 said 'with Selector's meaning' without stating Selector's FR3: it now says that a mark that draws nothing (an unchosen row when the theme's radio is CheckIndicator) takes no room, so at start only the chosen row's label is indented, which the nested-theme case shows (the check is drawn on the chosen row only). Its verification row names both.
…s decision is approved The concept row said its stability was stable while DEC-11, which decides it, is still proposed; the record's other proposed concept, Sub-menu regions, says proposed. Record wording only; check:knowledge and prettier pass.
…the indicatorPosition it passes to its rows Main now has the component's own record, component:DropdownMenuRadioGroup (draft). This change adds indicatorPosition to the context the group gives its radio items, which that record lists as value, onChange and hasCloseOnSelect only. The record now names it: proposed FR10 (the group passes the edge to every item, which places its mark, per component:DropdownMenu FR18), a Mark position concept, the ownership and compatibility notes, the edge in the item's indicator paint, a verification row and the Chromium spec in verified_by. The rule for where the mark sits stays component:DropdownMenu FR18 and DEC-11, which own the radio item.
f4ed94a to
47f04b2
Compare
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but both records that would govern it are still drafts, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 in component:DropdownMenu and FR10 in component:DropdownMenuRadioGroup as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether an unset group keeps its pointer-dependent edge instead of Selector's end, whether FR18's rule that a mark that draws nothing takes no room stands (with the check recipe at start, only the chosen row's label is indented), and whether checkbox rows keep today's placement. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, menus without the prop render as before, and nothing else is needed from you until that decision lands.
[Automated review]
This branch was successfully deployed
2 participants
User need
A menu that sets one option, such as rows shown, should mark its choice as Selector does: Selector's check at Selector's edge.
DropdownMenuRadioItemputs its mark at the row's start under a fine pointer, and no prop chooses the edge, so callers fall back to aDropdownMenuItemwhose role they write through a ref.Problem and solution fit
DropdownMenuRadioGrouptakesindicatorPosition: 'start' | 'end', Selector's prop and type with the same meaning, on every pointer. Unset keeps today's placement (start under a fine pointer, end under a coarse one).radioindicator, as Selector's is the theme'scheck. For the check in one menu, a nestedThemeextends the product theme withindicators: {radio: CheckIndicator}; other radios keep the circle.Current specification
component:DropdownMenuis a draft. The proposed Mark position concept, FR18 and DEC-11 record the shape for the record owner;component:DropdownMenuRadioGroup(draft) names the edge, as proposed FR10. Other proposals in open pull requests: #7352 FR19/DEC-12, #7532 FR21/DEC-14, #7541 FR7–FR9/DEC-1 incomponent:DropdownMenuRadioGroup; #7839 adds FR23, and with #7840 amends FR20.component:SelectordefinesindicatorPosition; this reuses itsIndicatorPositiontype.Public delta and caller burden
DropdownMenuRadioGroup, also exported asContextMenuRadioGroupandBreadcrumbMenuRadioGroup. Nothing changes without it.Design and alternatives
end.radio, and a caller-setroleonDropdownMenuItem.Evidence
menuitemradiowith the radio mark and the group consumes the prop (onmainit leaks to the DOM). A nested theme mappingradioto the check marks one menu's chosen row with the check while another menu keeps its radios (passes on both).endputs every mark after its label against the row's end with labels aligned,startkeeps it before the label, and a keyboard choice moves the checked state. Theendcase fails against a build withmain's radio item.Writing rubric results
R1 narration: the concept, FR18, FR10 and DEC-11 are present tense.
R2 research: none.
R3 evidence: FR18's verification row names the tests.
R4 rejected alternatives: DEC-11 lists three.
R5 change references: none.
R6 open questions: none.
Scope
Testing
Review round (47f04b2, rebased onto
mainda1020b):mainaddedcomponent:DropdownMenuRadioGroup([Component Audit] DropdownMenuRadioGroup: let builders reach the group root #7911), whose ownership lists the group's context as value,onChangeandhasCloseOnSelect; it now also namesindicatorPosition(proposed FR10, a Mark position concept, the edge in the item's indicator paint, a verification row), while the mark's placement stayscomponent:DropdownMenuFR18 and DEC-11. [Component Audit] DropdownMenuRadioGroup: let builders reach the group root #7911'srefmerges besideindicatorPosition. DropdownMenu, ContextMenu, Breadcrumbs, Selector and doc-prop suites: 693 tests; radio mark position and DropdownMenuRadioGroup audit Chromium specs: 13; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (f4ed94a): the Mark position concept row reads
proposed, as DEC-11 is, matching the record's other proposed concept. Record wording only;check:knowledgepasses.Review round (5181913, rebased onto
mainbe42333): the Chromium spec gains a case in a touch-emulating context, where an unset group puts the mark after the label andstartkeeps every mark before it; it fails with thestartpin dropped, which the fine-pointer case did not catch. FR18 now states Selector's FR3 (a mark that draws nothing takes no room, so withCheckIndicatoronly the chosen row's label is indented atstart), and its verification row names both pointers. DropdownMenu, ContextMenu, Breadcrumbs, Selector and doc-prop suites: 691 tests; radio mark position Chromium spec: 4; typecheck,lint:strict,check:knowledge, Storybook build: clean.