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

Add lint rule for inaccessible disabled Button #62080

Merged
merged 26 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
6ebce5c
Add lint rule for inaccessible disabled `Button`
mirka May 28, 2024
cad8b3e
Exclude react native files
mirka May 28, 2024
9154483
Include files in root `storybook` folder
mirka May 30, 2024
7040967
Fix in Storybook editor playground (matches actual behavior)
mirka May 30, 2024
7a705eb
Fix in install block button (is clearly a busy state)
mirka May 30, 2024
cfefecc
Ignore in gallery image reordering buttons
mirka May 30, 2024
bb06929
Fix in LinkControl copy link button (aleviates confusion)
mirka May 30, 2024
a5696ec
Ignore in edit-site pagination buttons (not confusing, and useful)
mirka May 30, 2024
07b64d3
Fix in enable custom fields (is clearly a busy state)
mirka May 30, 2024
d94c5fb
Fix in DataViews list view
mirka May 30, 2024
0ec3bd9
Fix in DataViews CompactItemActions (is dropdown trigger)
mirka May 30, 2024
16ddf06
Fix in template part title modal
mirka May 30, 2024
8e72fa3
Fix in PageList block (should be perceivable, esp. because the descri…
mirka May 30, 2024
fc29ec1
Fix in ConvertToLinksModal button (should be perceivable, and doesn't…
mirka May 30, 2024
c666444
Fix in edit-site "Apply globally" button (should be perceivable, and …
mirka May 30, 2024
7e05a82
Fix in edit-site nav menu rename modal (should be perceivable to sign…
mirka May 30, 2024
a111992
Fix in RevisionsButtons (button is never visible when `areStylesEqual`)
mirka May 30, 2024
b6b9f09
Fix in RevisionsButtons (contains important info and should be percei…
mirka May 30, 2024
388112f
Fix in GlobalStylesSidebar (should be perceivable)
mirka May 30, 2024
f4d5ae2
Fix in PostPublishPanel cancel button (can cause focus loss)
mirka May 30, 2024
def18f4
Fix in PostPreviewButton (should be perceivable)
mirka May 30, 2024
8ff5fe9
Defer decision in ButtonBlockAppender
mirka May 30, 2024
d188fc5
Fix in reusable blocks import form (should be perceivable, can cause …
mirka May 30, 2024
6f39b75
Adapt test for PostPreviewButton
mirka May 30, 2024
bca37a2
Improve rigidity of accessible disabled detection in test
mirka Jun 3, 2024
d16d2b4
Add disable reason for ButtonBlockAppender
mirka Jun 3, 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
18 changes: 18 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,24 @@ module.exports = {
],
},
},
{
files: [
'packages/*/src/**/*.[tj]s?(x)',
'storybook/stories/**/*.[tj]s?(x)',
],
excludedFiles: [ '**/*.native.js' ],
rules: {
'no-restricted-syntax': [
'error',
{
selector:
'JSXOpeningElement[name.name="Button"]:not(:has(JSXAttribute[name.name="__experimentalIsFocusable"])) JSXAttribute[name.name="disabled"]',
Copy link
Member

Choose a reason for hiding this comment

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

Confirmed that the rule works well 👍

message:
'`disabled` used without the `__experimentalIsFocusable` prop. Disabling a control without maintaining focusability can cause accessibility issues, by hiding their presence from screen reader users, or preventing focus from returning to a trigger element. (Ignore this error if you truly mean to disable.)',
},
],
},
},
{
files: [
// Components package.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export default function InstallButton( { attributes, block, clientId } ) {
}
} )
}
__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.

Clearly a busy state, which can cause focus loss.

disabled={ isInstallingBlock }
isBusy={ isInstallingBlock }
variant="primary"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ function ButtonBlockAppender(
onClick={ onToggle }
aria-haspopup={ isToggleButton ? 'true' : undefined }
aria-expanded={ isToggleButton ? isOpen : undefined }
// Disable reason: There shouldn't be a case where this button is disabled but not visually hidden.
// eslint-disable-next-line no-restricted-syntax
disabled={ disabled }
label={ label }
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ export default function LinkPreview( {
isEmptyURL || showIconLabels ? '' : ': ' + value.url
) }
ref={ ref }
__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.

Alleviates confusion, since this is a "standard" button usually present in the view.

disabled={ isEmptyURL }
size="compact"
/>
Expand Down
8 changes: 8 additions & 0 deletions packages/block-library/src/gallery/v1/gallery-image.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,13 +222,17 @@ class GalleryImage extends Component {
onClick={ isFirstItem ? undefined : onMoveBackward }
label={ __( 'Move image backward' ) }
aria-disabled={ isFirstItem }
// Disable reason: Truly disable when image is not selected.
// eslint-disable-next-line no-restricted-syntax
disabled={ ! isSelected }
/>
<Button
icon={ chevronRight }
onClick={ isLastItem ? undefined : onMoveForward }
label={ __( 'Move image forward' ) }
aria-disabled={ isLastItem }
// Disable reason: Truly disable when image is not selected.
// eslint-disable-next-line no-restricted-syntax
disabled={ ! isSelected }
Comment on lines 224 to 236
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 was wondering whether a component having both disabled and aria-disabled props was something to flag with another lint, but this I think is a legitimate use case. (Though the same logic can also be expressed with a combination of the disabled and the __experimentalIsFocusable prop.)

/>
</ButtonGroup>
Expand All @@ -237,12 +241,16 @@ class GalleryImage extends Component {
icon={ edit }
onClick={ this.onEdit }
label={ __( 'Replace image' ) }
// Disable reason: Truly disable when image is not selected.
// eslint-disable-next-line no-restricted-syntax
disabled={ ! isSelected }
/>
<Button
icon={ closeSmall }
onClick={ onRemove }
label={ __( 'Remove image' ) }
// Disable reason: Truly disable when image is not selected.
// eslint-disable-next-line no-restricted-syntax
disabled={ ! isSelected }
/>
</ButtonGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export function ConvertToLinksModal( { onClick, onClose, disabled } ) {
</Button>
<Button
variant="primary"
__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.

Should be perceivable, and doesn't clutter tab sequence.

disabled={ disabled }
onClick={ onClick }
>
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/page-list/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ export default function PageListEdit( {
<p>{ convertDescription }</p>
<Button
variant="primary"
__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.

Should be perceivable, especially because the description is always present.

disabled={ ! hasResolvedPages }
onClick={ convertToNavigationLinks }
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ export default function TitleModal( { areaLabel, onClose, onSubmit } ) {
<Button
variant="primary"
type="submit"
__experimentalIsFocusable
disabled={ ! title.length }
aria-disabled={ ! title.length }
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 and aria-disabled were set with an identical condition, which is clearly a mistake, but signals the intent to keep it focusable.

>
{ __( 'Create' ) }
</Button>
Expand Down
1 change: 1 addition & 0 deletions packages/dataviews/src/item-actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ function CompactItemActions< Item extends AnyItem >( {
size="compact"
icon={ moreVertical }
label={ __( 'Actions' ) }
__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.

This is a dropdown trigger, so it can potentially cause focus loss.

disabled={ ! actions.length }
className="dataviews-all-actions-button"
/>
Expand Down
1 change: 1 addition & 0 deletions packages/dataviews/src/view-list.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ function ListItem< Item extends AnyItem >( {
size="compact"
icon={ moreVertical }
label={ __( 'Actions' ) }
__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.

An empty action menu should still be perceivable to alleviate confusion, and does not clutter tab sequence due to Composite use.

disabled={ ! actions.length }
onKeyDown={ ( event: {
key: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export function CustomFieldsConfirmation( { willEnable } ) {
className="edit-post-preferences-modal__custom-fields-confirmation-button"
variant="secondary"
isBusy={ isReloading }
__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.

Clearly a busy state, which can cause focus loss.

disabled={ isReloading }
onClick={ () => {
setIsReloading( true );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ export default function GlobalStylesSidebar() {
isPressed={
isStyleBookOpened || isRevisionsStyleBookOpened
}
__experimentalIsFocusable
disabled={ shouldClearCanvasContainerView }
onClick={ toggleStyleBook }
size="compact"
Expand All @@ -162,6 +163,7 @@ export default function GlobalStylesSidebar() {
label={ __( 'Revisions' ) }
icon={ backup }
onClick={ toggleRevisions }
__experimentalIsFocusable
disabled={ ! hasRevisions }
isPressed={
isRevisionsOpened || isRevisionsStyleBookOpened
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ function RevisionsButtons( {
>
<Button
className="edit-site-global-styles-screen-revisions__revision-button"
__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.

This button can contain a lot of important information and should be perceivable even when disabled.

disabled={ isSelected }
onClick={ () => {
onChange( revision );
Expand Down Expand Up @@ -219,7 +220,6 @@ function RevisionsButtons( {
</p>
) : (
<Button
disabled={ areStylesEqual }
Copy link
Member

Choose a reason for hiding this comment

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

Confirmed that this can and should be removed because areStylesEqual will never be true since it's already false based on the ternary logic this is nested in.

size="compact"
variant="primary"
className="edit-site-global-styles-screen-revisions__apply-button"
Expand Down
8 changes: 8 additions & 0 deletions packages/edit-site/src/components/pagination/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ export default function Pagination( {
<Button
variant={ buttonVariant }
onClick={ () => changePage( 1 ) }
// Disable reason: Would not cause confusion, and allows quicker access to a relevant nav button.
// eslint-disable-next-line no-restricted-syntax
disabled={ disabled || currentPage === 1 }
aria-label={ __( 'First page' ) }
>
Expand All @@ -54,6 +56,8 @@ export default function Pagination( {
<Button
variant={ buttonVariant }
onClick={ () => changePage( currentPage - 1 ) }
// Disable reason: Would not cause confusion, and allows quicker access to a relevant nav button.
// eslint-disable-next-line no-restricted-syntax
disabled={ disabled || currentPage === 1 }
aria-label={ __( 'Previous page' ) }
>
Expand All @@ -72,6 +76,8 @@ export default function Pagination( {
<Button
variant={ buttonVariant }
onClick={ () => changePage( currentPage + 1 ) }
// Disable reason: Would not cause confusion, and allows quicker access to a relevant nav button.
// eslint-disable-next-line no-restricted-syntax
disabled={ disabled || currentPage === numPages }
aria-label={ __( 'Next page' ) }
>
Expand All @@ -80,6 +86,8 @@ export default function Pagination( {
<Button
variant={ buttonVariant }
onClick={ () => changePage( numPages ) }
// Disable reason: Would not cause confusion, and allows quicker access to a relevant nav button.
// eslint-disable-next-line no-restricted-syntax
disabled={ disabled || currentPage === numPages }
aria-label={ __( 'Last page' ) }
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export default function RenameModal( { menuTitle, onClose, onSave } ) {

<Button
__next40pxDefaultSize
__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.

Should be perceivable to signal that the user input is invalid.

disabled={ ! isEditedMenuTitleValid }
variant="primary"
type="submit"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ function PushChangesToGlobalStylesControl( {
<Button
__next40pxDefaultSize
variant="secondary"
__experimentalIsFocusable
disabled={ changes.length === 0 }
onClick={ pushChanges }
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ export default function PostPreviewButton( {
className={ className || 'editor-post-preview' }
href={ href }
target={ targetId }
__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.

Should be perceivable, because the button is expected to be there.

disabled={ ! isSaveable }
onClick={ openPreviewWindow }
role={ role }
Expand Down
12 changes: 10 additions & 2 deletions packages/editor/src/components/post-preview-button/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,16 @@ describe( 'PostPreviewButton', () => {
).toBeInTheDocument();
} );

it( 'should be disabled if post is not saveable.', () => {
it( 'should be accessibly disabled if post is not saveable.', () => {
mockUseSelect( { isEditedPostSaveable: () => false } );

render( <PostPreviewButton /> );

expect( screen.getByRole( 'button' ) ).toBeDisabled();
expect( screen.getByRole( 'button' ) ).toBeEnabled();
expect( screen.getByRole( 'button' ) ).toHaveAttribute(
'aria-disabled',
'true'
);
Comment on lines +148 to +151
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the recommended way to test for aria-disabled (see testing-library/jest-dom#144 (comment)).

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we can keep both assertions as they check for different things?

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes a lot of sense, actually. Addressed in bca37a2.

} );

it( 'should not be disabled if post is saveable.', () => {
Expand All @@ -153,6 +157,10 @@ describe( 'PostPreviewButton', () => {
render( <PostPreviewButton /> );

expect( screen.getByRole( 'button' ) ).toBeEnabled();
expect( screen.getByRole( 'button' ) ).not.toHaveAttribute(
'aria-disabled',
'true'
);
} );

it( 'should set `href` to edited post preview link if specified.', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/post-publish-panel/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export class PostPublishPanel extends Component {
</div>
<div className="editor-post-publish-panel__header-cancel-button">
<Button
__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.

Can cause focus loss due to a dynamic disable.

disabled={ isSavingNonPostEntityChanges }
onClick={ onClose }
variant="secondary"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ function ImportForm( { instanceId, onUpload } ) {
<Button
type="submit"
isBusy={ isLoading }
__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.

Should be perceivable, and can cause focus loss.

disabled={ ! file || isLoading }
variant="secondary"
className="list-reusable-blocks-import-form__button"
Expand Down
2 changes: 2 additions & 0 deletions storybook/stories/playground/with-undo-redo/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,14 @@ export default function EditorWithUndoRedo() {
<Button
onClick={ undo }
disabled={ ! hasUndo }
__experimentalIsFocusable
icon={ undoIcon }
label="Undo"
/>
<Button
onClick={ redo }
disabled={ ! hasRedo }
__experimentalIsFocusable
icon={ redoIcon }
label="Redo"
/>
Expand Down
Loading