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

fix(OverflowMenu): remove default tabindex and SVG focusable attribute #6976

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Oct 6, 2020

Closes #6935

This PR removes the redundant default tabindex value on the overflow menu trigger button and also removes the redundant focusable attribute from the icon props. It looks like the focusable="false" was meant to prevent extra tab stops in IE11, but I was unable to replicate that issue

Changelog

Removed

  • default tabindex value
  • focusable attribute on trigger button SVG

Testing / Reviewing

Confirm that the trigger button no longer receives a tabindex value by default and that there are no changes to focus behavior and tab stops particularly in IE11

@emyarod emyarod requested a review from a team as a code owner October 6, 2020 14:23
@emyarod emyarod force-pushed the 6935-overflowmenu-unnecessary-a11y-attrs branch from 5876afe to d7af850 Compare October 6, 2020 14:23
@netlify
Copy link

netlify bot commented Oct 6, 2020

Deploy preview for carbon-elements ready!

Built with commit 5876afe

https://deploy-preview-6976--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 6, 2020

Deploy preview for carbon-components-react ready!

Built with commit 5876afe

https://deploy-preview-6976--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Oct 6, 2020

Deploy preview for carbon-elements ready!

Built with commit 1fcf335

https://deploy-preview-6976--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 6, 2020

Deploy preview for carbon-components-react ready!

Built with commit 1fcf335

https://deploy-preview-6976--carbon-components-react.netlify.app

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Seems like this has broken keyboard controls for the menu itself on Firefox latest
image

@emyarod
Copy link
Member Author

emyarod commented Oct 8, 2020

that was a separate existing issue fixed in #6978, try updating your branch to pull in the latest commits to Carbon

@joshblack
Copy link
Contributor

bump @dakahn @tw15egan when you get a sec today 👀

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@emyarod emyarod force-pushed the 6935-overflowmenu-unnecessary-a11y-attrs branch from 18dd89c to 724922a Compare October 14, 2020 15:48
@tw15egan tw15egan requested a review from dakahn October 14, 2020 20:14
@carmacleod
Copy link
Contributor

carmacleod commented Oct 20, 2020

For the record, it was not the focusable=false on the SVG that was unnecessary, it was the focusable="false" on the span inside the SVG. A google of "SVG focusable=false IE11" still finds a lot of hits, so you may still need focusable=false on the SVG itself. (Note that the bug was fixed in old Edge, but as far as I know it was never fixed in IE11, so I'm surprised that you didn't see an extra tab stop in IE11)

@emyarod
Copy link
Member Author

emyarod commented Oct 20, 2020

yeah the SVG itself still has focusable=false by default since that attribute is added when we generate our icons

@emyarod emyarod deleted the 6935-overflowmenu-unnecessary-a11y-attrs branch October 20, 2020 16:36
@carmacleod
Copy link
Contributor

Ah, ok. Thanks for the explanation. I was confused by the title and opening comment in this PR. :)
I just tested whether SVGs in IE11 still need the focusable=false, and they definitely still need it.
Here's my little test case: https://carmacleod.github.io/playground/svg-test.html
(Just FYI at this point, since you didn't actually delete the focusable=false from the SVG itself 😄 )

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.

Unnecessary focusable and tabindex in an OverflowMenu component
5 participants