Repository navigation
Conversation
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@astryxdesign/core) · View in Storybook
Component 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. Visual Regression57 of 504 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 37 more. 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.
Approval waits on a maintainer decision: component:ContextMenu FR12 is new and component:DropdownMenu FR20 and DEC-13 are rewritten, all in draft records, and no owner has approved that text at this head, so a maintainer needs to approve it or say what must change (including FR20's change from showing a ContextMenu sub-menu's rows alone to showing its regions). Separately, one fix is needed whatever the decision: in a ContextMenu with presentation="bottom-sheet" and compound menuContent, a sub-menu with header or footer now renders its dialog, field and button inside the sheet's role="menu" without the development warning, because the surface context is provided where no host is rendered (ContextMenu.tsx, the DropdownMenuSurfaceContext provider).
- Providing
nullfor the surface context when the bottom sheet is used would keep the previous rows-alone fallback there, and a compound bottom-sheet test would lock it in. - Smaller points:
ContextMenu.doc.mjsdoes not list the newheaderandfooteritem keys, and on a phone the drilled-in regions view is painted without thecontext-menutheme target or the caller's styling.
[Automated review]
| <DropdownMenuRootOpenContext value={isOpen}> | ||
| {resolvedMenuContent} | ||
| </DropdownMenuRootOpenContext> | ||
| <DropdownMenuSurfaceContext value={surfaceValue}> |
There was a problem hiding this comment.
renderedMenu is also the compound content of the bottom sheet (line 830), where no surface host is rendered. A DropdownMenuSubMenu with header or footer there sees the context, turns regions on (aria-haspopup="dialog"), finds no host element, and renders its dialog flyout in place, inside the sheet's role="menu", with no dev warning. Passing usesBottomSheet ? null : surfaceValue keeps the rows-alone fallback in the sheet.
There was a problem hiding this comment.
Done in 5656b06: the surface context is null in the bottom sheet, so a compound sub-menu with header or footer shows its rows alone there, with the development warning, now worded "only in a DropdownMenu or in a ContextMenu's pointer menu". A new compound bottom-sheet case pins it and fails with the previous ContextMenu.tsx. ContextMenu FR12, DropdownMenu FR20 and the changeset say so.
| // menu, never inside it. It owns no box until such a sub-menu's drilled-in | ||
| // view shows there; then the menu steps aside (see `menu`) and the host | ||
| // paints the menu's surface in its place. | ||
| surfaceHost: { |
There was a problem hiding this comment.
While a regioned drilled-in view shows, this host paints the menu's surface with fixed tokens, but the context-menu theme target and the caller's xstyle, className and style stay on the hidden menu (lines 750–759). Themed or caller-styled ContextMenus change look when the view opens.
There was a problem hiding this comment.
Done in 5656b06: the host carries the menu's context-menu theme target and the caller's xstyle, className and style, with its own display rule applied last, so it still owns no box until a view shows. Its radius reads --_dropdown-menu-radius, as the menu's does, so a theme's radius applies too. A new case checks that the drilled-in view's surface carries the theme class and the caller's class and style; it fails on the previous head. ContextMenu.doc.mjs now lists the header and footer item keys.
6148369 to
5656b06
Compare
There was a problem hiding this comment.
Approval waits on a maintainer decision: component:ContextMenu FR12 is new and component:DropdownMenu FR20 and DEC-13 are rewritten, all in draft records, and no owner has approved that text at this head, so a maintainer needs to approve it or say what must change (including FR20's change from showing a ContextMenu sub-menu's rows alone to showing its regions). Separately, one fix is needed whatever the decision: inside a ContextMenu, a mouse press on a regioned sub-menu's row, header field or footer button closes the whole menu without acting, because ContextMenu's outside-press listener (ContextMenu.tsx lines 503–521) treats the new surface host as outside the menu.
- Counting the surface host as inside that check, with a ContextMenu test that clicks a row, the field and the button using real pointer events, would fix and lock it in.
- Smaller: a drilled-in regions view opened by mouse shows the keyboard focus ring, because ContextMenu does not start the shared interaction-modality tracker as DropdownMenu does.
[Automated review]
| <LayerDepthProvider> | ||
| {renderedMenu} | ||
| <div | ||
| ref={attachSurfaceHost} |
There was a problem hiding this comment.
This host renders beside the role="menu" element, outside listRef, so the document mousedown listener at lines 503–521 (!menu.contains(e.target)) closes the ContextMenu on any mouse press inside a regioned sub-menu. In Chromium a click on one of its rows, its header field or its footer button closes the whole menu without acting, and a touch tap on the field does too; only the keyboard reaches them. Counting the host as inside (for example && !surfaceHostRef.current?.contains(e.target as Node)) plus a ContextMenu case that clicks a row, the field and the button with real pointer events would cover it.
There was a problem hiding this comment.
Fixed in 3d7064b: the outside check is now menu && !menu.contains(target) && !surfaceHostRef.current?.contains(target), so a press in the dialog (its field, button or rows) stays inside, and so does a touch tap's compatibility mousedown. Two cases cover it, both failing with the previous ContextMenu.tsx:
- a user-event case that clicks the field (it keeps focus and takes typing), the button (it acts) and a row (it acts), with the menu's popover still open after the first two;
- a Chromium case on a new ContextMenu "Submenu / header and footer" story that right-clicks, opens Labels, then clicks the field, types, clicks "Create label" and picks a row. Against a Storybook built from the previous
ContextMenu.tsx, the field click finds no field.
| [closeMenu, size, drillIn, label], | ||
| ); | ||
| // What a sub-menu with a header or footer needs, on an internal context. | ||
| const surfaceValue = useMemo<DropdownMenuSurfaceValue>( |
There was a problem hiding this comment.
With the surface provided here, a regioned sub-menu's drilled-in view paints the keyboard focus ring when getInteractionModality() reads 'keyboard' (DropdownMenuSubMenu.tsx line 1177). DropdownMenu starts the shared tracker at its root (DropdownMenu.tsx line 972); ContextMenu does not, so on a page with nothing else tracking, a mouse open of the view shows the ring. Calling useInteractionModalityTracking() in ContextMenu would match DropdownMenu.
There was a problem hiding this comment.
Done in 3d7064b: ContextMenu calls useInteractionModalityTracking(), as DropdownMenu does, so a mouse open of the drilled-in view reads pointer input and shows no keyboard ring. Its case lives in a file of its own (ContextMenuInteractionModality.test.tsx), because once any component starts the document-wide tracker it stays on for the file, and the bottom-sheet cases start it through BottomSheet. It reads "keyboard" with the previous ContextMenu.tsx and "pointer" with the change. #7303 adds the same call for its own rule; whichever merges second keeps one.
There was a problem hiding this comment.
Approval waits on a maintainer decision: component:ContextMenu FR12 is new and component:DropdownMenu FR20 and DEC-13 are rewritten, all in draft records, so a maintainer needs to approve that text on this head or say what must change, including FR20's switch from showing a ContextMenu sub-menu's rows alone to showing its header and footer. The earlier press and focus-ring problems are fixed. One fix is needed whatever the decision: a data item whose header filter leaves its items empty is rendered as a plain row (renderDropdownItems.tsx line 114 and MenuBottomSheetActionList.tsx line 82), so in the popover the field and footer vanish after one unmatched character, and in the bottom sheet the level cannot be reopened after Back.
- Treating a data item that has
items(even empty) and aheaderorfooteras a sub-menu in both places, with a test that types a filter matching nothing, would fix it and lock it in.
[Automated review]
| label={option.label} | ||
| isDisabled={option.isDisabled}> | ||
| isDisabled={option.isDisabled} | ||
| header={option.header} |
There was a problem hiding this comment.
These keys reach the sub-menu only while option.items.length > 0 (line 114); a header filter that empties items turns the row into a leaf (renderLeafItem strips header and footer), so the field and footer unmount mid-typing and focus falls to the page. MenuBottomSheetActionList.tsx line 82 has the same test, so in the sheet the level can't be reopened after Back. Treating a row with items (even empty) plus a renderable header or footer as a sub-menu in both places, as compound DropdownMenuSubMenu already behaves, would fix it.
There was a problem hiding this comment.
Fixed where the code lives: in #7839's e9f7275, which this pull request now sits on (5c782b2). renderDropdownItems and MenuBottomSheetActionList both read a new opensSubMenu. A row with items opens a sub-menu, and with a renderable header or footer it stays one while items is empty, as compound DropdownMenuSubMenu does. Without either, an empty items is still a plain row.
Tests that type a filter matching nothing, each failing with the previous code:
- in feat(DropdownMenu): data items take a sub-menu's header and footer, and the bottom sheet shows a level's own #7839, the popover's dialog keeps its field focused with what was typed, and its footer;
- in feat(DropdownMenu): data items take a sub-menu's header and footer, and the bottom sheet shows a level's own #7839, the DropdownMenu sheet's row opens its level again after Back;
- here, the same for ContextMenu's bottom sheet.
FR20 and the docs say so.
3d7064b to
5c782b2
Compare
There was a problem hiding this comment.
Approval waits on a maintainer decision: component:ContextMenu FR12 is new and component:DropdownMenu FR20, its Sub-menu regions concept and DEC-13 are rewritten in draft records, and no engineering owner has approved that text on this head. A maintainer needs to approve it with an exact-head /approve-spec or say what must change, including FR20's switch from showing a ContextMenu sub-menu's rows alone to showing its header and footer. The problems raised on earlier heads are fixed here, including a data item whose filter matches nothing now keeping its level, and no other code change is needed. One point for that decision: the proposed text does not say how an empty filtered level is announced to screen-reader users.
[Automated review]
5c782b2 to
42522c6
Compare
… 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.
…et's levels Main added component:DropdownMenuSubMenu (meta#8227), whose Regions concept says that without a surface host the regions are not rendered. The bottom sheet renders no surface host, and with this pull request each of its levels shows its own row's header and footer in place of the menu's (proposed component:DropdownMenu FR23). The concept's invalid-value cell now says sheet levels still show them, a Sheet state names what a level shows, and the family list gives the bottom sheet and its levels to component:DropdownMenu. Record only.
There was a problem hiding this comment.
Approval waits on an owner decision about draft spec text: component:ContextMenu FR12 (new) and FR2, component:DropdownMenu FR20, FR23 (new), its Sub-menu regions row and DEC-13, and the component:DropdownMenuSubMenu Regions and Sheet rows are all draft, and no engineering owner has approved them on this head. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or says what must change, including whether the records should state how a sub-menu whose filter matches nothing is announced. No code change is needed: the problems raised on earlier heads are fixed and covered by tests, and the implementation matches the proposed text.
[Automated review]
… it (#7308) ## User impact On a phone, a `ContextMenu` a long press opens closes at once: as the finger lifts, Chromium sends the held element a compatibility `mousedown`, read as a press outside. ## Problem and solution fit - Why it occurs: the menu closes on any document `mousedown` outside it, and a touch's compatibility events target the held trigger. - Why it harms: no touch path. - Why it matters: iOS opens one only by a long press. ## Expected behavior and authority `spec:AST-094` (current) FR33: with a finger or pen, "the long press that opens a ContextMenu MUST NOT close it at the lift". `component:ContextMenu` FR5 routes the long-press lift there, and `architecture:interaction-modality` (current) INV4 asks for an operable path per modality. This PR implements that clause only; FR33's outside press (FR32), lift and handoff stay with their own work, and it changes no record row. #7303 proposes FR10 (focus on open); #7840, FR12. Overlap search over open PRs and records (`component:ContextMenu`, `layer-runtime.md`; "outside press", "long press", "compatibility"): #7303 and #7840 touch the same listener. ## Smallest restoration `main`'s outside `mousedown` listener stays, shadow-root handling included. It skips the compatibility mouse events of the finger press that opened the menu: the long press, or a touch `contextmenu` where Chromium sends one, recorded by the shared gesture counter (`Layer/gestureCounter`), which advances on every `pointerdown` and `keydown`. Any later press is a new gesture, so every other outside press keeps its outcome: a mouse or pen closes at the press, a tap on its compatibility `mousedown`, while a scroll, a cancelled or stopped `pointerdown`, an unpanned drag or a second finger leaves it open. A mouse or keyboard open records no gesture. No listener, API or default is added; `architecture:layer-runtime` names the skip. ## Evidence - Before, in real Chromium (CDP touch): the menu opens at about 510 ms; on lift come `pointerup`, `touchend`, then trusted `mousedown`, `mouseup` and `click` on the trigger, and it closes. - After: the menu stays open through that `click`, in a shadow root too; a tap or mouse click outside still closes it. - iOS Safari reportedly sends the `mousedown` mid-hold; the same gesture is skipped, but WebKit was not run here. - Regression proof: the long-press, touch `contextmenu` and moved-menu unit cases and both Chromium long-press cases fail on `main`; the mouse, pen and no-`mousedown` cases fail on the previous head and pass on `main`. ## Writing rubric results R1 narration: the layer-runtime line is present tense. R2 research: none. R3 evidence: none in the record; the tests below verify the line. R4 rejected alternatives: none. R5 change references: none. R6 open questions: none. ## Scope - [x] One defect is restored. - [x] Separable contradictory or unsettled tagalongs were removed or split. - [x] No new public API, visual representation, default, or interaction model is hidden under the bug-fix label. - [x] Public text and artifacts contain no internal Meta context. ## Testing - Maintainer merge 690291d brought in `main`: `ContextMenu.spec.md` takes `main`'s table, whose FR5 already routes the long-press lift to `spec:AST-094` FR33, so this PR no longer proposes FR11; the changeset names only the Chromium lift the tests show. The code, tests and `layer-runtime.md` line are unchanged. - Review round (163af16, rebased onto `main` 8b74911): narrowed to the long-press lift per the owner's requested change. `main`'s outside `mousedown` listener returns with one skip, the opening finger press's compatibility events (shared gesture counter); the pointer listeners, the nested-root levels and the five other outcome changes are gone. FR11, `layer-runtime.md` and the changeset say only that. ContextMenu, DropdownMenu, hooks, Layer and CLI suites: 1422 tests; long-press, shadow-root and ContextMenu audit Chromium specs: 23; native types, typecheck, `eslint` (strict), `check:knowledge` and the Storybook build pass. - Rebased onto `main` 39b5981 (486cec8), patch unchanged: the `typecheck` job's native platform types step runs a script `main` added in #8024, which 1fe5329's older base lacked. ContextMenu, DropdownMenu, hooks and CLI suites: 1216 tests; the long-press and shadow-root Chromium specs: 5; the native typecheck, build and platform-types steps, typecheck, `eslint` (strict), `check:knowledge` and the Storybook build pass. - Review round (1fe5329): the document `pointerup`/`pointercancel` listeners come on with an outside finger or pen press and off once none is live, instead of staying for the whole open menu (`architecture:react-component-runtime` INV5); `layer-runtime.md` says so. A new case shows none attached while the menu is open at rest and none after the lift; it fails on the previous head. ContextMenu, DropdownMenu, hooks and CLI suites: 1151 tests; the long-press and shadow-root Chromium specs: 5; typecheck, `eslint` (strict), `check:knowledge` and the Storybook build pass. ``` Rebased onto main 9f3ab05 (a5784a5, one commit): #7914 judged presses inside a shadow root with a `mousedown` listener on the menu's root, run before the document's. Capture reaches the document first, so each root from the document down to the menu's own now listens in the capture phase and defers a press on the host of the next root down; a press on such a host itself is judged by that host, whatever the host's own listeners do. #7914's open, closed and nested cases fire pointer events and also lift a finger on the menu; a new case stops presses in each root and on a host. They fail with the document judging every press (#7914's cases), with roots listening as the press bubbles, and with only the menu's own root listening (the new case). The `a11y/Menu press in a shadow root` stories gain a ContextMenu; the shadow-root spec checks in Chromium that a clicked item is picked (red with the document judging every press), that a press or a tap on the host closes the menu, and that its opening long press does not. `layer-runtime.md` names the extra listeners. vitest ContextMenu, DropdownMenu, Selector, hooks, Breadcrumbs Tests 1193 passed playwright MenuPressShadowRoot, ContextMenuLongPress, ContextMenuAudit --project=chromium 23 passed typecheck, lint:strict, check:knowledge, storybook:build ✓ Review round 3: rebased onto main 95e1674 as one commit; the rule is FR11, since #7303 proposes FR10 in the same record: vitest ContextMenu, DropdownMenu, useLongPress, docProp* Tests 376 passed playwright ContextMenu/__tests__ --project=chromium 23 passed typecheck, lint:strict, check:knowledge, storybook:build ✓ Review round 2: the listeners run in the capture phase, so a control that stops the press (ResizeHandle, Table's column-resize handle) still closes the menu; new case "closes on a press whose control stops it" fails with bubble-phase listeners: vitest ContextMenu, DropdownMenu, useLongPress, docProp* Tests 374 passed (DropdownMenuLazySheet's import hook timed out at 10 s under load; 2 passed alone) playwright ContextMenu/__tests__ --project=chromium 23 passed typecheck, lint:strict, check:knowledge, storybook:build ✓ Review round: the three changed presses are named (changeset, body, commit message), a new case pins the cancelled press, the unpanned drag and a second finger's tap (it fails on `main`), and docs/architecture/layer-runtime.md names the new listeners: vitest ContextMenu, DropdownMenu, useLongPress, docProp* Tests 375 passed playwright ContextMenu/__tests__ --project=chromium 23 passed typecheck, lint:strict, check:knowledge, storybook:build ✓ Rebased onto main 22c592a as one commit. Main's record now has FR9 (overlay dismissal), so this rule was proposed FR10 (now FR11, beside #7303's FR10): vitest ContextMenu, DropdownMenu, hooks, docProp* Tests 854 passed playwright ContextMenu/__tests__ --project=chromium 23 passed typecheck, lint:strict, check:knowledge, storybook:build ✓ red on main: `ContextMenuLongPress.a11y.chromium.spec.ts` against a `main` build: 1 failed (the long press and lift), 1 passed (the guard) pnpm exec vitest run packages/core/src/ContextMenu/ContextMenu.test.tsx -t "ContextMenu outside dismissal" main's ContextMenu.tsx: 3 failed, 3 passed (tap/scroll, mouse and mid-tap re-render guards) · with the change: 6 passed (the pen case fails on main, where a pen closes it at once; the re-render case failed on an earlier head) pnpm exec vitest run packages/core/src/ContextMenu packages/core/src/DropdownMenu packages/core/src/hooks/useLongPress.test.tsx \ packages/core/src/hooks/useMenuPress.test.tsx packages/core/src/docPropLiterals.test.ts \ packages/core/src/docPropReferences.test.ts --maxWorkers=8 Test Files 9 passed (9) Tests 324 passed (324) pnpm storybook:build (this branch; main for the red arm) ASTRYX_STORYBOOK_DIR=<main build> pnpm exec playwright test \ packages/core/src/ContextMenu/__tests__/ContextMenuLongPress.a11y.chromium.spec.ts --project=chromium 1 failed, 1 passed (long press: the menu is gone after the lift's click; mouse guard passes) pnpm exec playwright test packages/core/src/ContextMenu/__tests__/ContextMenuLongPress.a11y.chromium.spec.ts --project=chromium 2 passed · --repeat-each=2: 4 passed pnpm -F @astryxdesign/core typecheck, pnpm lint:strict, check:knowledge ✓ ```
Main's menu sheet renders nothing at rest and drops two wrappers (meta#8392): a sub-menu level's heading now sits in a row with its back button and the root's stands alone. This branch keeps its level regions on that structure: the opened level is still read from the current items, and its own header and footer, keyed by level, replace the menu's. ContextMenu's suite keeps this branch's plain-row case beside main's sheet-inset cases, which replace the old padding check.
…t level with its own, and the Sheet state names a data item's level Two wording points from the review on 5745647. The menu's own header and footer entries in DropdownMenu.doc.mjs, and FR15, still said the bottom sheet shows them at every level, while FR20 has a level's own regions replace both: the doc entries now say so, and FR15 says it with a pointer to FR20 (its closing sentence loses a restatement so the cell keeps its column width). The DropdownMenuSubMenu record's Sheet state now names a data item's level, since a compound menu never uses the bottom sheet. Record and doc wording only.
…xtMenu ContextMenu renders the same surface host beside its menu as DropdownMenu and provides the internal surface context, so a DropdownMenuSubMenu with header or footer gets its dialog outside every menu element there too, and the ContextMenu bottom sheet shows a data item's own regions. Both pass showsRegions to renderDropdownItems and MenuBottomSheetActionList, so a data item with header or footer stays a sub-menu while its items are empty, as in DropdownMenu. Proposed ContextMenu FR12, following DropdownMenu FR20 and DEC-13.
… drilled-in view is painted as the menu is The surface context is null in the bottom sheet, which renders no surface host, so a compound sub-menu with header or footer shows its rows alone there with the development warning, instead of its dialog inside the sheet's menu. The surface host carries the menu's context-menu theme target and the caller's xstyle, className and style, its own display rule last, so a drilled-in view keeps a themed or caller-styled menu's look. The docs list the header and footer item keys; ContextMenu FR12, DropdownMenu FR20 and the changeset say both. Two new cases fail with the previous ContextMenu.tsx and DropdownMenuSubMenu.tsx.
…u, and the menu tracks input modality ContextMenu dismisses on a document mousedown outside its menu. A sub-menu with header or footer renders its dialog in the surface host beside the menu, so a mouse press, or a touch tap's compatibility mousedown, on its field, button or rows closed the whole menu without acting. The check now counts the host as inside. ContextMenu also starts the shared interaction-modality tracker, as DropdownMenu does, so a drilled-in regions view opened by mouse reads pointer input and shows no keyboard focus ring. A user-event case clicking the field, the button and a row, a modality case in its own file (the tracker is document-wide once started) and a Chromium case on a new ContextMenu story fail with the previous ContextMenu.tsx.
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (meta#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.
…that carries the context-menu target With a sub-menu's drilled-in view showing in the Pointer menu surface's place, the surface host carries the context-menu target and the caller's styling. FR2, the Pointer menu surface's design row, the paragraph above the theming anatomy, the ownership boundary and FR2's verification row now say so, pointing at proposed FR12. The host is part of the Pointer menu surface rather than a new anatomy element, so the public anatomy and the theming map are unchanged. Record wording only; check:knowledge and prettier pass.
…ser's own menu The menu cancels contextmenu so a right-click on it opens no browser menu. A sub-menu's dialog renders in the surface host beside the menu, but React bubbles its events through the menu, so a right-click on a header field there lost the browser's own menu (paste, spelling), which the same field in a DropdownMenu keeps. The handler now cancels only events that begin inside the menu element. A case right-clicks the dialog's field (not cancelled) and a row (cancelled); it fails with the previous ContextMenu.tsx.
…ncelled The menu cancelled contextmenu only for events that began inside its own element, but a regioned sub-menu's rows render in its dialog in the surface host, outside that element, so a right-click on one opened the browser's menu over the app's. The handler now cancels any right-click inside a role="menu" element, the root's or a sub-menu's, while a field in a header or footer, outside every menu, keeps the browser's own menu. The case now also right-clicks a regioned sub-menu's row; it fails with the previous ContextMenu.tsx.
…enu's FR23 ContextMenu's sheet now finds its levels with the same path entries as DropdownMenu's, so each opened level is read from the current items while it shows, where main kept a snapshot from when it opened. FR12 now says its sheet levels follow component:DropdownMenu FR23 (the cell stays within its column), and its verification row names live levels. A case re-renders a ContextMenu sheet with a row added to an open level; it fails with the level read from its open-time row, as on main.
…surface host Main's component:DropdownMenuSubMenu (meta#8227) gives a sub-menu's regions to a DropdownMenu root and the surface host to component:DropdownMenu alone. With this pull request a ContextMenu renders the same host beside its menu (proposed component:ContextMenu FR12), so the Regions concept is available inside a DropdownMenu or ContextMenu root, the ownership boundary gives a ContextMenu's host to component:ContextMenu, and the family list names it. Record only.
81f58f0 to
f3c1494
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision about draft spec text: component:ContextMenu FR12 (new) and FR2, component:DropdownMenu FR15, FR20, FR23 (new), its Sub-menu regions row and DEC-13, and the component:DropdownMenuSubMenu Regions and Sheet rows are draft, and no engineering owner has approved them on this head. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or says what must change, for example whether a compound sub-menu in a ContextMenu bottom sheet should keep dropping its header and footer, and whether the records should say how a sub-menu whose filter matches nothing is announced. No code change is needed: the implementation matches the proposed text, and the issues raised on earlier heads are fixed and covered by tests.
[Automated review]
Main's meta#8515 added DropdownMenu FR21 (the menu grows out of its trigger under a finger) beside FR20; the requirements table keeps this branch's FR20 wording and FR23 row around it.
There was a problem hiding this comment.
Approval waits on an engineering-owner decision about draft spec text: component:ContextMenu FR12 (new) and FR2, component:DropdownMenu FR15, FR20, FR23 (new), its Sub-menu regions row and DEC-13, and the component:DropdownMenuSubMenu Regions and Sheet rows are draft, and no engineering owner has approved them on this head. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or says what must change, for example whether the surface host may carry the context-menu theme target while it owns no box, whether a compound sub-menu in a ContextMenu bottom sheet should keep dropping its header and footer, and how a sub-menu whose filter matches nothing is announced. No code change is needed: after the merge of main the implementation still matches the proposed text, and the issues raised on earlier heads stay fixed and covered by tests.
[Automated review]
No conflicts here; this keeps the ContextMenu sub-menu regions pull request, stacked on this one, reviewable against this branch after main's meta#8648 rewrote ContextMenu.spec.md.
…nu sub-menu regions Main's meta#8648 put ContextMenu on the public headless context menu (FR10) and rewrote the record's ownership lines. Her text is kept as merged; this branch adds back only its own surface-host wording (the ownership line, FR2 and its verification row) and proposed FR12 after FR10.
There was a problem hiding this comment.
Approval waits on an engineering-owner decision about draft spec text: component:ContextMenu FR12 (new) and FR2, component:DropdownMenu FR15, FR20, FR23 (new), its Sub-menu regions row and DEC-13, and the component:DropdownMenuSubMenu Regions and Sheet rows are draft, and no engineering owner has approved them on this head. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or says what must change, for example whether the surface host may carry the context-menu theme target while it owns no box, whether a compound sub-menu in a ContextMenu bottom sheet should keep dropping its header and footer, and how a sub-menu whose filter matches nothing is announced. No code change is needed: after the merge of main the implementation still matches the proposed text, and the issues raised on earlier heads stay fixed and covered by tests.
[Automated review]
|
I'm pausing this work for now to reduce contention on the repo. Thank you for the reviews and the spec guidance. |
User need
What led to it: this comes from moving every menu and picker in our app onto Astryx, which began with a press, slide and lift bug in its phone menus. Our rows' right-click menus reuse the same "Labels" sub-menu as the session menu, with its filter field and "Create label" button, and inside a
ContextMenuthose controls vanished.Why: a
DropdownMenuSubMenuwithheaderorfooter(#7530) shows its rows alone inside aContextMenu, with a development warning, becauseContextMenuhas no place outside its menu for the dialog. The same rows work in aDropdownMenu.Problem and solution fit
ContextMenunow renders the same surface host beside its menu asDropdownMenu, so a sub-menu with regions gets the same modal dialog: a flyout under a mouse, a drilled-in view on a phone. In its bottom sheet, a level whose data item has a header or footer shows them, as #7839 does forDropdownMenu.Current specification
component:ContextMenu(draft): this PR proposes FR12, which followscomponent:DropdownMenuFR20 and DEC-13; those now name either root as the one that renders the host.component:DropdownMenuSubMenu(draft) names that host too.Public delta and caller burden
ContextMenu, a sub-menu'sheaderandfooterrender, its row saysaria-haspopup="dialog", and the warning no longer fires.Design and alternatives
ContextMenu. No change, but the same sub-menu loses its controls depending on which menu holds it.ContextMenu. Possible, but two implementations of one behaviour.DropdownMenu's surface host (chosen). One behaviour for both roots. The cost:ContextMenumounts one more host element beside its menu.Evidence
New cases in
ContextMenu.test.tsxandContextMenuInteractionModality.test.tsxcover the dialog, its wiring and the sheet levels;SubMenuHeaderFooter.a11y.chromium.spec.tschecks them in Chromium. They fail on #7839's head.Writing rubric results
R1 narration: none.
R2 research: none.
R3 evidence: none.
R4 rejected alternatives: none.
R5 change references: none.
R6 open questions: none.
Evidence
See Testing.
Scope
Testing
In Storybook, open Core/ContextMenu → Submenu / header and footer.