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

[EuiButtonGroup] Usability/accessibility enhancements #4684

Closed
cjcenizal opened this issue Apr 2, 2021 · 2 comments · Fixed by #4876
Closed

[EuiButtonGroup] Usability/accessibility enhancements #4684

cjcenizal opened this issue Apr 2, 2021 · 2 comments · Fixed by #4876
Assignees

Comments

@cjcenizal
Copy link
Contributor

I noticed a couple issues while playing with this component. Oddly enough, they only occur with the button group on the left in the screenshot below.

Missing focus state

When tabbing through the UI, it looks like there's no visual indicator for when a button in the button group is focused. I think this is a keyboard accessibility issue. This might be due to the component rendering a <label> element. Can we use a <button> instead? Looks like this was a direction taken in earlier improvements to accessibility for this component (#4056).

image

Missing hover state

Based on screenshots in #4142, I believe hover states should be supported for this component. If so, then maybe this is a regression? Not sure. Either way, I think we need hover states for the icon buttons in EuiButtonGroup. It would be even nicer if we rendered a tooltip on hover to explain what the button was for, since many people can find icons confusing or unclear.

@cjcenizal cjcenizal added accessibility ⚠️ needs validation For bugs that need confirmation as to whether they're reproducible labels Apr 2, 2021
@cchaos
Copy link
Contributor

cchaos commented Apr 2, 2021

Thanks for the feedback. As a quick note, #4056 is the PR where there was an extensive re-work of this component and it's underlying elements for accessibility. The type of group you use multi vs single determines what is rendered. Multi-select button groups are buttons, but in order for the single type to behave properly they had to be a radio group, hence the label.

We're also still working through some of the regressions with tab focus outlines in the Amsterdam theme where I think you're specifically seeing these issues.

As for tooltips, I agree and it would be nice to add these as a baked-in option but for now, you can simply duplicate the label as the title property to use the browser's tooltip. See this CodeSandbox: https://codesandbox.io/s/naughty-darkness-r0xuz

Screen Shot 2021-04-02 at 15 43 32 PM

@cchaos cchaos added Amsterdam and removed ⚠️ needs validation For bugs that need confirmation as to whether they're reproducible labels Apr 2, 2021
@cjcenizal
Copy link
Contributor Author

Thanks for the background and tip @cchaos! FWIW I just tested the k7 theme and it looks like hover states work fine, but the focus states are missing in that theme too. That's a great tip about providing the title property -- it'd be great to update the snippet to do that, too.

@cchaos cchaos self-assigned this Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants