Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the tile form for menu radios; it can't be approved yet because FR24 and DEC-16 are new requirements in the draft component:DropdownMenu record, so an engineering owner needs to approve or amend them, including the no-target intentional: theming entry and ArrowLeft/ArrowRight keeping their sub-menu meanings inside a grid. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and as an engineering owner the author may do so. Whatever that decision, two fixes are needed:
- The cell's focus ring follows the browser's
:focus-visible, so after a mouse open, hovering a cell paints the keyboard ring, which in the default theme looks the same as the checked border; draw it only under keyboard modality, as the menu surfaces do (DropdownMenuRadioItem.tsx:261). children != nullturns afalseor''child into an empty cell that can still be focused and selected;isRenderable(children)avoids it (DropdownMenuRadioItem.tsx:238).
[Automated review]
66e41bb to
7ce5ad9
Compare
There was a problem hiding this comment.
Thanks for the tile form of DropdownMenuRadioItem; both earlier findings are fixed on this head, but it can't be approved yet because FR9, AR5 and DEC-1 are new requirements in the draft component:DropdownMenuRadioItem record, together with the intentional: no-target "Radio cell" entry in component:DropdownMenu. An engineering owner needs to approve or amend them (the cell with no radio mark named by its label, the reserved 2px accent border, the keyboard-only ring, and arrow keys in DOM order with ArrowLeft and ArrowRight keeping their sub-menu meanings) by commenting /approve-spec <full head SHA> on this pull request, and as an engineering owner the author may do so. Whatever that decision, one fix is needed:
isRenderable(children)is shallow, so children that render nothing through an array ({a && <A />}{b && <B />}with both false, or an empty.map()) still make an invisible cell that keyboard focus lands on and Enter selects; checkChildren.toArray(children).some(isRenderable)or narrow the documented rule (DropdownMenuRadioItem.tsx:276).
[Automated review]
|
|
||
| // Children that render make the option a cell; `false`, `''` and the like | ||
| // leave it a row. | ||
| if (isRenderable(children)) { |
There was a problem hiding this comment.
isRenderable is shallow, so children that render nothing through an array still make a cell: {a && <A />}{b && <B />} with both false, an empty .map() or [false] each render an 8px cell holding only the hidden label, which ArrowDown lands on and Enter selects. The docs and FR9 promise that children that render nothing leave the row. Children.toArray(children).some(isRenderable) drops empty nodes inside arrays; a fragment or a component returning null still can't be detected, so the docs should say so.
7ce5ad9 to
3db817d
Compare
There was a problem hiding this comment.
Thanks for the tile form of DropdownMenuRadioItem; it can't be approved yet because FR9, AR5 and DEC-1 are new requirements in the draft component:DropdownMenuRadioItem record, together with the intentional: no-target "Radio cell" entry in component:DropdownMenu, and no engineering owner has approved them on this revision. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and as an engineering owner the author may do so. Whatever that decision, two fixes are needed:
isRenderable(children)is shallow, so children that render nothing through an array ({a && <A />}{b && <B />}with both false, or an empty.map()) still make an invisible 8px cell that the arrow keys land on and Enter selects; checkChildren.toArray(children).some(isRenderable)and say in FR9 and the docs that a fragment or a component returningnullstill makes a cell (DropdownMenuRadioItem.tsx:276).- AR5 says focus a pointer moved there paints no ring, but after a key press the ring follows mouse-moved focus, as
architecture:interaction-modalityINV1 requires, so narrow AR5 to focus moved after pointer input, as DEC-1 already words it (DropdownMenuRadioItem.spec.md:192-194).
[Automated review]
| colors. The shared focus ring paints outside it only for focus the keyboard | ||
| moved there; focus a pointer moved there paints none, the browser's ring | ||
| included (`architecture:interaction-modality` INV1). |
There was a problem hiding this comment.
After a key press, hover moves focus with the last input, so a cell the mouse moves to keeps the ring (:focus-visible matches, outline solid in Chromium), as architecture:interaction-modality INV1 requires ("after keyboard input it does"). "Focus a pointer moved there paints none" is only true after a pointer press; DEC-1 already says "after a pointer open". Narrowing this sentence to focus moved after pointer input keeps AR5 consistent with INV1 and with the code.
3db817d to
97057c8
Compare
There was a problem hiding this comment.
Thanks for the tile form of DropdownMenuRadioItem; it can't be approved yet because FR9, AR5 and DEC-1 are new requirements in the draft component:DropdownMenuRadioItem record, together with the relaxed PR1, the row-scoped FR6, FR7 and AR1, and the intentional: "Radio cell" entry in component:DropdownMenu, and no engineering owner has approved them on this revision. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and as an engineering owner the author may do so. Whatever that decision, two fixes are needed:
isRenderable(children)is shallow, so children that render nothing through an array ({a && <A />}{b && <B />}with both false, or an empty.map()) still make an empty 8px cell that the arrow keys focus and Enter selects;Children.toArray(children).some(isRenderable)and a test for those shapes would match FR9 and the docs.- AR5 says focus a pointer moved there paints no ring, but after a key press the ring correctly shows on the cell the mouse moves to, as
architecture:interaction-modalityINV1 requires and the code does; AR5 and the component docblock should say "after pointer input", as DEC-1 already does.
[Automated review]
A one-of choice drawn as tiles in a menu (colour swatches, icons, numbered slots) had no Astryx form: DropdownMenuRadioItem always draws a full-width row with a radio mark, and its group context is not exported, so a caller's tile could not join the selection. Given children that render, the item now draws them as a cell instead of its row: one menuitemradio element with aria-checked, no radio mark and no row slots, named by its label through a visually hidden element it references with aria-labelledby, so the name stays the label when the tile shows text of its own. Children that render nothing (false, '') leave the row. The checked cell carries a 2px accent border that every cell reserves (transparent when unchecked; Highlight against Canvas under forced colors, which drop shadows). The shared focus ring paints outside it only for focus the keyboard moved there: focus the menu moves to the cell under the mouse after a pointer open paints none, not even the browser's ring, as the menu surfaces gate theirs on the last input. Cells keep the row's selection, close policy, handler order (caller onClick and onPointerMove run after the row's own), hover focus, press model, DOM-order arrow keys and typeahead; the caller lays them out with a grid on the group or on a wrapper inside it. icon, description and endContent are not shown in a cell and warn in development there. The cell is its own internal component, so the row keeps no state or Effect. ContextMenuRadioItem and BreadcrumbMenuRadioItem are the same component. Proposed FR9, AR5 and DEC-1 of component:DropdownMenuRadioItem record it, with a Presentation concept, a Cell state and design row, and the record's row-only facts (FR6, FR7, PR1, AR1) scoped to rows; component:DropdownMenu, which owns the target inventory, lists the Radio cell anatomy part with no target. A RadioCells story and a Chromium spec check the names in Chromium's accessibility tree, the accent and forced-colors paint, no ring for a cell hovered after a pointer open and the ring after ArrowDown, the arrow keys and Enter, and axe on the menu and on a sub-menu whose tiles sit in a wrapper grid; jsdom cases cover drawing, non-rendering children, selection, keys, typeahead, the keyboard-only ring, a wrapper, a disabled cell and the warning.
97057c8 to
582007e
Compare
There was a problem hiding this comment.
Thanks for the tile form of DropdownMenuRadioItem; it can't be approved yet because FR9, AR5 and DEC-1 are new requirements in the draft component:DropdownMenuRadioItem record, together with the relaxed PR1, the row-scoped FR6, FR7 and AR1, and the intentional: "Radio cell" entry in component:DropdownMenu, and no engineering owner has approved them on this revision. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and as an engineering owner the author may do so. Whatever that decision, these need fixing:
isRenderable(children)is shallow, so children that render nothing through an array ({a && <A />}{b && <B />}with both false, or an empty.map()) still make an empty 8px cell that the arrow keys focus and Enter selects;Children.toArray(children).some(isRenderable)with a test for those shapes, and docs that say a fragment or a component returningnullstill makes a cell, would match FR9.- AR5 says focus a pointer moved there paints no ring, but after a key press the ring correctly shows on the cell the mouse moves to, as
architecture:interaction-modalityINV1 requires and the code does; AR5, its verification row and the component docblock should say "after pointer input", as DEC-1 does. - Under forced colors the story's swatches disappear (each is a background colour and an unchecked cell's border is
Canvas), leaving only the checked and focused cells visible;forcedColorAdjust: 'none'on the swatch and a docs note for background-colour tiles would fix the example.
[Automated review]
| borderRadius: '50%', | ||
| }, | ||
| swatchColor: (color: string) => ({ | ||
| backgroundColor: color, |
There was a problem hiding this comment.
Under forced colors this background is replaced, and an unchecked cell's border is Canvas, so every unchecked swatch disappears: in Chromium with forced-colors emulation only the checked cell's Highlight border and the focused cell's ring are visible. forcedColorAdjust: 'none' here keeps the swatch colours, and a docs line could tell callers that a tile drawn as a background colour needs it.
There was a problem hiding this comment.
In 0e93926 the story's swatches take forcedColorAdjust: 'none', the children docs tell callers that a tile drawn as a background colour needs it, and the forced-colors Chromium case checks that an unchecked swatch keeps its colour (it reads white Canvas on the previous story).
…ve a row, AR5 says after pointer input, and the story's swatches keep their colour under forced colors Three points from the review on 582007e. isRenderable(children) was shallow, so an array that renders nothing ({a && <A />}{b && <B />} with both false, or an empty .map()) made an empty cell the arrow keys reached; a local rendersContent now counts an array only through an item that renders (React.Children is banned by no-react-introspection, so it walks the array itself), and an element always counts. The row case gains both array shapes (it fails on the previous head), a new case pins that a fragment or a component returning null is still a cell, and the docs (en, zh, dense) say so. AR5, its verification row, the docblock and the RadioCell comment now say focus after keyboard input paints the ring, also where the mouse then moves it, and focus after pointer input none; the keyboard-ring case adds that hover-moved step. The story's background-colour swatches take forcedColorAdjust: 'none', the children docs tell callers so, and the Chromium forced-colors case checks a swatch keeps its colour (white Canvas without it).
There was a problem hiding this comment.
Thanks for the tile form of DropdownMenuRadioItem; the three fixes from the last round hold on this revision and no code change is needed. Approval now waits only on the spec decision: FR9, AR5 and DEC-1 are new requirements in the draft component:DropdownMenuRadioItem record, together with the relaxed PR1, the row-scoped FR6, FR7 and AR1, and the intentional: "Radio cell" entry in component:DropdownMenu, and an engineering owner needs to approve or amend them. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and as an engineering owner the author may do so.
[Automated review]
User need
A menu often sets one choice from tiles: a colour from swatches, a slot from numbered cells. Each is a
menuitemradio, butDropdownMenuRadioItemalways draws a row with a radio mark and its group context is not exported, so apps rebuild the radio item beside Astryx's.Problem and solution fit
childrenonDropdownMenuRadioItem: given children that render, the item draws them as a cell instead of its row, with no radio mark, named bylabelthrough a visually hidden element;falseor''leaves the row.items.Current specification
component:DropdownMenuRadioItem(draft) gains a Presentation concept, FR9, AR5 and DEC-1 (pending owner review) and scopes FR6, FR7, PR1 and AR1 to rows;component:DropdownMenulists a Radio cell part with no target. #7319 also amends FR6. Overlap search ("radio cell", "tile", "swatch", "grid"): only #7517, forced-colors focus paint.Public delta and caller burden
children; omitted, nothing changes. The caller draws, names (label) and lays out cells.Design and alternatives
Evidence
DropdownMenuRadioCell.test.tsx: drawn, named, no mark, checked paint; non-rendering children; click, arrows, typeahead; keyboard-only ring; disabled; the warning.RadioCell.a11y.chromium.spec.ts: Chromium's names and checked state; the accent border; no ring on a cell hovered after a pointer open, a ring after ArrowDown; forced colors; Enter; axe, with a wrapper grid.Writing rubric results
R1 narration: FR9, AR5, DEC-1 and the new rows are present tense.
R2 research: none.
R3 evidence: FR9 cites DEC-1; its verification row names the tests.
R4 rejected alternatives: two
Rejected:lines in DEC-1.R5 change references: none.
R6 open questions: none.
Scope
Testing
main0aec0ee merged in, plus one commit): children that are an array of nothing ({a && <A />}{b && <B />}both false, an empty.map()) leave a row, through a local array walk (React.Childrenis banned byno-react-introspection); an element, even a fragment or a component returningnull, is still a cell, as the docs now say. AR5 and its docs say "after pointer input"; the story's swatches takeforcedColorAdjust: 'none', and the forced-colors Chromium case checks one keeps its colour. The row case fails on the previous head, the swatch case on the previous story. DropdownMenu and CLI suites: 651 tests; the 26 Chromium specs CI selects (153 cases); native types, typecheck,lint:strict,check:knowledgeand the Storybook build pass.main71601aa (582007e):main's ci: discover PR browser accessibility specs, so a new spec needs no workflow edit #8367 discovers pull-request browser specs, so this PR'sci.ymlstep is gone andRadioCell.a11y.chromium.spec.tsruns because the PR changes it. DropdownMenu and CLI suites: 650 tests; the 26 Chromium specs CI selects (153 cases); native types, typecheck,lint:strict,check:knowledgeand the Storybook build pass.Rebased onto
main10ca8ea (97057c8), patch unchanged: on 3db817d thebuild-storybookjob built and then failed to upload its artifact (ECONNRESET), andbuild,pr-rtlandpr-rtl (core)failed for want of it. The suites and Chromium specs below pass on this head again.Rebased onto
main7c45ff3 (3db817d), patch unchanged: the pr-a11y step sits besidemain's menu hover focus step (#8221). DropdownMenu, ContextMenu, Breadcrumbs, MoreMenu and cli suites: 788 tests; RadioCell, radio item audit and menu hover focus Chromium specs: 37; typecheck,eslint(strict),check:knowledgeand the Storybook build pass.Review round (rebased onto
main9effa5c):mainaddedcomponent:DropdownMenuRadioItem(#8191), so the cell's rules move there fromcomponent:DropdownMenu(FR24 and DEC-16 become FR9, AR5 and DEC-1), and the cell composes calleronClickandonPointerMoveafter its own, as #8191's rows do. A cell needs children that render (isRenderable), so{show && <Swatch />}withshowfalse stays a row. The ring now marks only focus the keyboard moved there, as the menu surfaces gate theirs, and the cell suppresses the browser's own ring; the cell is an internal component, so rows keep no state or Effect. With the previousDropdownMenuRadioItem.tsx, the non-rendering and keyboard-ring cases fail, and Chromium's hover case reads a solid outline.