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

[Checkbox, Menu, Switch] Audit focus states on disabled components #734

Merged
merged 7 commits into from
Oct 28, 2024
Merged

[Checkbox, Menu, Switch] Audit focus states on disabled components #734

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dd6b148
Partial changes for issue #656
hakim-toptal Oct 13, 2024
cc20541
Revert "Partial changes for issue #656"
onehanddev Oct 13, 2024
55f8f07
Reapply "Partial changes for issue #656"
onehanddev Oct 13, 2024
e5461c3
lint fixes
onehanddev Oct 13, 2024
de53105
Refactor: Set 'disabled' via useCheckboxRoot's getButtonProps and rem…
onehanddev Oct 23, 2024
8ce05b3
Added comma as before while reverting changes in CheckboxRoot
onehanddev Oct 23, 2024
238a110
Updated unit tests to expect disabled attribute instead of aria-disab…
onehanddev Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion docs/data/api/menu-trigger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"props": {
"className": { "type": { "name": "union", "description": "func<br>&#124;&nbsp;string" } },
"disabled": { "type": { "name": "bool" }, "default": "false" },
"focusableWhenDisabled": { "type": { "name": "bool" }, "default": "false" },
"label": { "type": { "name": "string" } },
"render": { "type": { "name": "union", "description": "element<br>&#124;&nbsp;func" } }
},
Expand Down
3 changes: 0 additions & 3 deletions docs/data/translations/api-docs/menu-trigger/menu-trigger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
"description": "Class names applied to the element or a function that returns them based on the component&#39;s state."
},
"disabled": { "description": "If <code>true</code>, the component is disabled." },
"focusableWhenDisabled": {
"description": "If <code>true</code>, allows a disabled button to receive focus."
},
"label": { "description": "Label of the button" },
"render": { "description": "A function to customize rendering of the component." }
},
Expand Down
1 change: 1 addition & 0 deletions packages/mui-base/src/Checkbox/Root/CheckboxRoot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ const CheckboxRoot = React.forwardRef(function CheckboxRoot(
extraProps: {
...otherProps,
...otherGroupProps,
disabled,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing disabled here, it should be set by useCheckboxRoot's getButtonProps. You can remove aria-disabled and have just disabled prop returned from there.
Same with the Switch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out! 🙏 I’ve updated the code.
Please let me know if there are any further changes needed. 😊

Copy link
Contributor Author

@onehanddev onehanddev Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It appears that removing aria-disabled is causing CircleCI’s browser tests to fail. Do you think we should reintroduce aria-disabled alongside the disabled prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the various a11y resources, we are not required to use aria-disabled if we already are using native form elements like button with appropriate roles like switch or checkbox. what do you think @michaldudak ?

Copy link
Member

Choose a reason for hiding this comment

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

No, having just the disabled attribute is enough. Using both disabled and aria-disabled is redundant and may trigger warnings in some a11y validators.
IMO the way to go would be updating the tests to expect the disabled attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaldudak I have fixed the tests, Please review it again. Thanks !

},
});

Expand Down
10 changes: 0 additions & 10 deletions packages/mui-base/src/Menu/Trigger/MenuTrigger.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,6 @@ namespace MenuTrigger {
* @default false
*/
disabled?: boolean;
/**
* If `true`, allows a disabled button to receive focus.
* @default false
*/
focusableWhenDisabled?: boolean;
/**
* Label of the button
*/
Expand Down Expand Up @@ -103,11 +98,6 @@ MenuTrigger.propTypes /* remove-proptypes */ = {
* @default false
*/
disabled: PropTypes.bool,
/**
* If `true`, allows a disabled button to receive focus.
* @default false
*/
focusableWhenDisabled: PropTypes.bool,
/**
* Label of the button
*/
Expand Down
1 change: 0 additions & 1 deletion packages/mui-base/src/Menu/Trigger/useMenuTrigger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ export function useMenuTrigger(parameters: useMenuTrigger.Parameters): useMenuTr

const { getButtonProps, buttonRef } = useButton({
disabled,
focusableWhenDisabled: false,
buttonRef: mergedRef,
});

Expand Down
2 changes: 1 addition & 1 deletion packages/mui-base/src/Switch/Root/SwitchRoot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ const SwitchRoot = React.forwardRef(function SwitchRoot(
className,
propGetter: getButtonProps,
ownerState,
extraProps: other,
extraProps: { ...other, disabled },
customStyleHookMapping: styleHookMapping,
ref: forwardedRef,
});
Expand Down
Loading