Repository navigation
fix(DropdownMenu): a disabled trigger never opens the menu #7475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
642b9ca
036a208
4cb0a80
935c63d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@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 anchored menu; the anchored open path and the bottom-sheet trigger's click now check the trigger's availability first, for the default Button and for a natively or `aria-disabled` custom trigger. | ||
|
|
||
| @cixzhang |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -716,7 +716,15 @@ function DropdownMenuBottomSheet({ | |
| openModalityRef.current = 'keyboard'; | ||
| } | ||
| }; | ||
| const handleSheetTriggerClick = () => { | ||
| const handleSheetTriggerClick = (event: React.MouseEvent<HTMLElement>) => { | ||
| // A caller-rendered trigger that is natively or aria-disabled must not | ||
| // open the sheet; the default Button already refuses its own click. | ||
| if ( | ||
| event.currentTarget.matches(':disabled, [aria-disabled="true"]') || | ||
| button.isDisabled === true | ||
| ) { | ||
| return; | ||
| } | ||
| onClick?.(); | ||
| setOpen(!isOpen); | ||
| }; | ||
|
|
@@ -1190,6 +1198,16 @@ function DropdownMenuPopover({ | |
| modality: 'keyboard' | 'pointer' = 'keyboard', | ||
| notifyClick = false, | ||
| ): boolean => { | ||
| // An unavailable trigger never opens the menu. Button keeps an | ||
| // aria-disabled trigger focusable for its tooltip and swallows only | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: the selector on the next line already matches the built-in Button's |
||
| buttonRef.current?.matches(':disabled, [aria-disabled="true"]') === true | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This clause is the only thing protecting caller-rendered renderTrigger={props => (
<button type="button" {...props} aria-disabled="true">
More
</button>
)} |
||
| ) { | ||
| return false; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This early return leaves focusLastOnOpenRef.current = false;
return false;The existing rejected-show branch below has the same gap. |
||
| } | ||
| acceptedOpenRef.current = false; | ||
| notifyClickOnShowRef.current = notifyClick; | ||
| pendingControlledOpenRef.current = isControlled; | ||
|
|
@@ -1205,7 +1223,7 @@ function DropdownMenuPopover({ | |
| } | ||
| return true; | ||
| }, | ||
| [isControlled, popover], | ||
| [button.isDisabled, isControlled, popover], | ||
| ); | ||
|
|
||
| const isMenuOpenNow = isControlled | ||
|
|
@@ -1328,7 +1346,11 @@ function DropdownMenuPopover({ | |
| return; | ||
| } | ||
| focusLastOnOpenRef.current = e.key === 'ArrowUp'; | ||
| openAndFocus(); | ||
| if (!openAndFocus()) { | ||
| // A refused open (disabled trigger) must not leave ArrowUp's | ||
| // "land on the last row" armed for a later open. | ||
| focusLastOnOpenRef.current = false; | ||
| } | ||
| } | ||
| return; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Then assert that a click and Enter on the trigger each leave
aria-expanded="false".