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

Fixed EuiBetaBadge a11y #2559

Merged
merged 4 commits into from
Nov 26, 2019
Merged

Fixed EuiBetaBadge a11y #2559

merged 4 commits into from
Nov 26, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Nov 25, 2019

Summary

Since EuiBetaBadge's don't actually have any clickability to them, they were ignored from the tab order and therefore, the description which only shows on hover could never be read by a screenreader. This PR basically just adds tab-index=0 to the element putting it into the focus order of the DOM. We handle this the same way for EuiIconTip.

Screen Recording 2019-11-25 at 12 39 PM

Screen Shot 2019-11-21 at 11 53 23 AM

This also then required moving the DOM order of the beta badge in EuiCard so that it didn't come before the title of the card.

Screen Recording 2019-11-21 at 12 01 PM

Checklist

  • [ ] Checked in dark mode
  • [ ] Checked in mobile
  • Checked in IE11 and Firefox
  • [ ] Props have proper autodocs
  • [ ] Added documentation examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

cchaos added 2 commits November 21, 2019 11:53
Fix for a11y by making the span focusable and adds focus ring for visual indicator
@snide snide mentioned this pull request Nov 25, 2019
19 tasks
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; pulled and tested beta badge docs locally

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

I thought on this PR for a long while... Probably longer than I needed to...

Just adding a tabIndex to a non-interactive element doesn't sit right but I couldn't figure out a smarter solution and it largely works, so, ultimately a 👍 from me. It definitely improves on the current state!

Ignoring the semantics of it, the one practical issue I came across is when navigating the page with a screen reader (not jumping around with TAB), the description isn't read out and nothing tells me that this element is something special so I should focus on it.

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Tested locally and LGTM! 🎉

@cchaos
Copy link
Contributor Author

cchaos commented Nov 26, 2019

@myasonik Yeah it's more of a limitation of our basic tooltip behavior than specific to EuiBetaBadge that we should address at some point. Thanks!

@cchaos cchaos merged commit b90dbbb into elastic:master Nov 26, 2019
@cchaos cchaos deleted the beta-badge-a11y branch November 26, 2019 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants