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.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval of this fix waits on an engineering owner's decision on the three draft requirements it adds (DropdownMenu FR22, ContextMenu FR11, DropdownMenuSubMenu FR10), because no current record states this Tab rule yet. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. The DropdownMenu and ContextMenu change works as described and its new tests fail without the fix, but one change is needed either way:
BreadcrumbItemmenus render the same sub-menu, but their root still ignores keys from a flyout (BreadcrumbItem.tsx:582), so Tab on a breadcrumb sub-menu row now closes only the flyout and leaves focus on the page body with the menu open; giving that root the same Tab branch, with a test, would match BreadcrumbItem FR5 and FR6.
[Automated review]
| // In a menu with a header or footer above this level, that level | ||
| // moves focus on; in a plain menu the root closes the whole menu, | ||
| // and focus moves on from its trigger (APG menu button). | ||
| close({focusTrigger: false}); |
There was a problem hiding this comment.
This close now runs under every root that hosts DropdownMenuSubMenu, not only DropdownMenu and ContextMenu. BreadcrumbItem renders the same sub-menu (a nested items entry, or BreadcrumbMenuSubMenu), but its root still returns early for keys from a flyout (BreadcrumbItem.tsx:582). So Tab on a breadcrumb sub-menu row closes only the flyout and leaves focus on the page body with the breadcrumb menu open. Adding the same if (e.key === 'Tab') closeMenu(); branch to that root's non-owned path (BreadcrumbItem FR5 already says Tab closes its menu), with a test, would keep the three roots consistent.
github-actions
Bot
added
the
needs:design-review
|
/approve-spec 153d2d9 |
|
I'll run a full review to re-check this with your approval noted. |
a424847 to
7d5c40d
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 7d5c40d |
github-actions
Bot
removed
the
needs:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Breadcrumbs | 16.1 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · ✓ |
| ContextMenuItem | 26.4 kB (0) · 6.1 kB CSS (0) | — | — · — · — |
| DropdownMenuCheckboxItem | 21.1 kB (0) · 6.4 kB CSS (0) | — | ✓ · ✓ · ✓ |
16 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DropdownMenuDivider | 5.2 kB (0) · 548 B CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuGroup | 6.3 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuItem | 20.4 kB (0) · 6.0 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuRadioGroup | 6.3 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuRadioItem | 21.2 kB (0) · 6.4 kB CSS (0) | — | ✓ · ✓ · — |
| lab/Comments | 279.2 kB (−20 B) · 2.5 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 273.4 kB (−45 B) · 2.4 kB CSS (0) | — | — · — · — |
| ContextMenu · Storybook | 88.0 kB (−51 B) · 16.4 kB CSS (0) | 702 (+3) | ✓ · ✓ · ✓ |
| TableCell | 89.2 kB (−51 B) · 16.8 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 88.8 kB (−60 B) · 16.5 kB CSS (0) | — | — · — · — |
| Table | 97.3 kB (−68 B) · 17.0 kB CSS (0) | — | — · — · — |
| BreadcrumbItem | 75.2 kB (−70 B) · 11.4 kB CSS (0) | — | ✓ · ✓ · ✓ |
| MoreMenu | 85.2 kB (−76 B) · 16.5 kB CSS (0) | — | — · — · — |
| DropdownMenu | 84.9 kB (−81 B) · 16.5 kB CSS (0) | — | ✓ · ✓ · ✓ |
| DropdownMenuSubMenu | 63.5 kB (−81 B) · 9.4 kB CSS (0) | — | ✓ · ✓ · ✓ |
| Markdown | 159.0 kB (−86 B) · 19.3 kB CSS (0) | — | — · — · — |
Package size:
- @astryxdesign/cli 3.38 MB packed (−11 B); no new dependencies
- @astryxdesign/core 6.62 MB packed (−255 B); no new dependencies
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 23 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
There was a problem hiding this comment.
Thanks for closing this keyboard gap. The engineering-owner approval at this head settles the new Tab rows, and in DropdownMenu and ContextMenu the change works as described: the new tests fail without the fix and pass with it. One change is needed before approval:
BreadcrumbItemmenus render the same sub-menu, but their root still ignores keys from a flyout (BreadcrumbItem.tsx:595), so Tab on a breadcrumb sub-menu row now closes only the flyout and leaves focus on the page body with the breadcrumb menu still open; giving that root the same Tab branch as the other two roots, with a Breadcrumbs test, would match BreadcrumbItem FR5 and FR6.
The fix makes a new commit, so an engineering owner needs to comment /approve-spec <full head SHA> again on the new head; the author can do this as an engineering owner. Three comment rewrites are suggested inline and are optional.
[Automated review]
| // In a menu with a header or footer above this level, that level | ||
| // moves focus on; in a plain menu the root closes the whole menu, | ||
| // and focus moves on from its trigger (APG menu button). | ||
| close({focusTrigger: false}); |
There was a problem hiding this comment.
This close now runs under every root that hosts DropdownMenuSubMenu, not only DropdownMenu and ContextMenu. BreadcrumbItem renders the same sub-menu (a nested items entry, or BreadcrumbMenuSubMenu), but its root still returns early for keys from a flyout (BreadcrumbItem.tsx:595). So Tab on a breadcrumb sub-menu row closes only the flyout and leaves focus on the page body with the breadcrumb menu open. Adding the same branch to that root's non-owned path keeps the three roots consistent and matches BreadcrumbItem FR5 and FR6:
if (!ownsEvent(e)) {
if (e.key === 'Tab') {
closeMenu();
}
return;
}A Breadcrumbs test for Tab on a flyout row would guard it.
| // Otherwise Tab leaves the flyout stack: every open level closes as | ||
| // the key bubbles past it, this one's own rows or a deeper level's. | ||
| // In a menu with a header or footer above this level, that level | ||
| // moves focus on; in a plain menu the root closes the whole menu, | ||
| // and focus moves on from its trigger (APG menu button). |
There was a problem hiding this comment.
Optional (comment style): the second sentence carries three topics in 33 words, and "the root closes the whole menu" is not yet true under BreadcrumbItem. A shorter version:
// Tab leaves the flyout stack. Each open level closes as the key passes it.
// The root, or a level above with a header or footer, then moves focus on.| // Except Tab: each open flyout has closed itself as the key bubbled | ||
| // up, and the root moves on as Tab from one of its own rows would. | ||
| // With a header or footer that walks the regions; otherwise the menu | ||
| // closes and focus moves on from the trigger (APG menu button). |
There was a problem hiding this comment.
Optional (comment style): one topic per sentence reads more easily here:
// Tab is the exception. Each open flyout has already closed itself.
// With a header or footer, the walk goes on through the regions.
// Otherwise the menu closes, and focus moves on from the trigger.| // Except Tab: each open flyout has closed itself as the key bubbled up, | ||
| // and the menu closes as Tab from one of its own rows would (APG). |
There was a problem hiding this comment.
Optional (comment style), split into one fact per sentence:
// Tab is the exception. Each open flyout has already closed itself,
// so the menu closes as for Tab on its own row.
|
/approve-spec e6face5 |
There was a problem hiding this comment.
Thanks for the follow-up. BreadcrumbItem menus now close on Tab from a sub-menu row the same way DropdownMenu and ContextMenu do, and the engineering-owner approval at this head settles the new Tab rows. The new tests fail without the fix and pass with it, and in a real browser Tab from a flyout row closes every level and moves focus on in all three menus.
Approved pending CI: the required build, test and docsite-test suites are deferred on this push and run before merge; lint and typecheck pass.
One optional comment-style suggestion is inline.
[Automated review]
| // Except Tab: each open flyout has closed itself as the key bubbled up, | ||
| // and the menu closes as Tab from one of its own rows would (APG). |
There was a problem hiding this comment.
Optional (comment style): this is one 27-word sentence with two topics. A shorter version:
// Tab is the exception. Each open flyout has already closed itself,
// so the menu closes as for Tab on its own row.
There was a problem hiding this comment.
Approved pending CI: the required build, test and docsite-test suites are deferred on this push and run before merge; lint, typecheck and the CLI smoke test pass.
The new commit removes the deleted menuRegionsContext module from the CLI's recorded module names, so the resolution test matches the source again; nothing else changed since the last review. Tab or Shift+Tab on a sub-menu row still closes every level of DropdownMenu, ContextMenu and BreadcrumbItem menus, and the engineering-owner approval of the new Tab rows holds on this head. The optional comment-style suggestions from earlier stay inline.
[Automated review]
…opdownMenu FR23 (main's FR22 is the entrance hold)
|
/approve-spec 655a76d |
There was a problem hiding this comment.
Requesting one small change before approval.
- Miss: the changeset (
.changeset/submenu-tab-closes-menu.md, line 5) says Tab from a sub-menu flyout row now closes everyDropdownMenu, but aDropdownMenuwith its ownheaderorfooterstill closes only the flyout and moves to its footer with the menu open, as before (the existing "Tab from a flyout row closes the flyout and moves to the footer" test asserts this), so please scope the note to menus without aheaderorfooterand end it with "A menu or sub-menu with aheaderorfooterkeeps its Tab cycle."
The fix itself looks right: the new tests fail without the change and pass with it, and the new Tab rows (DropdownMenu FR23, ContextMenu FR11, DropdownMenuSubMenu FR10) carry an engineering owner's approval for this revision. The optional comment-style suggestions from earlier stay inline.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] `DropdownMenu`, `ContextMenu` and `BreadcrumbItem` menus: Tab or Shift+Tab on a row inside an open sub-menu flyout now closes every open level and the menu, calls `onOpenChange(false)` once, and lets focus move on from the trigger, as Tab already did from a top-level row (WAI-ARIA Menu Button). Before, the menu stayed open. A sub-menu with a `header` or `footer` keeps its Tab cycle. |
There was a problem hiding this comment.
This note reads as if every DropdownMenu changed, but a menu with its own header or footer still closes only the flyout on Tab and moves to its footer with the menu open, as before (the existing "Tab from a flyout row closes the flyout and moves to the footer" test in DropdownMenu.test.tsx). A scoped version:
[fix] `DropdownMenu`, `ContextMenu` and `BreadcrumbItem` menus without a `header` or `footer`: Tab or Shift+Tab on a row inside an open sub-menu flyout now closes every open level and the menu, calls `onOpenChange(false)` once, and lets focus move on from the trigger, as Tab already did from a top-level row (WAI-ARIA Menu Button). Before, the menu stayed open. A menu or sub-menu with a `header` or `footer` keeps its Tab cycle.
|
/approve-spec 831d715 |
There was a problem hiding this comment.
Approved pending CI: the deferred build, typecheck and test suites have not run on this head yet; they run before merge.
Thanks for tightening the changeset. It now scopes the note to menus without a header or footer, which matches the new DropdownMenu FR23 row and the header-and-footer tests. The Tab rows (DropdownMenu FR23, ContextMenu FR11, DropdownMenuSubMenu FR10) carry an engineering owner's approval for this head, and the BreadcrumbItem branch follows its existing FR5 ("Escape, Tab, and selection close it"). The five new tests fail on main and pass here, and the header and footer suites still pass.
The earlier comment-style suggestions on the new source comments stay optional.
[Automated review]
cixzhang
added
the
ci:run
Fixes #8840
User impact
A keyboard user in a
DropdownMenuorContextMenusub-menu flyout who presses Tab expects to leave the menu, as Tab does from a top-level row. Instead the menu stays open and focus stays on the flyout row, so the person has to press Escape once per level before they can move on. After this change Tab or Shift+Tab from a row at any level closes every level and the menu, and focus moves on from the trigger in page order.Problem and solution fit
ownsEventis false), andDropdownMenuSubMenu's handler only closes itself on Tab when a header or footer sits above it.The fix completes the existing bubbling design:
DropdownMenuorContextMenu, seeing a Tab from a flyout, closes as it does for its own rows;Expected behavior and authority
Smallest restoration
DropdownMenuSubMenu: a Tab on a level without its own regions closes that level (close({focusTrigger: false})), whatever sits above it.DropdownMenuandContextMenuroots: a Tab from a flyout closes the menu, or walks the regions when the root has them, as before.MenuRegionsContexthad no reader left. It only told a flyout whether a level above had regions, so the second commit removes it.Evidence
Real Chrome 156 (headless), Storybook from
mainand from this branch, same keyboard probe. Tab to the trigger, Enter, ArrowDown to the sub-menu row, ArrowRight into the flyout, then Tab:mainafter Tab in the flyoutmain:DropdownMenuSubMenu.test.tsx"Tab from a flyout leaves the whole menu": one level, Shift+Tab, two levels. Each checks every level closed,onOpenChange(false)once, focus on the trigger, and the Tab not prevented.ContextMenu.test.tsxmatching case.Scope
Testing
pnpm vitest runfor DropdownMenu, ContextMenu and MoreMenu: 412 pass.tsc --noEmitfor core, eslint, prettier,pnpm check:knowledge,pnpm check:sync.Writing rubric results
Carry onto main (conflict-only)
Rebased onto main, keeping both commits. The only conflicts: main moved the ContextMenu, DropdownMenu and DropdownMenuSubMenu specs to
docs/specs/components/and reshaped their requirement tables to ID | Invariant | Basis. The same rows are re-applied there:The old draft-review-state column ("Decided; verified in jsdom") has no place in the new tables, so it is dropped. Code, tests and changeset are unchanged. DropdownMenu and ContextMenu tests: 427/427.
git range-diff (old 153d2d9 → new 7d5c40d)