[Button] Make disabled buttons more accessible #6461

zakwarsame · 2022-06-30T21:42:09Z

WHY are these changes introduced?

Buttons are currently insufficiently accessible when the disabled prop is set to true
The native disabled HTML attribute causes this by ignoring the entire element when navigating with the Tab or Tab + Shift keys and also makes it difficult for screen readers to locate it.
Using the aria-disabled instead makes it easy for screen readers and keyboard users to interpret the button.

Fixes #5967

WHAT is this pull request doing?

Replaces disabled with aria-disabled

How to 🎩

Using storybook, go to Button > Disabled state, and navigate using the Tab key

🖥 Check out this short demo video

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide

…native html disabled

github-actions · 2022-06-30T21:46:00Z

size-limit report 📦

Path	Size
polaris-react-cjs	198.26 KB (+0.05% 🔺)
polaris-react-esm	133.14 KB (+0.05% 🔺)
polaris-react-esnext	188.29 KB (+0.04% 🔺)
polaris-react-css	41.75 KB (+0.02% 🔺)

mrcthms

Overall, your logic is pretty sound here. Just a couple of semantic things to take a look at.

Also, you mentioned the disabled buttons still having a focus state when a user presses a key. Ideally we would want to avoid having the button change visually at all when it is disabled, event when focused. It's an industry best practice to do so, and also could cause potential confusion for users if it is the only button on screen and they feel they are interacting with it, but don't have an active button to compare it to note this is a disabled button, rather than us just having super low contrast regular buttons, if that makes sense.

.changeset/hot-crabs-act.md

polaris-react/src/components/Button/Button.tsx

polaris-react/src/components/UnstyledButton/UnstyledButton.tsx

polaris-react/src/components/UnstyledButton/tests/UnstyledButton.test.tsx

zakwarsame · 2022-07-06T19:56:24Z

thanks for the premium feedback marc, should all be resolved :)

mrcthms

This looks good to me! We'll probably need to get somebody from @Shopify/polaris-team to take a 👀 too though before we can merge it.

mrcthms · 2022-07-07T10:04:18Z

polaris-react/src/components/Button/Button.scss

@@ -224,7 +224,7 @@
  }

  &:hover,
