mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
94.13k stars 32.35k forks source link

unnecessary to always have tabindex=0 on <button /> in html #29169

Open yuanwei0525 opened 3 years ago

yuanwei0525 commented 3 years ago

Current Behavior 😯

Following code <Button variant="contained">Contained</Button> renders image

Expected Behavior πŸ€”

If tabIndex is not provided to component Button, it is unnecessary to render tabindex=0 in result html since button is tab-able by default, unless a number is provided to Button as parameter.

Steps to Reproduce πŸ•Ή

Steps:

  1. go to https://mui.com/components/buttons/
  2. inspect button from one of example

Context πŸ”¦

Accessibility

Your Environment 🌎

Chrome browser

mnajdova commented 3 years ago

But this is not breaking something tough right? It's safer to always have it, as it may be rendered as another element too. We could remove it, but why complicate the logic.

Is there something that is not working for your use-case?

yuanwei0525 commented 3 years ago

It does not break anything but redundant to have. Our team is working on a project using mui and our accessibility team recommended us to remove tabindex on button element.

I was curious about how other popular UI frameworks render button component and noticed bootstrap, semantic ui, ant, elemental ui do not render tabindex to button element in html. There might be more because I did not check every framework.

Reference:

sandyClark603 commented 2 years ago

Same issue here. My developers have been trying to circumvent by sending an empty tabindex over, but that just makes it worse.

Focusable Items are already focusable. The only time to add a TabIndex is to bring a non-focusable Item into focus or to remove a focusable item from focus. Having a tabIndex ="0" on an HTML Semantic Element is redundant and should be removed.

danielcavanagh commented 1 year ago

for anyone who's interested in a reasonable workaround, if you assign null to tabIndex it will not appear in the html. the props type doesn't accept null but you can workaround that with a cast

<Button tabIndex={ null as unknown as undefined }>no tabindex!</Button>

i think at the very least the type should be changed to accept null

KittyGiraudel commented 1 year ago

~It turns out that having tabindex="0" on all buttons solves a pretty significant yet wontfix bug from Safari.~ Basically Safari does not assign the <button> node to document.activeElement, which has serious accessibility repercussions for dialog libraries like a11y-dialog. ~If the button has a non-null tabindex attribute however, Safari does behave as expected. πŸ™ƒ~

In other words: considering it doesn’t cause any problem to have it on every button, and since it can both help when using polymorphic components ~and as a workaround for a 15 years old focus bug in a major browser~, Iβ€˜d say keep it. :)

Edit: My bad, I spoke too fast. :(

danielcavanagh commented 1 year ago

ok. that's good info. if the current behaviour avoids an issue in safari then it seems reasonable to keep it

however this was actually causing an issue for us, which is why we had to work around it. it's not just a tidyness issue or whatever. the only problem is i can't remember what the issue was right now... πŸ˜†

at the very least null should be passable without a cast