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

[EuiKeyboardAccessible] Discuss deprecation #3838

Closed
myasonik opened this issue Jul 30, 2020 · 8 comments · Fixed by #4135
Closed

[EuiKeyboardAccessible] Discuss deprecation #3838

myasonik opened this issue Jul 30, 2020 · 8 comments · Fixed by #4135

Comments

@myasonik
Copy link
Contributor

I'd like to deprecate and remove EuiKeyboardAccessible.

From the docs:

You can make interactive elements keyboard-accessible with the EuiKeyboardAccessible component. This is necessary for non-button elements and a tags without href attributes.

But that doesn't seem to hold up because:

  1. All interactive elements should either be a native HTML element (e.g., button, anchor, label) or are more complex components (e.g., tabs, typeaheads) that require a lot more work than what EuiKeyboardAccessible does or even could reasonably do
  2. The example here of anchors without href attributes shouldn't be interactive so I wouldn't recommend using it like that

And, taking a look at Kibana as an example, there are 5 uses of it today:

  • 2 are being used on divs that should probably be converted to native button elements
  • 1 is being used as some super janky focus control and doesn't do anything and should probably just be removed
  • 1 is wrapping a div which wraps a button element... so, I guess should probably be removed?
  • 1 is wrapping an anchor so should probably be removed

So, at least in Kibana, there seems to be a lot of misuse of it already.

@thompsongl
Copy link
Contributor

thompsongl commented Jul 31, 2020

There are only two components in EUI that use it (EuiBasicTable and EuiStepHorizontal; one usage each). I'm guessing both instances could benefit from a more tailored approach like mentioned above.

@cchaos
Copy link
Contributor

cchaos commented Aug 3, 2020

Don't forget, however, to not just check for usages of the component <EuiScreenReaderOnly> but we also use the class .euiScreenReadOnly and the SASS mixin euiScreenReaderOnly(). I would suggest auditing the usage of those as well.

@myasonik
Copy link
Contributor Author

myasonik commented Aug 3, 2020

@cchaos I'm only suggesting removing EuiKeyboardAccessible, I'm not suggesting touching EuiScreenReaderOnly at all.

I double checked and it doesn't seem like EuiKeyboardAccessible has any associated styles but if you know of any, point me in the direction and I'll check Kibana for it.

@cchaos
Copy link
Contributor

cchaos commented Aug 4, 2020

Gah! I so did not read that correctly at all. Sorry... I'm good with deprecating it. Probably just throw a "Set for deprecation" label up on it, and let's add it to the next (Dec. 2020) deprecation cycle #1469

@myasonik
Copy link
Contributor Author

myasonik commented Aug 4, 2020

Should we remove it from the docs?

@cchaos
Copy link
Contributor

cchaos commented Aug 4, 2020

No, just setup a label like we have for EuiNavDrawer.

@cchaos cchaos changed the title [Discuss] Deprecate [EuiKeyboardAccessible] [EuiKeyboardAccessible] Discuss deprecation Sep 20, 2020
@cchaos cchaos added this to Needs triage/design in Accessibility via automation Sep 20, 2020
@cchaos cchaos moved this from Needs triage/design to Not started in Accessibility Sep 20, 2020
@cchaos cchaos moved this from Not started to Needs triage/design in Accessibility Sep 20, 2020
@myasonik
Copy link
Contributor Author

So I started taking a look at removing EuiKeyboardAccessible I don't know how to proceed with EuiBasicTable.

It uses EuiKeyboardAccessible on a table row which gives it a role=button and assigns a click event to that which isn't something screen readers can really handle...

The best way to make a clickable table row accessible is to delegate click events on the row onto a visible button but that would be a pretty big breaking change. Is that something we can roll out somehow? (Slowly?)

@snide
Copy link
Contributor

snide commented Oct 14, 2020

@myasonik It might be a good idea to just remove EuiKeyboardAccessible and move it's functionality into EuiBasicTable in that one spot. That way at least the component itself doesn't spread. We can target the table issue in a later PR. Off hand, the role=button in that case seems needed. It seems more a limitation of HTML table structures versus the reality of need (clickable rows).

Accessibility automation moved this from Needs triage/design to Closed Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Accessibility
  
Closed
Development

Successfully merging a pull request may close this issue.

4 participants