-  &:focus {
+  &:focus:not(.disabled) {


alex-page · 2022-07-07T14:34:36Z

polaris-react/src/components/Button/Button.tsx

+  const handleClick = useCallback(
+    (event: React.MouseEvent<HTMLButtonElement>) => {
+      if (disabled) {
+        event.preventDefault();
+        return;
+      }
+      toggleDisclosureActive();
+    },
+    [disabled, toggleDisclosureActive],
+  );
+  const handleKeyDown = useCallback(
+    (event: React.KeyboardEvent<HTMLButtonElement>) => {
+      if (['Enter', ' '].includes(event.key) && disabled) {
+        event.preventDefault();
+      }
+    },
+    [disabled],
+  );


I don't know off the top of my head, but I assumed using aria-disabled would have given us this functionality for free. This logic could probably be repeated for all Polaris elements that are disabled.

sarahill · 2022-07-07T15:31:43Z

I don't know off the top of my head, but I assumed using aria-disabled would have given us this functionality for free. This logic could probably be repeated for all Polaris elements that are disabled.

+1 aria-disabled should give us what we want here and communicates with folks using screen readers https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-disabled

zakwarsame · 2022-07-07T15:46:08Z

I don't know off the top of my head, but I assumed using aria-disabled would have given us this functionality for free. This logic could probably be repeated for all Polaris elements that are disabled.

+1 aria-disabled should give us what we want here and communicates with folks using screen readers https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-disabled

Yeah we could benefit from the fact that it communicates with assistive technologies. Apart from the semantics, though, we don't get features like click prevention, focus prevention:

While adding disabled to an HTML form control causes :disabled user-agent styles to be applied, adding aria-disabled="true" doesn't, by default, alter an element's appearance. The element can be styled with the attribute selector `[aria-disabled="true"] ...
You should include JavaScript to disable the functionality of the element while also changing the appearance of the element so sighted users know it is disabled. - MDN

A nice table from this article reference:

I'm currently exploring exporting the logic to a custom hook in order to be repeated in other Polaris elements.

alex-page · 2022-07-11T20:08:45Z

Hey @zakwarsame I don't think we should ship changes based off the css tricks article. Have we found issues using assistive technologies? Can we replicate the issue you are fixing?

zakwarsame · 2022-07-11T21:52:36Z

Hey @zakwarsame I don't think we should ship changes based off the css tricks article. Have we found issues using assistive technologies? Can we replicate the issue you are fixing?

The source I quoted there is actually from MDN- I only used the table from css tricks to represent what's written in MDN visually. I can understand that we need to confirm it, so I've replicated this myself. Here is how:

Issue

What we're trying to fix can be easily replicated on our current storybook by clicking through the disabled buttons with both mouse and keyboard, and observing that nothing happens. Below I'm demonstrating the problem. Neither the keyboard, nor the assistive voice-over picks up that there is a disabled button:

Current disabled button walk-through video (unmute to hear 🔊) :

disabled-issue.mov

Solution

aria-disabled tackles this by enabling keyboard navigation and indicates to assistive technologies that the element upon which it is set and all focusable descendants are disabled.

Walk-through showing that with aria-disabled, we can both navigate and get feedback from an assistive technology for indication (unmute to hear 🔊)

assistive-voiceover.mov

Why we need to additionally disable clicks with javascript on aria-disabled elements

Even though we have aria-disabled and pointer-events: none; in our styling for these elements, keyboards are still able to click the buttons.

Below, notice how nothing happens when I click with a mouse (due to our css code pointer-events: none;) but also nothing prevents the keyboard from clicking:

Here I'm showing what happens when we click the button with `aria-disabled` and our current styling

not-quite-fixed.mov

I've temporarily included the code console.log('This should not be clickable') in the handleClick function for emphasis on problem. It's not in the PR.

Final proposal

With this PR, we get the best of aria-disabled and Javascript by stopping the button from being clicked with both mouse and keyboard while maintaining the accessibility functionality.

Assistive technology picks it up (unmute to hear 🔊):

assistive-fixed.mov

Keyboard cannot click button but can navigate

keyboard-fixed.mov

alex-page · 2022-07-11T21:56:15Z

@zakwarsame this is an excellent write up. Thank you. Honestly this is super.

It's interesting to me that disabled elements can receive focus. What do you think about changing the tabindex? This could stop disabled elements from receiving focus and then if they aren't focused they cannot be clicked on. Is there a reason they need to be focusable?

zakwarsame · 2022-07-11T22:49:30Z

Thank you! I think having them be focusable but styling to indicate that they're not clickable is the perfect compromise.

Keyboard accessibility is one of the most important aspects of web accessibility. Many users with motor disabilities rely on a keyboard. Some people have tremors which don't allow for fine muscle control. Others have little or no use of their hands, or no hands at all. In addition to traditional keyboards, some users may use modified keyboards or other hardware that mimics the functionality of a keyboard. Blind users also typically use a keyboard for navigation. Users without disabilities may use a keyboard for navigation because of preference or efficiency
-WebAIM

Considering that most people with visual impairments will prefer to navigate using the keyboard, wouldn't tabIndex do the opposite of what we're trying to achieve here?

A negative value (usually tabindex="-1") means that the element is not reachable via sequential keyboard navigation...
The user won't be able to focus any element with a negative tabindex using the keyboard - MDN

I'm going to explore this further before merging, for best results.

zakwarsame · 2022-07-12T14:06:15Z

tabIndex is picked up by screen readers. This is the best solution for avoiding keyboard focus and allowing assistive technologies to navigate.

No focus when only navigating with keyboards:

not-focusable.mov

With a screen reader (🔊):

tabIndex-soln.mov

polaris-react/src/components/Button/Button.tsx

mrcthms

Couple of little nits but nothing from my side that would block a release, nice one!

polaris-react/src/utilities/use-disable-interaction.ts

polaris-react/src/components/UnstyledButton/UnstyledButton.tsx

arthurgouveia · 2023-06-23T16:18:57Z

@zakwarsame I'm currently having to run Polaris Uplift on Email, and we need to overwrite some properties to make sure it matches our UI.

Normally, I target :disabled for that and got surprised we don't add it do the button. While I can see aria-disabled addition being a good thing, why would we remove the native disabled as well? Would putting it back cause any harm to the current implementation?

From an a11y POV, I don't see how disabled and aria-disabled wouldn't announce the same on a screenreader given both should serve the same purpose, while the first actually also natively makes the button not work.

Surely I can target the aria-disabled but I'm curious as to why we would remove the native browser solution as well.

replace our button disabled method by using aria-disabled instead of …

83fde2a

…native html disabled

zakwarsame changed the title ~~[Button] Making disabled duttons more accessible~~ [Button] Make disabled buttons more accessible Jun 30, 2022

zakwarsame added 6 commits June 30, 2022 18:42

avoid interaction with buttons that are disabled

d6d7074

add changeset

51cacbd

remove unused code

af42674

tests for preventing events onclick and keypressdown

259ff24

correct bulkactions disabled test

449ca68

minor namechange on a variable

98c5b1d

mrcthms requested changes Jul 5, 2022

View reviewed changes

convert changeset to minor and correct both click and keypress logics

64dae27

revert scss 'active' modification

c70ad73

mrcthms approved these changes Jul 7, 2022

View reviewed changes

alex-page reviewed Jul 7, 2022

View reviewed changes

zakwarsame added 7 commits July 8, 2022 20:41

create hook to be used in multiple places

6adaa60

create hook to be used in multiple places

fe049c8

create tests for the new hook

f37311d

remove typo

31bb2e1

remove typo

57010b2

remove typos

405f3e0

use stoppropagation on tests

f19eaae

add a negative tabIndex to disabled buttons and test them

85c1fd8

alex-page reviewed Jul 12, 2022

View reviewed changes

polaris-react/src/components/Button/Button.tsx Outdated Show resolved Hide resolved

alex-page force-pushed the main branch 7 times, most recently from a3a40bb to 0281d22 Compare July 14, 2022 05:31

mrcthms approved these changes Jul 18, 2022

View reviewed changes

polaris-react/src/utilities/use-disable-interaction.ts Outdated Show resolved Hide resolved

polaris-react/src/utilities/use-disable-interaction.ts Outdated Show resolved Hide resolved

polaris-react/src/components/UnstyledButton/UnstyledButton.tsx Outdated Show resolved Hide resolved

zakwarsame added 3 commits July 20, 2022 12:19

make handlers require event and stop setting the default

63041ce

remove keydown since there won't be focus with keyboards

c0120c4

remove keydown since there won't be focus with keyboards

f7a8409

zakwarsame merged commit 6fab899 into main Jul 21, 2022

zakwarsame deleted the improve/disabled-button-state branch July 21, 2022 18:13

This was referenced Jul 21, 2022

Version Packages #6694

Merged

Version Packages #6721

Merged

sam-b-rose mentioned this pull request Jul 28, 2022

[Button] Events stop propagating on click due to explicitly calling handler #6798

Closed

acmertz mentioned this pull request Nov 18, 2022

Properly handle buttons with aria-disabled in focus helpers #7760

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Button] Make disabled buttons more accessible #6461

[Button] Make disabled buttons more accessible #6461

zakwarsame commented Jun 30, 2022 •

edited

github-actions bot commented Jun 30, 2022 •

edited

mrcthms left a comment

zakwarsame commented Jul 6, 2022

mrcthms left a comment

mrcthms Jul 7, 2022

alex-page Jul 7, 2022

sarahill commented Jul 7, 2022

zakwarsame commented Jul 7, 2022 •

edited

alex-page commented Jul 11, 2022

zakwarsame commented Jul 11, 2022 •

edited

alex-page commented Jul 11, 2022 •

edited

zakwarsame commented Jul 11, 2022 •

edited

zakwarsame commented Jul 12, 2022

mrcthms left a comment

arthurgouveia commented Jun 23, 2023

[Button] Make disabled buttons more accessible #6461

[Button] Make disabled buttons more accessible #6461

Conversation

zakwarsame commented Jun 30, 2022 • edited

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

github-actions bot commented Jun 30, 2022 • edited

size-limit report 📦

mrcthms left a comment

Choose a reason for hiding this comment

zakwarsame commented Jul 6, 2022

mrcthms left a comment

Choose a reason for hiding this comment

mrcthms Jul 7, 2022

Choose a reason for hiding this comment

alex-page Jul 7, 2022

Choose a reason for hiding this comment

sarahill commented Jul 7, 2022

zakwarsame commented Jul 7, 2022 • edited

alex-page commented Jul 11, 2022

zakwarsame commented Jul 11, 2022 • edited

alex-page commented Jul 11, 2022 • edited

zakwarsame commented Jul 11, 2022 • edited

zakwarsame commented Jul 12, 2022

mrcthms left a comment

Choose a reason for hiding this comment

arthurgouveia commented Jun 23, 2023

zakwarsame commented Jun 30, 2022 •

edited

github-actions bot commented Jun 30, 2022 •

edited

zakwarsame commented Jul 7, 2022 •

edited

zakwarsame commented Jul 11, 2022 •

edited

alex-page commented Jul 11, 2022 •

edited

zakwarsame commented Jul 11, 2022 •

edited