elastic / eui

Elastic UI Framework πŸ™Œ
https://eui.elastic.co/
Other
58 stars 842 forks source link

[EuiButton] Review the `isLoading` and `isDisabled` prop behavior for screen reader experience #5666

Open 1Copenut opened 2 years ago

1Copenut commented 2 years ago

Description

@miukimiu brought up a great question in #5649 with her comment about the isLoading behavior:

To introduce a EuiButtonIcon isLoading state I can follow the other buttons isLoading state approach:

  • The button gets disabled.
  • The EuiIcon gets replaced with a EuiLoadingSpinner.
  • We introduce a new size to EuiLoadingSpinner "XXL" so that all the sizes match the EuiIcon sizes.

But I have some a11y questions.

  • When the button gets disabled the focus state is missed.
  • It gets removed from the accessibility tree and is invisible to assistive technology users

So when the EuiButtonIcon isLoading would adding an aria-disabled instead of disabled be more appropriate? Or is it OK to keep following the other buttons' pattern?

The challenge

Our buttons all use the HTML5 disabled attribute to remove buttons' default behavior. Visual users can see the dimmed button and make an assertion they need to take some action before the button will become enabled.

Screen reader users do not have this same experience. The disabled attribute removes the button's tabindex (so no keyboard navigation) and removes it from interactive element menus for screen readers. For all intents and purposes, the button is invisible to assistive technology.

Some relevant research

WCAG guidance

chandlerprall commented 2 years ago

πŸ‘ If we decide to implement aria-disabled instead of disabled in some cases, we'll need to prevent onClick and possibly other events from firing.

elizabetdev commented 2 years ago

The following article is also interesting to add to @1Copenut's relevant research list: Making Disabled Buttons More Inclusive.

github-actions[bot] commented 2 years ago

πŸ‘‹ Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

1Copenut commented 2 years ago

Another discussion of this point, on Material UI: https://github.com/mui/material-ui/issues/14455

cee-chen commented 1 year ago

https://github.com/elastic/eui/issues/4797 Should be handled at the same time

github-actions[bot] commented 1 year ago

πŸ‘‹ Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

github-actions[bot] commented 7 months ago

πŸ‘‹ Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

JasonStoltz commented 5 months ago

We re-prioritized this in our queue as a High priority.

As it stands today, there are many instances throughout Kibana of disabled EuiButtons that have tooltips attached, and it is a pattern that is still unfortunately being propogated.

Disabled buttons are inaccessible to screenreaders, and the tooltip information in these disabled buttons give valuable context as to why the button is disabled. This means that screen readers have no idea that the button exists, is disabled, and contains additional context. This can be a dead end for screen reader users.

We would like to prioritize changing our buttons to leverage aria-disabled instead.

A challenge we'll have here is that we'll have to programatically mimic a disabled button rather than relying on the native disabled behavior, which could have downstream impacts to current usage. We should consider rolling this out gradually as a new beta variant of the button, rather than changing all buttons at once.

Additionally, we'll need to ensure this works for button groups as well.

JasonStoltz commented 5 months ago

Beyond EuiButton, @JoseLuisGJ pointed out that we may have some other components that need to be reviewed. There is at least one case of an EuiCard in a disabled state using a tooltip.

Image