Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[useButton] Fix focusableWhenDisabled components #1313

Merged
merged 11 commits into from
Jan 10, 2025
Merged

[useButton] Fix focusableWhenDisabled components #1313

merged 11 commits into from
Jan 10, 2025

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Jan 9, 2025 •
edited
Loading

Using mergeReactProps, our event handlers can't (easily) prevent external handlers since external ones are called first, it seems best to just bypass mergeReactProps here and manually call the external onClick.

Additionally found these issues along the way and fixed them:

  • <Menu.RadioGroup disabled /> didn't work
  • <Select.Trigger disabled /> didn't work

Closes #1312

@mj12albert mj12albert added the bug 🐛 Something doesn't work label Jan 9, 2025
@mj12albert mj12albert changed the title Fix focusableWhenDisabled [useButton] Fix focusableWhenDisabled Jan 9, 2025
@netlify Netlify
Copy link

netlify bot commented Jan 9, 2025 •
edited
Loading

✅ Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 52838ad
🔍 Latest deploy log https://app.netlify.com/sites/base-ui/deploys/677fe102eb5c0c00080bd422
😎 Deploy Preview https://deploy-preview-1313--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mj12albert mj12albert force-pushed the fix/use-button-focusable-when-disabled branch from d006ad4 to eb2738a Compare January 9, 2025 06:32
@mj12albert mj12albert force-pushed the fix/use-button-focusable-when-disabled branch from eb2738a to a51cd51 Compare January 9, 2025 06:42
@mj12albert mj12albert marked this pull request as ready for review January 9, 2025 06:48
@mj12albert
Copy link
Member Author

mj12albert commented Jan 9, 2025 •
edited
Loading

<Select.Trigger disabled /> doesn't seem to work, though it's not related to focusableWhenDisabled

atomiks
atomiks reviewed Jan 9, 2025
View reviewed changes
packages/react/src/select/item/SelectItem.test.tsx Outdated
Comment on lines 138 to 141
const item = screen.getByText('two');
act(() => item.focus());
expect(item).toHaveFocus();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Select should now follow Menu with useListNavigation's disabledIndices prop

@atomiks
Copy link
Contributor

atomiks commented Jan 9, 2025

<Select.Trigger disabled /> doesn't seem to work, though it's not related to focusableWhenDisabled

I think this could also be fixed in this PR. The interaction hooks like useClick only check for readOnly for the enabled state, but not disabled like Menu does

@atomiks atomiks mentioned this pull request Jan 9, 2025
Merged
@mj12albert mj12albert force-pushed the fix/use-button-focusable-when-disabled branch from b9f2cbb to 04fa81c Compare January 9, 2025 10:59
mj12albert
mj12albert commented Jan 9, 2025
View reviewed changes
packages/react/src/menu/radio-group/MenuRadioGroup.tsx
*
* @default false
*/
disabled?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added back <Menu.RadioGroup disabled /> for consistency with regular RadioGroup

@mj12albert mj12albert force-pushed the fix/use-button-focusable-when-disabled branch from dbd21fc to 38e5ab6 Compare January 9, 2025 11:39
@mj12albert mj12albert force-pushed the fix/use-button-focusable-when-disabled branch from 38e5ab6 to b5ac66d Compare January 9, 2025 13:11
@mj12albert mj12albert added component: select This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module! labels Jan 9, 2025
@mj12albert mj12albert added component: tabs This is the name of the generic UI component, not the React module! component: collapsible This is the name of the generic UI component, not the React module! labels Jan 9, 2025
@mj12albert
Copy link
Member Author

The interaction hooks like useClick only check for readOnly for the enabled state, but not disabled like Menu does

Fixed, it seems ok to have listNavigation and typeahead ignore a disabled trigger (not disabled Select.Root)

@mj12albert mj12albert force-pushed the fix/use-button-focusable-when-disabled branch from b5ac66d to 6665fc1 Compare January 9, 2025 13:17
@mj12albert mj12albert changed the title [useButton] Fix focusableWhenDisabled [useButton] Fix focusableWhenDisabled components Jan 9, 2025
michaldudak
michaldudak reviewed Jan 9, 2025
View reviewed changes
packages/react/src/select/item/SelectItem.test.tsx
@@ -117,6 +117,29 @@ describe('<Select.Item />', () => {
});
});

it('should focus disabled items', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that hovering over disabled options doesn't focus them (unlike menu items)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that SelectItem inlines pointer-events: 'none' when disabled which causes this, is it safe to handle it in the handlers instead like this @atomiks ? 52838ad

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mj12albert yes, should be

@mj12albert mj12albert requested a review from michaldudak January 9, 2025 14:54
@mj12albert mj12albert merged commit 47bf76d into mui:master Jan 10, 2025
23 checks passed
@mj12albert mj12albert deleted the fix/use-button-focusable-when-disabled branch January 17, 2025 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atomiks atomiks atomiks approved these changes

@colmtuite colmtuite Awaiting requested review from colmtuite colmtuite is a code owner

@michaldudak michaldudak Awaiting requested review from michaldudak michaldudak is a code owner

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: collapsible This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module! component: select This is the name of the generic UI component, not the React module! component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[useButton] When focusableWhenDisabled the button can still be clicked or pressed
3 participants
@mj12albert @atomiks @michaldudak