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.

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

Components: Fix inaccessible disabled Buttons #62306

Merged
merged 12 commits into from
Jul 3, 2024
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@ module.exports = {
'no-restricted-syntax': [
'error',
...restrictedSyntax,
...restrictedSyntaxComponents,
{
selector:
':matches(Literal[value=/--wp-admin-theme-/],TemplateElement[value.cooked=/--wp-admin-theme-/])',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,19 +134,19 @@ describe( 'BlockSwitcher', () => {
expect( items[ 1 ] ).toHaveTextContent( headingBlockType.title );
} );

test( 'should render disabled block switcher when we have a single selected block without styles and we cannot remove it', () => {
test( 'should render accessibly disabled block switcher when we have a single selected block without styles and we cannot remove it', () => {
useSelect.mockImplementation( () => ( {
blocks: [ headingBlock1 ],
icon: copy,
hasBlockStyles: false,
canRemove: false,
} ) );
render( <BlockSwitcher clientIds={ [ headingBlock1.clientId ] } /> );
expect(
screen.getByRole( 'button', {
name: 'Block Name',
} )
).toBeDisabled();
const blockSwitcher = screen.getByRole( 'button', {
name: 'Block Name',
} );
expect( blockSwitcher ).toBeEnabled();
expect( blockSwitcher ).toHaveAttribute( 'aria-disabled', 'true' );
} );

test( 'should render message for no available transforms', async () => {
Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- `UnitControl`: Fix colors when disabled. ([#62970](https://github.com/WordPress/gutenberg/pull/62970))
- `useUpdateEffect`: Correctly track mounted state in strict mode. ([#62974](https://github.com/WordPress/gutenberg/pull/62974))
- `UnitControl`: Fix an issue where keyboard shortcuts unintentionally shift focus on Windows OS. ([#62988](https://github.com/WordPress/gutenberg/pull/62988))
- Fix inaccessibly disabled `Button`s ([#62306](https://github.com/WordPress/gutenberg/pull/62306)).

### Internal

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ function ListBox( {
id={ `components-autocomplete-item-${ instanceId }-${ option.key }` }
role="option"
aria-selected={ index === selectedIndex }
__experimentalIsFocusable
Copy link
Member Author

Choose a reason for hiding this comment

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

Disabled items in a popover menu should be perceivable.

disabled={ option.isDisabled }
className={ clsx(
'components-autocomplete__result',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const VariantStates: StoryFn< typeof Button > = (
key={ variant ?? 'undefined' }
>
<Button { ...props } variant={ variant } />
{ /* eslint-disable-next-line no-restricted-syntax */ }
<Button { ...props } variant={ variant } disabled />
<Button
{ ...props }
Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/button/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ describe( 'Button', () => {
} );

it( 'should add a disabled prop to the button', () => {
// eslint-disable-next-line no-restricted-syntax
render( <Button disabled /> );

expect( screen.getByRole( 'button' ) ).toBeDisabled();
Expand Down Expand Up @@ -536,6 +537,7 @@ describe( 'Button', () => {

it( 'should become a button again when disabled is supplied', () => {
// @ts-expect-error - a button should not have `href`
// eslint-disable-next-line no-restricted-syntax
render( <Button href="https://wordpress.org/" disabled /> );

expect( screen.getByRole( 'button' ) ).toBeVisible();
Expand Down Expand Up @@ -624,8 +626,12 @@ describe( 'Button', () => {
<Button href="foo" />
{ /* @ts-expect-error - `target` requires `href` */ }
<Button target="foo" />

{ /* eslint-disable no-restricted-syntax */ }
{ /* @ts-expect-error - `disabled` is only for buttons */ }
<Button href="foo" disabled />
{ /* eslint-enable no-restricted-syntax */ }

<Button href="foo" type="image/png" />
{ /* @ts-expect-error - if button, type must be submit/reset/button */ }
<Button type="image/png" />
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/combobox-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,8 @@ function ComboboxControl( props: ComboboxControlProps ) {
<Button
className="components-combobox-control__reset"
icon={ closeSmall }
// Disable reason: Focus returns to input field when reset is clicked.
// eslint-disable-next-line no-restricted-syntax
disabled={ ! value }
onClick={ handleOnReset }
label={ __( 'Reset' ) }
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/dropdown-menu/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ function UnconnectedDropdownMenu( dropdownMenuProps: DropdownMenuProps ) {
? control.role
: 'menuitem'
}
__experimentalIsFocusable
Copy link
Member Author

Choose a reason for hiding this comment

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

Disabled items in a dropdown menu should be perceivable.

disabled={ control.isDisabled }
>
{ control.title }
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/form-token-field/token.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ export default function Token( {
className="components-form-token-field__remove-token"
icon={ closeSmall }
onClick={ ! disabled ? onClick : undefined }
// Disable reason: Even when FormTokenField itself is accessibly disabled, token reset buttons shouldn't be in the tab sequence.
// eslint-disable-next-line no-restricted-syntax
disabled={ disabled }
label={ messages.remove }
aria-describedby={ `components-form-token-field__token-text-${ instanceId }` }
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/range-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,8 @@ function UnforwardedRangeControl(
<ActionRightWrapper>
<Button
className="components-range-control__reset"
// If the RangeControl itself is disabled, the reset button shouldn't be in the tab sequence.
__experimentalIsFocusable={ ! disabled }
disabled={ disabled || value === undefined }
Copy link
Member Author

Choose a reason for hiding this comment

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

There is a bug in the disabled logic here (value will never be undefined), extracted as a separate issue in #63061.

variant="secondary"
size="small"
Expand Down
5 changes: 5 additions & 0 deletions packages/components/src/toolbar/toolbar-button/index.tsx
Copy link
Member Author

Choose a reason for hiding this comment

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

Disabled toolbar buttons should always be perceivable.

Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ function UnforwardedToolbarButton(
className
) }
isPressed={ isActive }
__experimentalIsFocusable
Copy link
Member Author

Choose a reason for hiding this comment

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

Disabled toolbar buttons should always be perceivable.

disabled={ isDisabled }
data-toolbar-item
{ ...extraProps }
Expand All @@ -85,6 +86,10 @@ function UnforwardedToolbarButton(
<Button
label={ title }
isPressed={ isActive }
// TODO: Should be focusable disabled, but adding `__experimentalIsFocusable` will trigger a
// focus bug when ToolbarButton is disabled via the `disabled` prop.
// Must address first: https://github.com/WordPress/gutenberg/issues/63070
// eslint-disable-next-line no-restricted-syntax
disabled={ isDisabled }
Comment on lines +89 to 93
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll need to fix this one after #63070 is addressed.

{ ...toolbarItemProps }
>
Expand Down
Loading