Repository navigation
fix(Overlay, DropdownMenuGroup, MobileNavToggle): forward host attributes to the element each owns - #8755
fix(Overlay, DropdownMenuGroup, MobileNavToggle): forward host attributes to the element each owns#8755kentonquatman wants to merge 4 commits into
Conversation
|
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:code-review
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
Modified ComponentsOverlay (@astryxdesign/core)
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 Regression2 of 10 shot(s) changed. A change here is a question, not a failure: check whether the after is the
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.
This looks ready to approve: Overlay and DropdownMenuGroup now forward host attributes and DOM handlers as architecture:public-component-api INV5 requires, while their own role, group name, ref and theme target stay component-controlled. The Overlay touch toggle still runs ahead of a caller onClick, so a caller cannot cancel it, which matches INV7. The five new tests fail without the fix and pass with it; once the pull request is marked ready for review, the required code review can proceed.
[Automated review]
github-actions
Bot
removed
the
needs:code-review
…element Overlay and DropdownMenuGroup each own one stable DOM element but typed their props as a Pick of the three styling props, so data-*, id, aria-* and DOM handlers passed to them never reached the page. Both now extend BaseProps and spread the remaining props onto that element, as other DOM-owning components do (architecture:public-component-api INV5). Overlay runs its touch toggle before a caller onClick or onMouseUp, so a caller handler cannot cancel it. DropdownMenuGroup keeps role="group" and, while it has a title, keeps that title as its name; an untitled group takes a caller's aria-labelledby.
MobileNavToggle owns one button but typed its props as a Pick of the three styling props plus data-testid, so other data-*, id, aria-* and DOM handlers never reached the page. It now extends BaseProps for the button, minus the aria-expanded and aria-controls it owns, and spreads the rest onto the Button before its own props (architecture:public-component-api INV5). The drawer toggle runs before a caller onClick, so a caller cannot cancel it (INV7). The changeset now covers all three components.
6adfa0d to
82b5b87
Compare
kentonquatman
changed the title
github-actions
Bot
added
the
needs:code-review
There was a problem hiding this comment.
This looks ready to approve: Overlay, DropdownMenuGroup and MobileNavToggle now forward host attributes and DOM handlers to the element each owns, as architecture:public-component-api INV5 requires, while their own role, group name, aria-expanded/aria-controls, ref and theme target stay component-controlled. The built-in touch and drawer toggles still run ahead of a caller onClick, which matches INV7, and the new tests fail without the fix (or with the spread or handler order reversed) and pass with it. One optional wording fix: the changeset says a test id passed to any of the three never reached the page, but MobileNavToggle already forwarded data-testid before this change. The required code review can proceed.
[Automated review]
github-actions
Bot
removed
the
needs:code-review
The release note said a test id passed to any of the three components never reached the page. MobileNavToggle already forwarded data-testid; it gains every other data-*, id, aria-* and handler.
|
Visual regression review (run 40622363152): the two changed shots, AppShell Playground in neutral light and dark, don't come from this PR. The committed baseline was captured on 2026-10-15, before #8311 made AppShell stories render edge to edge, and the after is that frame: no 16 px gutter, so the shot is 1024×768 instead of 1024×800. At 1024 px AppShell uses its desktop layout, where |
There was a problem hiding this comment.
This looks ready to approve: Overlay, DropdownMenuGroup and MobileNavToggle now forward host attributes and DOM handlers to the element each owns, as architecture:public-component-api INV5 requires, while their own role, group name, aria-expanded/aria-controls, ref and theme target stay component-controlled. The built-in touch and drawer toggles still run ahead of a caller onClick, which matches INV7, and the new tests fail without the fix (or with the spread or handler order reversed) and pass with it. One behavior worth knowing, not a blocker: a caller onClick on Overlay is skipped for clicks a descendant already prevented (for example a client-side router link in content), because that is how the shared composeEventHandlers composes handlers.
[Automated review]
There was a problem hiding this comment.
This looks ready to approve: Overlay, DropdownMenuGroup and MobileNavToggle now forward host attributes and DOM handlers to the element each one owns, as architecture:public-component-api INV5 requires, while their own role, title name, aria-expanded/aria-controls, ref and built-in toggles stay component-controlled (INV7). The new tests fail without the fix or with the spread or handler order reversed, and pass with it. Two optional follow-ups, neither blocking:
- A caller
onClickonOverlayis skipped for a click a descendant already prevented (a client-routed link incontent, for example), becausecomposeEventHandlersstops atdefaultPrevented;chainEventHandlerswould deliver every click while keeping the touch toggle uncancellable, if that delivery is preferred. - The
MobileNavToggledoc line could add that the button's accessible name stayslabel, since a calleraria-labelis replaced ascomponent:ButtonORD4 requires.
[Automated review]
| // The touch toggle runs before the caller's handler: Overlay promises no | ||
| // cancellation, so a caller preventDefault() cannot stop it | ||
| // (architecture:public-component-api INV7). | ||
| onClick={composeEventHandlers( |
There was a problem hiding this comment.
Non-blocking: composeEventHandlers returns once event.defaultPrevented is set, so this caller onClick is skipped for any click a descendant already prevented (a client-routed link in content, for example), also on desktop where the touch handler is undefined. A wrapping div would still receive those clicks. If DOM-like delivery is preferred, chainEventHandlers(overlay.containerProps.onClick, onClickProp) keeps the toggle first and uncancellable while the caller sees every click.
| displayName: 'Mobile Nav Toggle', | ||
| description: | ||
| 'Hamburger button that opens/closes the mobile nav drawer. Reads open state from AppShell context automatically: does NOT accept isOpen or onOpenChange props. Renders nothing above the mobile breakpoint.', | ||
| 'Hamburger button that opens/closes the mobile nav drawer. Reads open state from AppShell context automatically: does NOT accept isOpen or onOpenChange props. Renders nothing above the mobile breakpoint. Host attributes and DOM handlers (id, data-*, aria-*) reach the button; its aria-expanded and aria-controls stay component-owned, and a caller onClick runs after the toggle.', |
There was a problem hiding this comment.
Optional wording: a caller aria-label does not reach the button, because Button replaces it with label (component:Button ORD4). Saying the accessible name comes from label, beside aria-expanded and aria-controls, would keep this list exact; the dense line at 308 has the same gap.
User impact
Builders attach
data-*attributes to components for test selectors, analytics, and visual-editing or inspection tools.OverlayandDropdownMenuGroupdrop them, andMobileNavTogglekeeps onlydata-testid. Each accepts onlyxstyle,className,styleandrefin its props type (plusdata-testidonMobileNavToggle), but TypeScript accepts any hyphenated attribute on a JSX element, so<Overlay data-testid="hero">and<DropdownMenuGroup data-analytics-id="history">type-check and then never reach the DOM. Nothing warns. A test query or analytics selector finds nothing, and an app that wraps these components and forwards caller props loses them. The only workaround is a ref callback that writes attributes imperatively. The same applies toid,aria-*and DOM handlers, which the types reject outright.After this change, those attributes and handlers reach the element each component already owns: the
Overlayroot that carries its ref, theme target and styling props, therole="group"element ofDropdownMenuGroup, and the buttonMobileNavTogglerenders.Problem and solution fit
Pickof the three styling props, and both components destructure a closed set and spread nothing onto their root.SideNavCollapseButton, the other AppShell toggle, already takes the fullBasePropssurface).The fix gives each component the host surface other DOM-owning components already have: extend
BasePropsfor the owned element and spread the remaining props onto it, with component-owned semantics set after the spread.Expected behavior and authority
architecture:public-component-api(current), INV5: for a DOM-owning component, supported DOM, data, ARIA, style, class and event inputs reach the element that owns the contract, and component-owned accessibility and behavior cannot be accidentally overwritten. Its Owning code section:BasePropsdefines the shared DOM and styling surface for DOM-owning components. INV7: event handlers compose deliberately, and a consumer can cancel built-in behavior only where the public API promises cancellation.docs/contributing/api-conventions.md(DOM props, styling, and refs), which projects INV5: typeBasePropswith the contract element, omit names that collide with component concepts, forward neutraldata-*, ARIA, DOM and event props, and keep owned role and accessibility from being overwritten.Overlayits root container (refand theoverlaytheme target land there),DropdownMenuGroupitsrole="group"element (reflands there),MobileNavToggleits button (ref, styling anddata-testidland there).component:Overlay(current) governsOverlayRegionplacement only and leaves Overlay's root surface to source, so INV5 is the governing rule for these props.DropdownMenuGrouphas no current component record.component:MobileNav(current) listsarchitecture:public-component-apiINV5 and INV7 among its governing rules. Its FR11 owns what the toggle does (renders only in an enabled AppShell mobile layout, icon-only ghost Button, toggles the drawer,aria-expanded,aria-controls), and all of that is unchanged. Its contract-at-a-glance row ("MobileNavTogglewithlabelandchildren") is a review projection that already omits theref, styling props anddata-testidthe toggle accepts, and no requirement says the toggle drops host attributes, so the record needs no edit.DropdownMenuDivider), feat(TabMenu): forward host attributes and DOM handlers to the trigger #7295 (TabMenu), feat(DropdownMenuSubMenu): forward host attributes, DOM handlers and a ref to the trigger row #7318 (DropdownMenuSubMenu) and feat(DropdownMenuItem): forward host attributes and DOM handlers to the row #7196 (DropdownMenuItem).Smallest restoration
OverlayPropsextendsBaseProps<HTMLDivElement>instead of the three-propPick; the remaining props spread onto the root before its ref, theme target and merged styling. The touch tap-to-toggle handlers run first and a calleronClickoronMouseUpruns after them, so a callerpreventDefault()cannot cancel the toggle (Overlay promises no cancellation).DropdownMenuGroupPropsextendsOmit<BaseProps<HTMLDivElement>, 'role'>; the remaining props spread onto the group before its ownedrole="group". While the group has atitle, the heading keeps naming it; an untitled group passes a calleraria-labelledbythrough instead of clearing it.MobileNavTogglePropsextendsOmit<BaseProps<HTMLButtonElement>, 'aria-expanded' | 'aria-controls'>; the remaining props spread onto the Button before its own variant, label, icon, ARIA wiring, defaultdata-testidand styling. The drawer toggle runs first and a calleronClickruns after it, as inTabMenu, so a caller cannot cancel it. The accessible name stayslabel(Button setsaria-labelfrom it after caller props).{type: 'section'}path, or when the toggle renders.Evidence
Reproduction before the change: with the three source files reverted to
main, eight of the nine new tests fail; no element carries the forwarded attribute or runs the caller handler. The ninth (MobileNavTogglekeeps its ownaria-expandedandaria-controls) guards the spread order and passes onmainbecause nothing was forwarded.Result after the change: all pass. Attributes reach the
Overlayroot (the same node as its ref, keepingastryx-overlay), theDropdownMenuGroupelement (a callerroleis rejected by the type and overridden at runtime; the title still names the group), and theMobileNavTogglebutton (a calleraria-expandedoraria-controlsis overridden, and the drawer still opens when a calleronClickcallspreventDefault()).Representative unchanged path: the existing Overlay rendering, position, scrim, controlled visibility, styling, ref and touch-toggle suites; the DropdownMenu, ContextMenu, Breadcrumbs and MoreMenu suites; and the MobileNav, AppShell and TopNav suites, including the toggle's existing ARIA wiring tests.
Regression test or other mutation-sensitive proof: each guard was removed in turn and the suite rerun:
{...rest}onClick, calleronMouseUponClickfirst (cancellable)onClickstill toggles on taponClicknot composedonClickstill toggles on taponMouseUpnot composedonMouseUp{...rest}aria-labelledbydropped when untitled{...rest}after the owned rolepresentation){...rest}{...rest}after the owned ARIAaria-expandedandaria-controlsonClickfirst (cancellable)onClickruns after the toggleonClicknot composedonClickruns after the toggleScope
BasePropssurface INV5 and its Owning code section name for DOM-owning components; nothing component-specific is added.Public delta.
OverlayPropschanges fromPick<BaseProps<HTMLDivElement>, 'xstyle' | 'className' | 'style'>toBaseProps<HTMLDivElement>.DropdownMenuGroupPropschanges fromPick<BaseProps, 'xstyle' | 'className' | 'style'>toOmit<BaseProps<HTMLDivElement>, 'role'>.MobileNavTogglePropschanges fromPick<BaseProps, 'xstyle' | 'className' | 'style'>toOmit<BaseProps<HTMLButtonElement>, 'aria-expanded' | 'aria-controls'>. Every existing prop keeps its name, type and default, nothing becomes required, and no export changes. Observable change: host attributes and handlers a caller passes now reach the owned element, with the owned behavior listed above unchanged.How the components were chosen: a runtime check on current
main(render each candidate with adata-*attribute, look for it in the document) and a static pass over every exported component whose props reachBaseProps. Most components that dropped caller attributes in 0.6.x already forward them onmain(#7331, #7339, #7645 and others). The ones that still drop them, and why the rest stay out of this PR:Tooltip,Popover,HoverCard: no stable element of their own. The trigger is the caller's child, which takes its own attributes, and the surface mounts only while open, in more than one presentation. Choosing a host for them is an API decision, not a restoration.Theme,LayerProvider: providers that coordinate children; their props do not includeBaseProps.MoreMenu:component:MoreMenu(current) FR4 lists exactly what it forwards to DropdownMenu, includingdata-testidonly. Widening that is a change to the record.PowerSearch:component:PowerSearch(current) records the forwarding ofdata-*other thandata-testidas a known gap left to a separate stable decision.Open PRs that touch the same files without overlapping this fix: #7952 (
Overlay.tsx, OverlayRegion), #7535 (DropdownMenuGroup.tsx, row gap) and #7358 (MobileNavToggle.tsx, i18n slices). Any of them may need a trivial rebase after the other lands.Why this is a patch
Under
spec:AST-017FR1 and CONTRIBUTING's Changeset policy, a change is breaking only when a valid consumer of the latest stable release must change source or configuration to keep a supported contract working, or loses a stable public surface. FR5 makes broader accepted inputs nonbreaking when old usage is unchanged, and FR32 makes a correction breaking only when a valid released consumer relies on the corrected behavior. The concrete ways this could affect an existing consumer:id,role,tabIndex,hidden,inertand handlers are compile errors on all three components, so no typed callsite passes them. Only hyphenated names (data-*,aria-*) slip through, because TypeScript does not check them against a component's props. The released types and docs never accepted them, and FR3 does not make undocumented markup a contract. Writingaria-hiddenordata-*on one of these components while relying on it being ignored is not a supported scenario. A wrapper that spreads a wider object (which TypeScript does not excess-check) could now see an extraidor a secondonClickon the owned element. That path bypasses the released types, and the same exposure existed for every precedent below.Overlay's ref, theme class, merged styling and touch toggle;DropdownMenuGroup's role, its title as name, and merged styling;MobileNavToggle's variant, accessible name,aria-expanded,aria-controls, default test id, styling and drawer toggle. The tests and mutations above pin each of these.titleandcontentstay the components' ownReactNodeprops becauseBasePropsalready omits those HTML names. One compile-time edge remains: a consumer interface thatextendsone of these props types and declares a member with a native attribute name but an incompatible type (for example its ownonChange) would now fail to compile. The repository has consistently treated aPicktoBasePropswidening as additive and released it as a patch: fix(DropdownMenuDivider): forward host attributes to the separator #7645 ([fix]), feat(TabMenu): forward host attributes and DOM handlers to the trigger #7295, feat(DropdownMenuSubMenu): forward host attributes, DOM handlers and a ref to the trigger row #7318 and feat(DropdownMenuItem): forward host attributes and DOM handlers to the row #7196 ([feat]).#7331 was released as
[breaking]for a different reason. Its four components already declared the fullBasePropssurface, so typed callers could legally passhidden,inert,roleor handlers that were then ignored, and applying them changed typed, supported callsites. That does not happen here.Testing
All pass (819 ui tests in 33 files, 199 node tests in 12 files). DOM attribute and handler forwarding only; no layout, pixel or focus dependence. The touch branch is exercised the way the existing Overlay touch tests do it, by stubbing
(hover: none), and the toggle the way the existing MobileNavToggle tests do it, below the AppShell breakpoint.Before marking ready
spec-owner-approvalgate does not apply. If a reviewer preferscomponent:MobileNav's contract-at-a-glance row to name the toggle's host attributes explicitly, that is a one-line edit to a current record and needs approval from its owner.