search

elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
6.08k stars 826 forks source link

[EuiTabs ][A11Y] - Using `EuiBetaBadge` within `EuiTab` #7791

Closed alexwizp closed 3 months ago

alexwizp commented 3 months ago

Related to: https://github.com/elastic/observability-dev/issues/3410

We have an issue with using EuiBetaBadge within EuiTab. The AXE plugin triggers a problem: interactive controls must not be nested.

I need your guidance on how to correctly use EuiBetaBadge within EuiTab. I couldn't find a good way to fix this with the existing API. Here are my thoughts on how we can address it:

  1. Extend the EuiBetaBadge props to allow overriding the role="button" attribute.
  2. Extend the EuiTab API to include betaBadgeProps. This would allow us to handle it more natively, similar to how it's done with EuiCards and EuiKeyPadMenuItems.

I vote for option number 2

cee-chen commented 3 months ago

Option 1 should already be fully achievable via ...rest spread, e.g. <EuiBetaBadge role="presentation" /> or even role={undefined}.

Option 2 should not be necessary as the tab itself is not rendering a beta badge directly, it's using an append or prepend API where the consumer is rendering <EuiBetaBadge /> themselves and needs to apply the correct role type. Example:

https://codesandbox.io/p/sandbox/holy-surf-4cj9kk?file=%2Fdemo.js%3A30%2C7

I will note that if these tabs are using the toolTipContent prop on EuiBetaBadge and isn't just rendering text/icons, then something else needs to be done because that tooltip will not be keyboard accessible. We'll need to add a toolTipContent/toolTipProps on the entire EuiTab component/object.

alexwizp commented 3 months ago

@cee-chen Thank you for your answer. I've checked the EUI code, and I have a quick question about the role=button attribute. Not sure that we need it for that case. The element is not actually a button/link, and I'm considering removing the attribute. If you're okay with this change, I can create a PR to remove it.

cee-chen commented 3 months ago

role="button" and tabIndex={0} is meant to be there for scenarios where tooltipContent exists, so that keyboard users can tab to it to trigger the tooltip. I had previously thought the button role was helpful to indicate tab-ability, is that not correct? If not, I'm fine removing it.

https://github.com/elastic/eui/blob/5c4031565d1f0fef7e08e2377ea5b29bf8963b54/packages/eui/src/components/badge/beta_badge/beta_badge.tsx#L234-L237