Add lint rule for inaccessible disabled Button
#62080
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mirka
added
[Focus] Accessibility (a11y)
|
When you run Is anyone available to do a quick sweep of these and fix or ignore as appropriate? If not, I can go in and ignore them with some kind of TODO comment to fix when possible. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +3.06 kB (+0.18%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
tyxla
reviewed
May 29, 2024
| 'error', | ||
| { | ||
| selector: | ||
| 'JSXOpeningElement[name.name="Button"]:not(:has(JSXAttribute[name.name="__experimentalIsFocusable"])) JSXAttribute[name.name="disabled"]', |
There was a problem hiding this comment.
Confirmed that the rule works well 👍
|
FWIW, Ariakit handles a lot of this out of the box. I feel like before doing something like this we should just consider re-writing the internals to use Ariakit, keeping the same API. Then, it should be easier to figure out a strategy to nudge users to keep things accessible. This includes some ideas that I have explored in the past like making buttons with tooltips or binded as popover disclosure triggers automatically |
I have suggested this type of automatic mitigation as a preferred first step as well, but apparently there have already been a couple cases where this would not have been sufficient. Your idea of targeting all buttons bound as popover triggers sounds good, and would work in certain integrated components, but probably not all possible instances of a popover trigger. My feeling now is that these linting and automating strategies are not mutually exclusive, and would complement each other. We should continue to look into automating strategies, since that would benefit all consumers and not just Gutenberg where this lint rule would be in effect. People are already growing weary of the repeated accessibility issues that this introduces, so I'm hoping we can at least prevent new problematic instances and improve developer awareness, while working on some automated strategies in parallel. If they work well enough, maybe we can remove the lint rule. Does that sound reasonable? |
Empty action menu should still be perceivable to aleviate confusion, and does not clutter tab order due to Composite use.
disabled and aria-disabled are set with an identical condition, which doesn't make sense but signals the intent to keep it focusable.
…ption is always there)
… clutter tab order)
…doesn't clutter tab order)
…al that input is invalid)
mirka
commented
May 30, 2024
There was a problem hiding this comment.
I went ahead and looked through all of the errors myself, since I realized it would help me get a better overview of our problems.
For most of them I made a decision based on looking at the code only. Some of them, I looked into the original PR, blames, or actual app UI to gather more context. None of them are tested in app. I believe this is sufficient because technically this shouldn't be introducing "regressions" — there is little damage that adding a __experimentalIsFocusable could do. The only potentially regressive change is #62080 (comment), but the logic looks clear enough to me.
I wrote brief reasoning comments for each change to leave a trail in case somebody needs to blame.
Comment on lines
+147
to
+150
| expect( screen.getByRole( 'button' ) ).toHaveAttribute( | ||
| 'aria-disabled', | ||
| 'true' | ||
| ); |
There was a problem hiding this comment.
This is the recommended way to test for aria-disabled (see testing-library/jest-dom#144 (comment)).
There was a problem hiding this comment.
Or maybe we can keep both assertions as they check for different things?
There was a problem hiding this comment.
That makes a lot of sense, actually. Addressed in bca37a2.
| @@ -42,6 +42,7 @@ export default function InstallButton( { attributes, block, clientId } ) { | |||
| } | |||
| } ) | |||
| } | |||
| __experimentalIsFocusable | |||
There was a problem hiding this comment.
Clearly a busy state, which can cause focus loss.
Comment on lines
63
to
64
| // TODO: Investigate whether this button should be accessible when disabled. | ||
| // eslint-disable-next-line no-restricted-syntax |
There was a problem hiding this comment.
This was the only instance I didn't feel confident enough to make a decision without deeper investigation. This is a publicly exported component with both a tabIndex and disabled prop exposed. (But no usages of these props within the Gutenberg repo.)
There was a problem hiding this comment.
If I did the archeology correctly, it was added here: #9815
It can now be seen when inserting buttons in the Buttons block:
Judging by the functionality and the initial intent, my expectation is that this disabled was left there unintentionally and there's no reason for it to be there in the first place. I expect that the inserter appender will not be visible at all, rather than stay visible and disabled. With that in mind, I don't think it needs to be accessible if it ends up disabled.
There was a problem hiding this comment.
Thank you so much for investigating! I added a disable reason in d16d2b4.
| @@ -149,6 +149,7 @@ export default function LinkPreview( { | |||
| isEmptyURL || showIconLabels ? '' : ': ' + value.url | |||
| ) } | |||
| ref={ ref } | |||
| __experimentalIsFocusable | |||
There was a problem hiding this comment.
Alleviates confusion, since this is a "standard" button usually present in the view.
Comment on lines
224
to
236
| 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 } |
There was a problem hiding this comment.
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.)
| @@ -43,6 +43,7 @@ export default function RenameModal( { menuTitle, onClose, onSave } ) { | |||
|
|
|||
| <Button | |||
| __next40pxDefaultSize | |||
| __experimentalIsFocusable | |||
There was a problem hiding this comment.
Should be perceivable to signal that the user input is invalid.
| @@ -168,6 +168,7 @@ export default function PostPreviewButton( { | |||
| className={ className || 'editor-post-preview' } | |||
| href={ href } | |||
| target={ targetId } | |||
| __experimentalIsFocusable | |||
There was a problem hiding this comment.
Should be perceivable, because the button is expected to be there.
| @@ -93,6 +93,7 @@ export class PostPublishPanel extends Component { | |||
| </div> | |||
| <div className="editor-post-publish-panel__header-cancel-button"> | |||
| <Button | |||
| __experimentalIsFocusable | |||
There was a problem hiding this comment.
Can cause focus loss due to a dynamic disable.
| @@ -86,6 +86,7 @@ function ImportForm( { instanceId, onUpload } ) { | |||
| <Button | |||
| type="submit" | |||
| isBusy={ isLoading } | |||
| __experimentalIsFocusable | |||
There was a problem hiding this comment.
Should be perceivable, and can cause focus loss.
There was a problem hiding this comment.
Accessible disabled matches the behavior of the undo/redo buttons in the actual WP block editor.
After going through all these errors manually, I am now more convinced that we should be approaching this with a combination of automated mitigation and human decision-making (prompted by lints). Automated mitigation can prevent a baseline number of accessibility issues, but a human making context-aware design decisions for each instance can further enhance the UX for screen reader users. |
tyxla
reviewed
May 31, 2024
| @@ -219,7 +220,6 @@ function RevisionsButtons( { | |||
| </p> | |||
| ) : ( | |||
| <Button | |||
| disabled={ areStylesEqual } | |||
There was a problem hiding this comment.
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.
tyxla
approved these changes
May 31, 2024
There was a problem hiding this comment.
Thanks for taking the time to audit and fix them one by one! 👍
I spent some time going through them all with your comments and I think they all make sense. I've left a few comments where there was doubt or uncertainty.
I think it can be shipped - do you think we should ask folks from #59518 for some feedback just in case?
Thanks for the review, I appreciate it 🙏 Since this touches many files, I'll go ahead and merge today. It should be easy enough to revert or tweak, whether in full or in part. |
|
Thanks everyone for all your effort ❤️ A linting rule seems the best approach to prevent such a11y issues and, more importantly, to improve knowledge and education. |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jun 4, 2024
* Add lint rule for inaccessible disabled `Button` * Exclude react native files * Include files in root `storybook` folder * Fix in Storybook editor playground (matches actual behavior) * Fix in install block button (is clearly a busy state) * Ignore in gallery image reordering buttons * Fix in LinkControl copy link button (aleviates confusion) * Ignore in edit-site pagination buttons (not confusing, and useful) * Fix in enable custom fields (is clearly a busy state) * Fix in DataViews list view Empty action menu should still be perceivable to aleviate confusion, and does not clutter tab order due to Composite use. * Fix in DataViews CompactItemActions (is dropdown trigger) * Fix in template part title modal disabled and aria-disabled are set with an identical condition, which doesn't make sense but signals the intent to keep it focusable. * Fix in PageList block (should be perceivable, esp. because the description is always there) * Fix in ConvertToLinksModal button (should be perceivable, and doesn't clutter tab order) * Fix in edit-site "Apply globally" button (should be perceivable, and doesn't clutter tab order) * Fix in edit-site nav menu rename modal (should be perceivable to signal that input is invalid) * Fix in RevisionsButtons (button is never visible when `areStylesEqual`) * Fix in RevisionsButtons (contains important info and should be perceivable) * Fix in GlobalStylesSidebar (should be perceivable) * Fix in PostPublishPanel cancel button (can cause focus loss) * Fix in PostPreviewButton (should be perceivable) * Defer decision in ButtonBlockAppender * Fix in reusable blocks import form (should be perceivable, can cause focus loss) * Adapt test for PostPreviewButton * Improve rigidity of accessible disabled detection in test * Add disable reason for ButtonBlockAppender Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
This was referenced Jun 4, 2024
This was referenced Jun 4, 2024
|
This is a bug fix no? Looks like a bunch of buttons were made more accessible. |
ellatrix
added
[Type] Bug
ellatrix
pushed a commit
that referenced
this pull request
Jun 11, 2024
* Add lint rule for inaccessible disabled `Button` * Exclude react native files * Include files in root `storybook` folder * Fix in Storybook editor playground (matches actual behavior) * Fix in install block button (is clearly a busy state) * Ignore in gallery image reordering buttons * Fix in LinkControl copy link button (aleviates confusion) * Ignore in edit-site pagination buttons (not confusing, and useful) * Fix in enable custom fields (is clearly a busy state) * Fix in DataViews list view Empty action menu should still be perceivable to aleviate confusion, and does not clutter tab order due to Composite use. * Fix in DataViews CompactItemActions (is dropdown trigger) * Fix in template part title modal disabled and aria-disabled are set with an identical condition, which doesn't make sense but signals the intent to keep it focusable. * Fix in PageList block (should be perceivable, esp. because the description is always there) * Fix in ConvertToLinksModal button (should be perceivable, and doesn't clutter tab order) * Fix in edit-site "Apply globally" button (should be perceivable, and doesn't clutter tab order) * Fix in edit-site nav menu rename modal (should be perceivable to signal that input is invalid) * Fix in RevisionsButtons (button is never visible when `areStylesEqual`) * Fix in RevisionsButtons (contains important info and should be perceivable) * Fix in GlobalStylesSidebar (should be perceivable) * Fix in PostPublishPanel cancel button (can cause focus loss) * Fix in PostPreviewButton (should be perceivable) * Defer decision in ButtonBlockAppender * Fix in reusable blocks import form (should be perceivable, can cause focus loss) * Adapt test for PostPreviewButton * Improve rigidity of accessible disabled detection in test * Add disable reason for ButtonBlockAppender Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
ellatrix
pushed a commit
that referenced
this pull request
Jun 11, 2024
* Add lint rule for inaccessible disabled `Button` * Exclude react native files * Include files in root `storybook` folder * Fix in Storybook editor playground (matches actual behavior) * Fix in install block button (is clearly a busy state) * Ignore in gallery image reordering buttons * Fix in LinkControl copy link button (aleviates confusion) * Ignore in edit-site pagination buttons (not confusing, and useful) * Fix in enable custom fields (is clearly a busy state) * Fix in DataViews list view Empty action menu should still be perceivable to aleviate confusion, and does not clutter tab order due to Composite use. * Fix in DataViews CompactItemActions (is dropdown trigger) * Fix in template part title modal disabled and aria-disabled are set with an identical condition, which doesn't make sense but signals the intent to keep it focusable. * Fix in PageList block (should be perceivable, esp. because the description is always there) * Fix in ConvertToLinksModal button (should be perceivable, and doesn't clutter tab order) * Fix in edit-site "Apply globally" button (should be perceivable, and doesn't clutter tab order) * Fix in edit-site nav menu rename modal (should be perceivable to signal that input is invalid) * Fix in RevisionsButtons (button is never visible when `areStylesEqual`) * Fix in RevisionsButtons (contains important info and should be perceivable) * Fix in GlobalStylesSidebar (should be perceivable) * Fix in PostPublishPanel cancel button (can cause focus loss) * Fix in PostPreviewButton (should be perceivable) * Defer decision in ButtonBlockAppender * Fix in reusable blocks import form (should be perceivable, can cause focus loss) * Adapt test for PostPreviewButton * Improve rigidity of accessible disabled detection in test * Add disable reason for ButtonBlockAppender Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
* Add lint rule for inaccessible disabled `Button` * Exclude react native files * Include files in root `storybook` folder * Fix in Storybook editor playground (matches actual behavior) * Fix in install block button (is clearly a busy state) * Ignore in gallery image reordering buttons * Fix in LinkControl copy link button (aleviates confusion) * Ignore in edit-site pagination buttons (not confusing, and useful) * Fix in enable custom fields (is clearly a busy state) * Fix in DataViews list view Empty action menu should still be perceivable to aleviate confusion, and does not clutter tab order due to Composite use. * Fix in DataViews CompactItemActions (is dropdown trigger) * Fix in template part title modal disabled and aria-disabled are set with an identical condition, which doesn't make sense but signals the intent to keep it focusable. * Fix in PageList block (should be perceivable, esp. because the description is always there) * Fix in ConvertToLinksModal button (should be perceivable, and doesn't clutter tab order) * Fix in edit-site "Apply globally" button (should be perceivable, and doesn't clutter tab order) * Fix in edit-site nav menu rename modal (should be perceivable to signal that input is invalid) * Fix in RevisionsButtons (button is never visible when `areStylesEqual`) * Fix in RevisionsButtons (contains important info and should be perceivable) * Fix in GlobalStylesSidebar (should be perceivable) * Fix in PostPublishPanel cancel button (can cause focus loss) * Fix in PostPreviewButton (should be perceivable) * Defer decision in ButtonBlockAppender * Fix in reusable blocks import form (should be perceivable, can cause focus loss) * Adapt test for PostPreviewButton * Improve rigidity of accessible disabled detection in test * Add disable reason for ButtonBlockAppender Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
ellatrix
removed
the
Backport to WP 6.6 Beta/RC
|
This was cherry-picked to the wp/6.6 branch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative to #59518
Part of #56547
What?
Adds an eslint error for when the
disabledprop is used without the__experimentalIsFocusableprop in theButtoncomponent.Why?
We have long been discussing how to prevent accessibility issues stemming from
disabledcontrols (#56547). Although there are situations where you do want to completely disable a control, we more often find cases where this was not a conscious decision but simply not considered at all. In these cases, we often end up with inaccessible disabled controls, which are not perceivable by screen readers, or cause focus loss after closing a popover.Next steps
__experimentalIsFocusableprop. We should consider renaming it toaccessibleWhenDisabled, which is clear and matches the naming in Ariakit. (Button: Stabilize__experimentalIsFocusableprop #62282)accessibleWhenDisabledprop to other relevant components, and add those components to the eslint rule.How?
Other strategies considered include:
disabledprop to be accessible disabled by default. This is not ideal because it goes against the standard behavior of thedisabledHTML attribute.disabledprop toisDisabled, and then make that accessible disabled by default. This is not ideal because it will require a lot of API changes, and the behavioral difference between the two props would be unclear.disabled, and nudge people toward usingaria-disabledinstead. This is not ideal because usingaria-disabledis not necessarily going to address all issues, depending on the element/component.Testing Instructions
✅ The linter should error in places where a
Buttonhas adisabledprop without a__experimentalIsFocusableprop.✅ The error message should be clear and actionable.