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
PR Analysis Report
Modified ComponentsContextMenu (@astryxdesign/core)
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression46 of 486 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 | 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.
Approval waits on a maintainer to approve or replace the governing spec for this addition, because component:DropdownMenu is still a draft record: what must be settled is the data-item header/footer and bottom-sheet level-region requirements this PR writes into FR20, the Sub-menu regions row and DEC-13. Two defects need fixes either way, each with a test that reproduces it:
- In a bottom sheet opened from the keyboard, typing into a level's header field (or the menu's own field at a level) moves focus to Back after the first character, because the keyboard-open focus effect in
DropdownMenu.tsxnow re-runs whenever the caller re-rendersitems. - A level whose
itemsfilter to none becomes a plain row in both presentations (renderDropdownItems.tsx,MenuBottomSheetActionList.tsx), so its field and footer disappear mid-typing and the level can't be reopened while the query stays.
[Automated review]
| const currentSubmenu = submenuPath.at(-1); | ||
| // The opened level as the caller renders it now: its rows, header and | ||
| // footer stay live while it shows. | ||
| const currentSubmenu = resolveSheetLevel(items, submenuPath); |
There was a problem hiding this comment.
Reading the level live makes currentItems (next line) a new array on every render where the caller rebuilds items, and the keyboard-open effect further down ([currentItems, isOpen], ending around line 763) then refocuses the sheet's first button, which at a level is Back. In a sheet opened with Enter, typing "perf" into the action-sheet story's Filter labels field leaves "p" with focus on Back, and the menu's own header field at a level regresses the same way. Keying that effect to the open and the level (submenuPath) instead of currentItems would keep the field focused; a keyboard-open typing test would cover it.
There was a problem hiding this comment.
Fixed in 056b6f6, the same issue the 01:48Z review raised again. The keyboard-open effect now depends on [isOpen, submenuPath], not currentItems, so it runs on the open and on each level change but not on caller renders. A case opens the sheet from the keyboard and types into a level's field key by key; it fails with the previous effect.
| label={option.label} | ||
| isDisabled={option.isDisabled}> | ||
| isDisabled={option.isDisabled} | ||
| header={option.header} |
There was a problem hiding this comment.
This branch only runs while option.items.length > 0 (line 114), so when a header filter matches nothing the row turns into a leaf: the flyout, the field being typed in and the footer unmount, focus drops to the page, and while the caller keeps the query the level can't be reopened. A compound DropdownMenuSubMenu with no rows keeps all three. Treating a row with items and a renderable header or footer as a sub-menu even with zero rows, here and in MenuBottomSheetActionList's isSubmenu, would match it.
There was a problem hiding this comment.
Fixed in e9f7275 and narrowed in ecedadd. renderDropdownItems and MenuBottomSheetActionList both read opensSubMenu(item, {showsRegions}): where sub-menu regions show (DropdownMenu's popover and sheet), a row with header or footer stays a sub-menu while its items is empty. Elsewhere, and without either, an empty items is still a plain row. Cases type a filter that matches nothing, in the popover and in the sheet; both fail with the previous code.
| // A leaf row carries no header or footer of its own. | ||
| expect( | ||
| screen.getByRole('menuitem', {name: 'Rename', ...h}), | ||
| ).not.toHaveAttribute('header'); |
There was a problem hiding this comment.
Nit: Rename has no header, so this passes with or without the strip in renderLeafItem. Giving a leaf row a header and asserting it renders nowhere would prove the strip.
There was a problem hiding this comment.
Agreed, the assertion was vacuous. In 2f03c7c, Rename now has a header. The case checks that no such field renders and that the row carries no header attribute; with renderLeafItem's strip removed, the attribute check fails.
| * @file menuSheetPath.ts | ||
| * @input The menu's live `items` and the bottom sheet's path of opened rows | ||
| * @output `resolveSheetLevel`: the opened level as the caller renders it now | ||
| * @position Internal; used by the DropdownMenu and ContextMenu bottom sheets. |
There was a problem hiding this comment.
Nit: only DropdownMenu.tsx imports this at this head; ContextMenu's sheet doesn't use it yet.
232688c to
fcf3a1d
Compare
There was a problem hiding this comment.
Approval waits on a maintainer to approve or replace the governing spec for this addition: component:DropdownMenu is still a draft record, and what must be settled is the data-item header/footer and bottom-sheet level-region requirements this PR writes into FR20, the Sub-menu regions row and DEC-13. Two defects need fixes either way, each with a test that reproduces it:
- In a bottom sheet opened from the keyboard, typing into a level's header field (or the menu's own field at a level) moves focus to Back after the first character, because the keyboard-open focus effect in
DropdownMenu.tsxre-runs whenever the caller re-rendersitems. - A level whose
itemsfilter to none becomes a plain row in both presentations (renderDropdownItems.tsx,MenuBottomSheetActionList.tsx), so its field and footer disappear mid-typing and the level can't be reopened while the query stays.
[Automated review]
fcf3a1d to
e9f7275
Compare
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (facebook#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
There was a problem hiding this comment.
Approval waits on a maintainer to approve or replace the governing spec for this addition: component:DropdownMenu is still a draft record, and what must be settled is the data-item header/footer requirement (including a row staying a sub-menu while its items is empty) and the bottom-sheet level regions this PR writes into FR20, the Sub-menu regions row and DEC-13. The emptied-filter fix holds; two defects need fixes either way, each with a test that reproduces it:
- In a bottom sheet opened from the keyboard, typing into a header field at a level (the level's own, or the menu's own
header) moves focus to Back after the first character, because the keyboard-open focus effect inDropdownMenu.tsxre-runs whenever the caller re-rendersitems; menus that pass no new prop regress too. - A sheet level whose row label changes while it is open, such as a count in "Labels (5)", and that has no
idkeeps its open-time header, so its controlled filter ignores typing; find the row another way, or document and warn that such a row needs anid.
[Automated review]
| // The opened level as the caller renders it now: its rows, header and | ||
| // footer stay live while it shows. | ||
| const currentSubmenu = resolveSheetLevel(items, submenuPath); | ||
| const currentItems = currentSubmenu?.items ?? items; |
There was a problem hiding this comment.
Still open on this revision: at a level currentItems is now a new array on every caller render with inline items, and the keyboard-open effect at lines 729-764 depends on [currentItems, isOpen], so after Enter on the trigger each keystroke in a header field at a level refocuses the sheet's first button, Back. A menu's own controlled header at a nested level keeps "abc" on the base and "a" here. Running that effect on the open and on level navigation (submenuPath) instead, with a case that opens the sheet from the keyboard and types several characters into a level's field, would fix and cover it.
There was a problem hiding this comment.
Fixed in 056b6f6: the keyboard-open focus effect now depends on [isOpen, submenuPath], so it runs on the open and on each level change but not on caller renders, which make a new currentItems at a level read from inline items. New case: open the sheet with Enter, Enter into a level with a controlled filter, focus the field and type "f", "e", "a", letting the sheet's animation frames run after each key. With the previous effect the field keeps "" because focus went back to Back; now it reads "fea" and keeps focus.
| for (const opened of path) { | ||
| const live: DropdownMenuItemData | undefined = | ||
| options != null ? findRow(options, opened) : undefined; | ||
| level = live ?? opened; |
There was a problem hiding this comment.
When a row's label changes while its level shows (a count such as Labels (${visible.length})) and it has no id, no row matches and the level keeps its open-time shape, header included: typing "perf" into that level's controlled filter leaves it empty and the rows unfiltered, while id: 'labels' makes it work. The docs ask for an id only when labels repeat. A positional fallback at this level, or docs plus a development warning for rows whose label can change, would close it.
There was a problem hiding this comment.
Fixed in 056b6f6 with the positional fallback. Each opened row is now kept with its index among its level's rows (openSheetLevel). When no row at the level carries the opened text label any more, as with a count in it, the row at that index is the level, provided it opens a sub-menu. Repeated labels without an id still keep the open-time shape, so they never swap levels; the existing repeated-label case still passes.
New case: a row labelled Labels (${n}), no id, with a controlled filter. Typing "perf" now shows "perf" and filters to Performance, and the heading reads "Labels (1)". With the previous menuSheetPath.ts the field stays empty.
The header JSDoc and the changeset now say an id is needed for repeated labels or rows that reorder.
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (facebook#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
There was a problem hiding this comment.
Approval waits on a maintainer to approve or reject the requirements this PR writes into the draft component:DropdownMenu record: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), the bottom sheet showing a level's own regions in place of the menu's, and how the sheet finds an opened level again, including the new positional match. The keyboard-focus and relabelled-row fixes hold; one new defect needs a fix either way:
- When an opened row without an
idleavesitemswhile its level shows, for example filtered out by the menu's own searchheader, the sheet switches to whichever sub-menu row now sits at that position (menuSheetPath.ts); take the positional match only when it can still be the same row, such as while the level keeps its row count, and add a case for it. - Minor:
ContextMenushares the item type and helpers, so aContextMenurow with aheaderand emptyitemsnow opens an empty sub-menu instead of acting.
[Automated review]
| (sameLabel.length === 0 && atPosition != null && opensSubMenu(atPosition) | ||
| ? atPosition | ||
| : undefined) |
There was a problem hiding this comment.
When the opened row has no id and leaves items while its level shows (say the menu's own search header, which still shows at a level without regions of its own, filters it out), no row carries its label, so this takes whatever sub-menu row now sits at that index: the level's rows, heading and dialog name switch to a sub-menu the person never opened, with no new prop involved. The base and the previous revision keep the opened level. Taking the positional candidate only while the level keeps the row count it had when the row opened (a relabel keeps it, a removal does not) would close it, and a case that filters the opened row out would cover it.
There was a problem hiding this comment.
Fixed in ecedadd. Each path entry now also keeps its level's row count (SheetPathEntry.count). Position is used only for an opened row without an id, when no row carries its label any more, while the level still has that many rows and the row there opens a sub-menu: that is, only when it can still be the same row. Otherwise the level keeps its last known shape.
New case: a sheet whose own search header filters the root rows "Move to", "Labels" and "Copy to". Open Labels, then type "to" into that search, which still shows on Labels. The level stays Labels with "Bug", with ids and without. The previous menuSheetPath.ts switches to "Copy to" and "Drafts" in both. The counted-label and repeated-label cases still pass.
| item.items != null && | ||
| (item.items.length > 0 || | ||
| isRenderable(item.header) || | ||
| isRenderable(item.footer)) |
There was a problem hiding this comment.
ContextMenu also renders through renderDropdownItems and MenuBottomSheetActionList, and ContextMenuItemData aliases DropdownMenuItemData, so a ContextMenu row with header and items: [] now opens an empty sub-menu (no rows, no field) where the base rendered a plain row. Applying the empty-items rule only where regions render would keep ContextMenu as it was.
There was a problem hiding this comment.
Fixed in ecedadd. The rule now applies only where regions render. opensSubMenu(item, {showsRegions}) reads the presentation:
renderDropdownItemspasses it through its recursion;MenuBottomSheetActionListtakes it asshowsLevelRegions;- DropdownMenu's popover and sheet pass
true; ContextMenu and Breadcrumbs leave it off.
So in this PR a ContextMenu row with header and items: [] stays a plain row, as before. A new ContextMenu case checks it in the pointer menu (no aria-haspopup) and in the sheet (a press acts and closes); it fails on the previous head. #7840, where ContextMenu shows sub-menu regions, passes the flag there and turns that case into an empty sub-menu that shows its field.
056b6f6 to
ecedadd
Compare
There was a problem hiding this comment.
Approval waits on a maintainer to approve or reject the requirements this PR writes into the draft component:DropdownMenu record: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), the bottom sheet showing a level's own regions in place of the menu's, and how the sheet finds an opened level again in the current items. The Recent-row fix from the last round holds and no defect blocks; two small points either way:
- FR20's "or a data item with
items, even empty" reads as applying under ContextMenu too, where such a row stays a plain row, so scope it to DropdownMenu. DropdownMenu.doc.mjsstill says an item'sidis needed only when the array reorders or filters, but in the bottom sheet a level opened from a repeated label without anidignores typing in its filter, so name repeated labels there as theheaderJSDoc does.
[Automated review]
| type: 'DropdownMenuOption[]', | ||
| description: | ||
| 'Array of menu entries. Each entry is one of: an action item `{label, onClick?, href?, target?, rel?, icon?, description?, endContent?, isDisabled?, variant?, hasCloseOnSelect?, id?}` (`href` makes the row a real link, so a modified click keeps the browser\'s meaning; variant `"destructive"` renders it in the error color; `endContent` holds trailing content such as a keyboard-shortcut hint; `id` is the row\'s stable React key, needed only when the array reorders or filters), a divider `{type: "divider"}`, or a section `{type: "section", title?, id?, items: [...action items]}`.', | ||
| 'Array of menu entries. Each entry is one of: an action item `{label, onClick?, href?, target?, rel?, icon?, description?, endContent?, isDisabled?, variant?, hasCloseOnSelect?, id?, items?, header?, footer?}` (nested `items` make the row a sub-menu, and `header` and `footer` then sit above and below that sub-menu\'s rows, as on `DropdownMenuSubMenu`, and keep the row a sub-menu while `items` is empty; `href` makes the row a real link, so a modified click keeps the browser\'s meaning; variant `"destructive"` renders it in the error color; `endContent` holds trailing content such as a keyboard-shortcut hint; `id` is the row\'s stable React key, needed only when the array reorders or filters), a divider `{type: "divider"}`, or a section `{type: "section", title?, id?, items: [...action items]}`.', |
There was a problem hiding this comment.
This still says id is "needed only when the array reorders or filters". In the bottom sheet, a level opened from a row without an id whose label repeats at its level is never found again (menuSheetPath.ts:89-90), so a controlled header filter there ignores typing: two "Size" rows, each with a filter field; typing into the image level's field leaves it empty and both sizes listed. The header JSDoc already asks for an id in that case; naming repeated labels here too would keep astryx docs in step.
There was a problem hiding this comment.
Done in 9206efa. The items doc now says id is "needed when the array reorders or filters, and in the bottom sheet for a row with nested items whose label another row at its level shares". The id JSDoc on DropdownMenuItemData says the same, matching the header JSDoc.
| | 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 | | ||
| | FR20 | With `header` or `footer` on a sub-menu (`DropdownMenuSubMenu`, or a data item with `items`, even empty), 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. In the bottom sheet a level's own regions replace the menu's. Without either, nothing changes. | Proposed DEC-13; FR15; AR5 | Proposed; verified in jsdom and Chromium, pending owner review | |
There was a problem hiding this comment.
"or a data item with items, even empty" carries no root qualifier, but ContextMenu and Breadcrumbs keep such a row a plain row (showsRegions is off there, and ContextMenu.test.tsx asserts it). Scoping "even empty" to DropdownMenu's popover and sheet would make the text match the code.
There was a problem hiding this comment.
Scoped in 9206efa: FR20 now reads "(DropdownMenuSubMenu, or a data item with items, in a DropdownMenu even empty)". Under ContextMenu and Breadcrumbs such a row stays a plain row, as the ContextMenu case asserts. The cell keeps its width.
…nd the bottom sheet shows a level's own A data item with items takes header and footer, as DropdownMenuSubMenu does: renderDropdownItems passes them to the sub-menu it renders, so the popover shows them as for any sub-menu with regions. A level whose data item has a header or footer shows them in place of the menu's own. The level is read from the current items (by identity, id, or a label unique at its level), so a field there filters it live. Each level's regions are keyed by whose they are, so what was typed into one field never shows up in another.
…nu while its items are empty renderDropdownItems and MenuBottomSheetActionList made a row a sub-menu only while items.length > 0, so a header filter that matched nothing turned it into a plain row: in the popover the dialog, its focused field and its footer unmounted mid-typing, and in the bottom sheet the level could not be opened again after Back. Both now read opensSubMenu: a row with items opens a sub-menu, and with a header or footer it stays one while items is empty, as the compound DropdownMenuSubMenu does. Without either an empty items is still a plain row. FR20, the docs and the changeset say so. Two new cases (the popover's field keeps focus and value through a filter that matches nothing; the sheet's row opens its level again after Back) fail with the previous renderDropdownItems.tsx and MenuBottomSheetActionList.tsx.
…nd finds a level whose label changes Two defects at a level read live from inline items. The keyboard-open focus effect depended on currentItems, a new array on every caller render, so after a keyboard open each keystroke in a level's field (or the menu's own controlled header) refocused the sheet's first action; it now runs on the open and on level changes (submenuPath). And a row whose label changes while its level shows (a count in it) matched no row, so the level kept its open-time header and rows; the sheet now remembers where each opened row sat, and when no row at its level carries its label any more it finds the row there, if that row opens a sub-menu. Repeated labels still keep the open-time shape without an id, as before. A keyboard-open case typing into a level's field and a counted-label case typing a filter fail with the previous DropdownMenu.tsx and menuSheetPath.ts respectively. The header JSDoc and the changeset say when an id is needed.
…can be the same row, and empty levels only where regions show The positional fallback in the sheet's level path took whichever sub-menu row sat at the opened row's index, so a row filtered out of its level, by the menu's own search header say, switched the sheet to a different level, ids or not. Each path entry now keeps its level's row count, and position is used only for a row without an id while the level keeps that count; otherwise the level keeps its last known shape. And the empty-items rule (a row with header or footer stays a sub-menu while items is empty) applied wherever renderDropdownItems and MenuBottomSheetActionList render, ContextMenu and Breadcrumbs included, where no sub-menu regions show: a row with header and items: [] there became an empty sub-menu instead of a plain row. Both now take showsRegions, which DropdownMenu's popover and sheet pass and others leave off. A sheet case where the menu's search filters out the opened row (without ids and with them) and a ContextMenu case for a header with empty items fail with the previous code.
…abel was its own When the opened row left items while its level showed, the sheet took the one row that now carried its label, even when the opened row had an id that row did not share: with two rows labelled Size, ids included, the menu's own search filtering out the opened one switched the sheet to the other's sub-menu. A row with an id is now found by identity or that id alone, and a row without one by its label only when the label was its own at its level when it opened (SheetPathEntry.isLabelUnique) and still is; otherwise the level keeps its last known shape. A case where the opened one of two rows labelled alike leaves items, with ids and without, fails with the previous menuSheetPath.ts in both arms. The header JSDoc and the changeset say so.
…s its last rows When an open level's row was found again but no longer had items (a Recent row built as items: recents.length > 0 ? ... : undefined, emptied by a Clear recent button in the menu's own footer), the sheet fell back to the menu's root items and listed them under the level's heading, Back and dialog name. resolveSheetLevel now takes a found row as the level only while it opens a sub-menu; otherwise the level keeps its last known shape, as it does for a row filtered out. The header comment also says that, without an id, the label match reads uniqueness from the rows shown when the level opened, so rows whose labels repeat need an id for the guarantee. A case where the open level's row loses its items fails with the previous menuSheetPath.ts.
… and the items doc says when the sheet needs an id FR20's "or a data item with items, even empty" read as applying under ContextMenu and Breadcrumbs too, where such a row stays a plain row (showsRegions is off there); it now reads "in a DropdownMenu even empty", and the cell keeps its width. The items doc and the id JSDoc said an id is needed only when the array reorders or filters; they now also name the bottom sheet's case, a row with nested items whose label another row at its level shares, as the header JSDoc does. Record and doc wording only.
3c96ff6 to
9206efa
Compare
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (facebook#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
There was a problem hiding this comment.
Approval waits on a maintainer to approve or reject the requirements this PR writes into the draft component:DropdownMenu record: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), the bottom sheet showing a level's own regions in place of the menu's, and how the sheet finds an opened level again in the current items. The two points from the last round are fixed and no defect blocks; one small docs point either way:
- In the bottom sheet, a level whose row has no
idand a label that changes while it shows (such asLabels (${n})) falls back to its open-time rows and header once the root's rows grow, so a controlled filter there drops what was typed;DropdownMenu.doc.mjscould sayidis needed when the array reorders, filters or grows, as the JSDoc does, andmenuSheetPath.tscould call that fallback the open-time shape rather than the last known one.
[Automated review]
| found != null && opensSubMenu(found, {showsRegions: true}) | ||
| ? found | ||
| : undefined; | ||
| level = live ?? opened.row; |
There was a problem hiding this comment.
Minor: this falls back to the row as it was when the level opened, not the last one found. With a Labels (${n}) row and no id, typing "perf" into the level's controlled filter and then adding a root row (say from the level's own footer) brings back Labels (3) with all three rows and an empty field, and further typing is ignored because the open-time header element keeps its open-time value. The id JSDoc already asks for an id when items can grow; saying the same in DropdownMenu.doc.mjs and calling this the open-time shape in the header comment would match the code.
There was a problem hiding this comment.
Done in 8e8d156, as you suggest.
The header comment now calls the fallback the shape the level had when it opened (its rows, header and footer then), and says a controlled field in that open-time header shows its open-time value. It also names this case: a row without an id whose label changes while its level shows is found only while its level keeps its row count.
The items doc now says id is "needed when the array reorders, filters or grows, and in the bottom sheet for a row with nested items whose label another row at its level shares or changes while its level shows". The id and header JSDoc and the changeset say the same. Comments and docs only.
…the docs say when a row needs an id
When the sheet cannot find an opened row again, the level keeps the shape it had when it opened, its rows, header and footer then, so a controlled field in that open-time header shows its open-time value. The header comment called that the last known shape; it now says the open-time shape and names the case the bot found: a row without an id whose label changes while its level shows (Labels (${n})) is found only while its level keeps its row count, so adding a root row brings the open-time level back. The items doc, the id and header JSDoc and the changeset now say an id is needed when the array reorders, filters or grows, and in the sheet when a row's label repeats or changes while its level shows. Comments and docs only.
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (facebook#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
…sses none on The data-mode case asserted that the Rename row carries no header attribute, but Rename had no header, so it passed with or without renderLeafItem's strip. Rename now has a header: the case checks that no such field renders and that the row carries no header attribute, which fails when the strip is removed.
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (facebook#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
There was a problem hiding this comment.
Approval waits on a maintainer to approve or change the requirements this PR writes into the draft component:DropdownMenu record: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), the bottom sheet showing a level's own regions in place of the menu's, and how the sheet finds an opened level again in the current items. The earlier points are fixed and no defect blocks; one small docs point either way:
- In the bottom sheet a level with only its own
footer(orheader) also hides the menu's other region, but the data-item JSDoc and the menu'sheader/footerdocs describe a per-region swap; say that a level with either region shows neither of the menu's, or fall back per region if that is the intent.
[Automated review]
| const levelHeader = levelHasRegions ? currentSubmenu.header : header; | ||
| const levelFooter = levelHasRegions ? currentSubmenu.footer : footer; |
There was a problem hiding this comment.
A level with either region of its own replaces both of the menu's here, so a level with only a footer also drops the menu's header (a menu-wide search field disappears at that level). The header and footer JSDoc above (lines 342-343 and 353-354) and the menu's header/footer entries in DropdownMenu.doc.mjs read as a per-region swap. Saying there that a level with either region shows neither of the menu's would match this code; if a per-region swap is the intent, falling back per region here would match the docs instead.
There was a problem hiding this comment.
Took the docs route, in 84cb23f. In the bottom sheet a level with a header or footer of its own shows its regions and neither of the menu's. That matches FR20 ("replacing the root's menu and regions") and the popover, where such a sub-menu's dialog holds only its own regions. A menu-wide search shown at a level with only a footer would also filter rows the level doesn't show.
The header and footer JSDoc, the items and DropdownMenuSubMenu docs, the changeset and FR20 now say so. FR20 keeps its width. A new case drills into a level with only a footer and finds neither the menu's search field nor its footer; with a per-region fallback in the code it fails.
…her of the menu's In the bottom sheet a level whose row has a header or footer of its own shows its regions and neither of the menu's, so a level with only a footer also drops the menu's header: the menu-wide search would filter rows the level does not show. The header and footer JSDoc, the items and DropdownMenuSubMenu docs, the changeset and FR20 read as a per-region swap; they now say so (FR20 keeps its width). A case drills into a level with only a footer and finds neither the menu's search nor its footer; it fails with a per-region fallback.
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (facebook#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
There was a problem hiding this comment.
Approval waits on a maintainer to approve or change the requirements this pull request writes into the draft component:DropdownMenu record: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), the bottom sheet showing a level's own regions in place of both of the menu's, and how the sheet finds an opened level again in the current items. The earlier points are fixed and no defect blocks; two small points either way:
- FR20 ends "Without either, nothing changes", but every opened bottom-sheet level now follows the current
items, so a level without regions shows rows added while it is open; if that is intended, the record should say so. - The menu's own
headerandfooterentries inDropdownMenu.doc.mjsstill say the bottom sheet shows them above and below the actions, with no exception for a level that has regions of its own.
[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 | | ||
| | FR20 | With `header` or `footer` on a sub-menu (`DropdownMenuSubMenu`, or a data item with `items`, in a `DropdownMenu` even empty), 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, outside every menu; under a root with no such host (`ContextMenu`) it shows its rows alone. On a fine pointer the dialog is the flyout; on a compact touch display, the drilled-in view, replacing 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 resumes at 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 handled it, returns focus to the row; the root stays open. A pointer leaving the flyout keeps it open until focus leaves, and it closes with the flyout holding its row. In the bottom sheet a level's own regions replace both of the menu's. Without either, nothing changes. | Proposed DEC-13; FR15; AR5 | Proposed; verified in jsdom and Chromium, pending owner review | |
There was a problem hiding this comment.
FR20 ends "Without either, nothing changes", but the bottom sheet now reads every opened level from the current items for every caller (resolveSheetLevel, DropdownMenu.tsx:685), regions or not: with no level regions anywhere, a row the caller adds while a level shows appears on this head and not on the base. If live levels are intended, stating them here (or in a requirement of their own) would keep the record true.
There was a problem hiding this comment.
Live levels are intended, so they now have a requirement of their own, in 8bb57c6. A menu-wide search in the menu's own header that filters nested rows needs them whether or not any level has regions; snapshotting regionless levels would leave that level unfiltered.
Proposed FR23: in the bottom sheet each opened level is read from the current items while it shows, with or without regions. It also says how the sheet finds the opened row again (identity, id alone, a label unique then and now, or position while the row count holds) and that a row it can't find, or one that no longer opens a sub-menu, leaves the level as it was when it opened. FR20 now ends "Without either, its surface is unchanged." (the cell keeps its width). The items JSDoc and docs and the changeset say so.
New case: a menu with no regions anywhere is re-rendered with a row added to an open level, and the row appears. With the level read from its open-time row, as on main, it fails. #7840 makes ContextMenu's FR12 point at FR23 too.
… from the current items The sheet reads each opened level from the current items for every caller, regions or not, so a row the caller adds, removes or relabels while a level shows, or that the menu's own header filters, shows at once; the base kept a snapshot from when the level opened. FR20 ended "Without either, nothing changes", which the live read contradicted. Live levels are intended (a menu-wide search that filters nested rows needs them, with or without level regions), so proposed FR23 states them, with how the sheet finds the opened row again and the open-time fallback, and FR20 now says that without a region the sub-menu's surface is unchanged (the cell keeps its width). The items JSDoc and docs and the changeset say so. A case re-renders a menu with no regions anywhere with a row added to an open level; it fails with the level read from its open-time row, as the base did.
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (facebook#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
There was a problem hiding this comment.
Approval waits on a maintainer to approve or change the requirements this pull request writes into the draft component:DropdownMenu record, the governing spec for this addition. Three things need settling: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), the bottom sheet showing a level's own regions in place of the menu's, and new FR23's live sheet levels with their row-matching rules. FR23 now records the live levels and the earlier points are fixed. One small docs fix remains either way: the menu's own header and footer entries in DropdownMenu.doc.mjs still say the bottom sheet shows them above and below the actions, with no exception for a level that has regions of its own.
[Automated review]
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (facebook#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
User need
#7530 gave
DropdownMenuSubMenuaheaderandfooter(a filter field, a "Create" button). A menu built fromitemscannot give a nested level the same, and the bottom sheet, which renders data items, shows the menu's own regions at every level, so a level's controls are missing on a phone.Problem and solution fit
A data item with
itemstakesheaderandfooter;renderDropdownItemspasses them to theDropdownMenuSubMenuit renders, so the popover shows them as #7530 does. In the bottom sheet, a level whose row has either shows them around its actions in place of both of the menu's. Each level's regions mount on their own, so text typed into one level's field never shows in another.Current specification
component:DropdownMenu(draft): the Sub-menu regions concept covers both presentations; FR20 and DEC-13, proposed with #7530, name data items and the sheet's level regions; new FR23 says every sheet level follows the currentitems. Other proposals in open pull requests: #7319 FR18/DEC-11, #7352 FR19/DEC-12, #7532 FR21/DEC-14, #7541 FR22/DEC-15; #7840 extends FR20 toContextMenu.Public delta and caller burden
header?andfooter?on a data item, read on a row withitems; with either, the row stays a sub-menu whileitemsis empty, so a filter that matches nothing keeps its field.itemswhile it shows (FR23), finding its row byid, by a label it has to itself, or, while its level keeps its row count, by position; rows that share a label, change it while they show, or reorder need anid.Design and alternatives
The sheet already is a modal dialog, so a level's regions sit in it directly, above and below its actions. Rejected: keeping the menu's regions at every level, which hides the level's own controls on a phone; and live levels only where regions show, which leaves a menu-wide search unable to filter nested rows.
Writing rubric results
Evidence
See Testing.
Scope
Testing
Review round (8bb57c6): FR20 ended "Without either, nothing changes", but the sheet reads every opened level from the current
items, regions or not. Live levels are intended, so new FR23 states them (with how the sheet finds the opened row again and the open-time fallback), and FR20 now says that without a region the surface is unchanged; theitemsJSDoc and docs and the Changeset say so. A new case re-renders a menu with no regions anywhere with a row added to an open level; it fails with the level read from its open-time row, as onmain. DropdownMenu, ContextMenu, Breadcrumbs and doc-prop suites: 445 tests; sub-menu header and footer Chromium spec: 7; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (84cb23f): a sheet level with a
headerorfooterof its own shows neither of the menu's regions, so a level with only afooteralso drops the menu'sheader; the JSDoc, theitemsandDropdownMenuSubMenudocs, the changeset and FR20 read as a per-region swap and now say so. A new case (a level with only a footer shows neither the menu's search nor its footer) fails with a per-region fallback. DropdownMenu, ContextMenu, Breadcrumbs and doc-prop suites: 444 tests; sub-menu header and footer Chromium spec: 7; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (2f03c7c): the data-mode case asserted the Rename row carries no
headerattribute, but Rename had noheader; it now has one, and the case checks that no such field renders and the row carries noheaderattribute, which fails whenrenderLeafItemstops stripping it. Test only. DropdownMenu, ContextMenu and Breadcrumbs suites: 443 tests; sub-menu header and footer Chromium spec: 7; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (8e8d156): when the sheet cannot find an opened row again, the level keeps the shape it had when it opened; the header comment now says the open-time shape rather than the last known one, and names the case found (a row without an
idwhose label changes while its level shows is found only while the level keeps its row count). Theitemsdoc, theidandheaderJSDoc and the changeset now say anidis needed when the array reorders, filters or grows, and in the sheet when a row's label repeats or changes while its level shows. Comments and docs only. DropdownMenu, ContextMenu, Breadcrumbs, Layer, MoreMenu and doc-prop suites: 679 tests; sub-menu header and footer Chromium spec: 7; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (9206efa, rebased onto
main1391928): FR20's "or a data item withitems, even empty" now reads "in aDropdownMenueven empty", since ContextMenu and Breadcrumbs keep such a row a plain row; theitemsdoc and theidJSDoc now also name the bottom sheet's case for anid(a row with nesteditemswhose label another row at its level shares). Record and doc wording only. The previous head'stest-node (2/4)andtest-ui (1/4)reds were in files this change does not touch (packages/cli/foundation/release/cache.test.mjs, a process-kill timing case, andSideNav.test.tsx, which usesusePopover, not DropdownMenu); both pass locally on that head (2/2 and 3/3), and this push reruns them. DropdownMenu, ContextMenu, Breadcrumbs, SideNav, Layer, MoreMenu, RadioList,useMenuPressand doc-prop suites: 1016 tests; CLI node project: 252; header and footer Chromium specs: 17; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (3c96ff6, rebased onto
mainea68606): when an open level's row was found again but no longer haditems(a Recent row emptied by a "Clear recent" button in the menu's ownfooter), the sheet fell back to the menu's root rows under the level's heading;resolveSheetLevelnow takes a found row only while it opens a sub-menu, and otherwise keeps the level's last known shape. The header comment says that, without anid, the label match reads uniqueness from the rows shown when the level opened, so repeated labels need anid. A new case (the open level's row loses itsitems) fails with the previousmenuSheetPath.ts. DropdownMenu, ContextMenu, Breadcrumbs, Layer, MoreMenu, RadioList,useMenuPressand doc-prop suites: 801 tests; header and footer Chromium specs: 17; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (6df50bc, rebased onto
main947b293): when the opened row leftitems, the sheet took the one row that now carried its label, even when the opened row had anidthat row did not share. A row with anidis now found by identity or thatidalone, and a row without one by its label only when the label was its own at its level when it opened (SheetPathEntry.isLabelUnique) and still is; otherwise the level keeps its last known shape. A new case (two rows labelled Size under the menu's own search; open the image one and type "text") fails with the previousmenuSheetPath.ts, with ids and without. DropdownMenu, ContextMenu, Breadcrumbs, Layer, MoreMenu, RadioList,useMenuPressand doc-prop suites: 800 tests; header and footer Chromium specs: 17; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (ecedadd, rebased onto
mainc5e01c0): the positional fallback took whichever sub-menu row sat at the opened row's index, so a row filtered out of its level (by the menu's own searchheader) switched the sheet to another level, ids or not; each path entry now keeps its level's row count, and position is used only for a row without anidwhile that count holds. The empty-itemsrule now applies only where sub-menu regions show:renderDropdownItemsandMenuBottomSheetActionListtakeshowsRegions, which DropdownMenu passes and ContextMenu and Breadcrumbs leave off, so a ContextMenu row withheaderanditems: []stays a plain row. Two new cases fail with the previous code: a sheet whose search filters out the opened row (with and without ids), and that ContextMenu row in the pointer menu and the sheet. DropdownMenu, ContextMenu, Breadcrumbs, Layer, MoreMenu, RadioList,useMenuPressand doc-prop suites: 799 tests; header and footer Chromium specs: 17; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (056b6f6): the sheet's keyboard-open focus effect depended on
currentItems, a new array on every caller render at a level read from inlineitems, so after a keyboard open each keystroke in a level's field refocused Back; it now runs on the open and on level changes (submenuPath). A row whose label changes while its level shows (a count) matched no row, so the level kept its open-time header and rows; the sheet now remembers where each opened row sat and, when no row at its level carries its label any more, finds the row there if it opens a sub-menu. Repeated labels without anidstill keep the open-time shape. Two new cases fail with the previous code: a keyboard open typing "fea" into a level's field (the field kept ""), and a counted labelLabels (${n})typing "perf" (the field stayed empty). DropdownMenu, ContextMenu, Layer, MoreMenu, RadioList,useMenuPressand doc-prop suites: 747 tests; header and footer Chromium specs: 17; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round (rebased onto
main968130f):renderDropdownItemsandMenuBottomSheetActionListmade a row a sub-menu only whileitemswas non-empty, so a header filter that matched nothing turned it into a plain row: in the popover the dialog, its focused field and footer unmounted mid-typing; in the bottom sheet the level could not be opened again after Back. Both now readopensSubMenu: with aheaderorfooter, a row withitemsstays a sub-menu while they are empty, as the compoundDropdownMenuSubMenudoes; without either, an emptyitemsis a plain row as before. Two new cases (the popover's field keeps focus and value through a filter that matches nothing; the sheet's row opens its level again after Back) fail with the previousrenderDropdownItems.tsxandMenuBottomSheetActionList.tsx. DropdownMenu, ContextMenu, Layer, MoreMenu, RadioList,useMenuPressand doc-prop suites: 745 tests; header and footer Chromium specs: 17; typecheck,lint:strict,check:knowledge, Storybook build: clean.Rebased onto
maincab6e90, no conflicts or changes: main's newpr-a11yTopNav step runs a spec file the previous head predates. DropdownMenu, ContextMenu and doc-prop suites: 385 tests; typecheck,check:knowledge: clean.On
mainb7860bc (#7530 merged).DropdownMenuSubMenu.test.tsx, four new cases (data mode passes a nested item's regions; the sheet shows a level's own regions, live; each level's field opens empty; a repeated label keeps the opened level): withmain'sDropdownMenu.tsxandrenderDropdownItems.tsx, 4 failed; with the change, 4 passed.__tests__/SubMenuHeaderFooter.a11y.chromium.spec.tsgains an action-sheet case on the new "Submenu / header and footer, action sheet" story: a level shows its own field and button around its actions, and the field filters them live.pr-a11yalready runs this file.useMenuPressand doc-prop suites: 743 tests; header and footer Chromium specs: 17; typecheck,lint:strict,check:knowledge, Storybook build: clean.