Repository navigation
fix(DropdownMenu): a disabled trigger never opens the menu - #7475
Conversation
Button keeps an aria-disabled trigger focusable for its tooltip and swallows only Enter and Space, so ArrowDown, ArrowUp, a press-down or a click still reached the open path and opened a menu the trigger said was unavailable (component:MoreMenu AR1 / component:DropdownMenu AR4, APG menu button; AST-021 FR4). Every open path now checks the trigger first, for the default Button and for a natively or aria-disabled custom trigger. Red-armed test.
|
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.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMoreMenu (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression44 of 448 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 24 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.
The new guard in openAndFocus does keep a disabled trigger closed, and the new MoreMenu test reproduces the original bug, but two small changes are needed before this can be approved.
- A refused ArrowUp leaves
focusLastOnOpenRefset, so once the trigger is enabled a screen-reader activation or a programmaticisMenuOpenopen lands on the last row instead of the first; please clear the flag whenever an open is refused and add a test for that sequence. - The
:disabled, [aria-disabled="true"]branch forrenderTriggercontrols has no test, since removing it keeps the menu suites green, so please add a disabled custom-trigger case that covers ArrowDown, a click and a mouse press. - The changeset says every open path now checks availability, but the bottom-sheet presentation's click handler still opens from a caller-rendered
aria-disabledcontrol, so please narrow the wording or add the same check there.
[Automated review]
| button.isDisabled === true || | ||
| buttonRef.current?.matches(':disabled, [aria-disabled="true"]') === true | ||
| ) { | ||
| return false; |
There was a problem hiding this comment.
This early return leaves focusLastOnOpenRef set when the refused key was ArrowUp, because handleButtonKeyDown sets it before calling here. Once the trigger is enabled, the next open that does not set the flag again lands focus on the last row instead of the first. That includes an AT activation (a detail 0 click, which this component treats as a keyboard open) and a controlled isMenuOpen open. In jsdom: ArrowUp while disabled, then enable, then either open → "Last". Without the earlier ArrowUp → "First". Clearing the flag on refusal fixes it:
focusLastOnOpenRef.current = false;
return false;The existing rejected-show branch below has the same gap.
| // arrive here; a custom trigger may be natively disabled instead. | ||
| if ( | ||
| button.isDisabled === true || | ||
| buttonRef.current?.matches(':disabled, [aria-disabled="true"]') === true |
There was a problem hiding this comment.
This clause is the only thing protecting caller-rendered renderTrigger controls (and loading or group-disabled built-in triggers), but no test covers it. With this line deleted, all 344 tests in the DropdownMenu, MoreMenu, ContextMenu and NavMenu suites still pass. Please add a custom-trigger case, for example with the trigger below, asserting that ArrowDown, a click and a mouse press each leave aria-expanded="false".
renderTrigger={props => (
<button type="button" {...props} aria-disabled="true">
More
</button>
)}| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] `DropdownMenu` and `MoreMenu`: a disabled trigger no longer opens the menu. A disabled trigger with a tooltip stays focusable (`aria-disabled`), and Button swallowed only Enter and Space, so ArrowDown, ArrowUp, a press, or a click could still open the menu; every open path now checks the trigger's availability first. |
There was a problem hiding this comment.
"every open path now checks the trigger's availability first" covers only the anchored menu. In the bottom-sheet presentation the trigger opens through handleSheetTriggerClick, which has no availability check, so a caller-rendered aria-disabled control still opens the sheet. The built-in disabled trigger stays closed there only because Button's click guard runs first. Please narrow the wording, or add the same check to the sheet's click handler.
…e bottom-sheet trigger click also refuses a disabled control Red-armed: a synthesized click after a refused ArrowUp now lands on the first row, and a disabled custom trigger stays closed on ArrowDown, a press and a click.
There was a problem hiding this comment.
The disabled-trigger fix now holds on every anchored open path, and the earlier feedback on the ArrowUp landing and the custom-trigger test is addressed, but one small change is needed before approval. The new availability check in the bottom-sheet trigger's click handler has no regression test, since removing it keeps every suite green, so please add a presentation="bottom-sheet" case with an aria-disabled renderTrigger control and assert that a click and Enter leave aria-expanded="false".
- Optional: the
button.isDisabledclause in both guards is already covered by the DOM check for the built-in trigger, and whenbuttonandrenderTriggerare both given it disables the caller's enabled control, which should win, so consider dropping it.
[Automated review]
| if ( | ||
| event.currentTarget.matches(':disabled, [aria-disabled="true"]') || | ||
| button.isDisabled === true | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This guard fixes the bottom sheet for a disabled caller control, but no test covers it: removing it keeps every suite green. Please add a bottom-sheet case to the custom-trigger suite, for example:
render(
<DropdownMenu
presentation="bottom-sheet"
items={[{label: 'Edit'}, {label: 'Delete'}]}
renderTrigger={({ref, ...props}) => (
<button type="button" ref={ref} aria-disabled="true" {...props}>
Row actions
</button>
)}
/>,
);Then assert that a click and Enter on the trigger each leave aria-expanded="false".
| // Enter and Space, so the arrow keys, a press and a click would still | ||
| // arrive here; a custom trigger may be natively disabled instead. | ||
| if ( | ||
| button.isDisabled === true || |
There was a problem hiding this comment.
Optional: the selector on the next line already matches the built-in Button's disabled or aria-disabled state, so every menu suite stays green without this clause. Keeping it also stops an enabled renderTrigger control from opening the menu when button is passed alongside it with isDisabled, although the caller's control is meant to win. Consider dropping it here and in the bottom-sheet guard.
…st; the DOM availability check alone guards both paths The button.isDisabled clause is dropped: the DOM check already covers the built-in Button, and when both button and renderTrigger are given the caller's enabled control wins. Red-armed: the bottom-sheet test fails with the sheet click guard removed.
There was a problem hiding this comment.
This fix is ready: a disabled DropdownMenu or MoreMenu trigger now stays closed on every key, press and click, in both the anchored menu and the bottom sheet, and the feedback from earlier revisions is addressed. The new tests fail against the previous implementation, each guard and the ArrowUp reset has its own covering test, and CI is green. One optional cleanup: in “a disabled custom trigger never opens the bottom sheet”, the queryByRole('dialog', {name: 'Project actions'}) check can never fail because the sheet is named “Menu”, so it can be dropped or pointed at that name.
[Automated review]
| expect( | ||
| screen.queryByRole('dialog', {name: 'Project actions'}), | ||
| ).not.toBeInTheDocument(); |
There was a problem hiding this comment.
Optional: this assertion cannot fail. With only renderTrigger given, the sheet takes its accessible name from the default button label (“Menu”), so no dialog is ever named “Project actions”. The two aria-expanded checks above already prove the sheet stays closed; drop this or match the sheet's real name.
This branch was successfully deployed
1 participant
Problem
A keyboard user on a disabled MoreMenu (or any DropdownMenu whose trigger is
aria-disabled— Button keeps a disabled trigger with a tooltip focusable) heard the control was unavailable, pressed ArrowDown, and the menu opened anyway. Button swallows Enter and Space for an aria-disabled control (Button.tsx L645–654) but lets other keys through, and DropdownMenu's trigger handler opens on ArrowDown/ArrowUp (L1317–1332); the pointer press model and a click could do the same. The Menu Button accessibility contract (#7469) records this as the one known failure ofmenu-button.control.unavailable-stays-closed;component:MoreMenuAR1 /component:DropdownMenuAR4 (#7459) govern the trigger, andspec:AST-021FR4 asks an unavailable control to stay unavailable.Fix
openAndFocus— the single path every open goes through (keyboard, pointer press, click, mounted-open walk-in) — returnsfalsewhen the trigger is unavailable:button.isDisabled, or a customrenderTriggercontrol matching:disabled, [aria-disabled="true"]. Nothing else changes; an enabled trigger behaves exactly as before.Round-2 changes
focusLastOnOpenRef, so a later screen-reader activation (synthesized click,detail === 0) or programmatic open lands on the first row, not the last. Test: "does not carry a refused ArrowUp into a later open" (red on the previous head).renderTriggerbranch is covered: "a disabled custom trigger never opens the menu" (aria-disabled custom button; ArrowDown, a mouse press, a click — red on main).Evidence
MoreMenu.test.tsx"keeps the menu closed from every opening key and press while disabled" — Enter, Space, ArrowDown, ArrowUp and a mouse press on the disabled trigger leavearia-expanded="false". Red arm: fails withDropdownMenu.tsxreverted to main (ArrowDown opened it); 260/260 across the DropdownMenu and MoreMenu suites with the fix (ContextMenu and NavMenu suites unchanged).more-menu-disabledstate passes and its known-failure record is removed in the contract PR's follow-up.Changeset
@astryxdesign/corepatch[fix].