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

[EuiLoading] Accessibility props #4814

Closed
j-m opened this issue May 20, 2021 · 7 comments
Closed

[EuiLoading] Accessibility props #4814

j-m opened this issue May 20, 2021 · 7 comments

Comments

@j-m
Copy link
Contributor

j-m commented May 20, 2021

Summary

EuiLoading needs to better communicate it's state to assistive tech. We can improve the component in two ways:

  1. First, add role="progressbar" and a default aria-label value to the loading state
  2. Also, accept a child to handle properly setting aria-busy

Details

Doing the first, improves the existing usage of the component. Doing the second, is a non-breaking API change that should better handle a more ideal UX.

For existing implementations, or for those that can't/don't want to upgrade to the new pattern, we should provide better documentation around aria-label and aria-busy

Documentation on aria-label should largely ask that something better than "loading content" be passed in.

Documentation on aria-busy should talk about where to put it (around the loading content) and give warnings on how it works (that it treats all children as aria-hidden) so devs don't overuse it.

In addition to accepting children to handle the aria-busy attribute setting, it'll need to also accept an isLoading prop (which is required if children are passed in).

@myasonik
Copy link
Contributor

Thanks for bringing this up – great catch!

Do you find much use from the aria-live? I would have thought it wouldn't do anything because the component is either rendered or isn't – there aren't any updates happening within it.

@j-m
Copy link
Contributor Author

j-m commented May 20, 2021

Whoops, good point! I never tested that it was needed

I'm also not convinced progressbar is the most appropriate role, but it's the best I know of - accessibility's not my strong suit

@myasonik
Copy link
Contributor

The role is good but your mention of it made me take a second look at what you have we actually don't want aria-busy here either. aria-busy should be placed around the loading content, not around the spinner itself.

To give an example, you might do something like this:

<p>The paragraphs below are loaded async</p>
<EuiLoadingContent
  lines={10}
  role="progressbar"
  aria-label="Loading content"
/>
<section aria-busy={isLoading}>
	{isLoading && render()}
</section>

It's important to note that aria-busy should (in practice won't always but will often) treat all the children as aria-hidden making them invisible to assistive tech.

@myasonik
Copy link
Contributor

@j-m I hope it's all right, I rewrote your issue description to focus on the things we should do.

If you want to revive any of the historical context or if I missed anything, Github saves a history of edits and you can get your original version back where it says "edited by myasonik".

Thanks!

@j-m
Copy link
Contributor Author

j-m commented May 21, 2021

Of course! Makes it easier for everyone

Hmm in that case would it make more sense to change how EuiLoadingContent works?
Let EUI handle the conditional render.
Then we would be able to use aria-live and consumers wouldn't have to worry about adding aria-busy to the linked component(s).

<EuiLoadingContent
  lines={10}
  role="progressbar"
  aria-label="Loading content"
  isLoaded={true}
>
  <EuiDataGrid .../>
</EuiLoadingContent>

@myasonik
Copy link
Contributor

That's a really good idea and it doesn't look like that's a breaking change either which is always easier to roll out.

Updating the issue description once more.

cchaos added a commit that referenced this issue Apr 25, 2022
- Added tintOrShade and shadeOrTint functions
- Converted margins to use either `gap` or logical property
- Using Emotion `keyframes`
- Cleanup classes and for loop for bars
- Improved accessibility #4814
@JasonStoltz
Copy link
Member

Fixed by #6562

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants