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

[core] fix(MenuDivider): remove tabIndex attr to resolve a11y failure #6457

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

bvandercar-vt
Copy link
Contributor

@bvandercar-vt bvandercar-vt commented Oct 11, 2023

Does not need tabIndex={-1}, role="separator" already makes it non-focusable. Having any tabIndex (even -1) causes this to fail aXe a11y tests, so we must remove it.

@adidahiya
Copy link
Contributor

adidahiya commented Oct 12, 2023

This tabIndex was added to fix a bug, you can see that via git blame: #6062

Can you fix the a11y issue without causing a regression re: #6054?

edit: in fact we even discussed this here: #6054 (comment)

@bvandercar-vt
Copy link
Contributor Author

bvandercar-vt commented Oct 12, 2023

This tabIndex was added to fix a bug, you can see that via git blame: #6062

Can you fix the a11y issue without causing a regression re: #6054?

edit: in fact we even discussed this here: #6054 (comment)

Lol, I totally forgot I got involved in the discussion over this change... just to revert my decision later.

I do see that setting tabIndex={0} was the solution used for solving the #6054 bug, but I'm not sure why that worked, and I'm not sure if it was the ideal approach to solving that problem.

Regardless, it appears something has changed in the overall code between bug #6054 and now that has fixed the core of the problem— I cannot reproduce that bug, with or without my changes in this PR, and even if I remove the tabIndex. So, maybe that fix occurred between bp4 and bp5? Maybe it was fixed in Popover at some point? Either way, it's good news— it means that the core of the popover escape problem was solved, and we don't need the tabIndex anymore.

Here is a code sandbox from the latest blueprint codebase to show you: https://codesandbox.io/p/sandbox/jovial-matan-4w98xm?file=%2Fsrc%2FCoreExample.tsx%3A10%2C48 . As you can see, there is no tabIndex on the separator, and the bug no longer occurs.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

ok, great, thanks for that code sandbox @bvandercar-vt. It looks like it is safe to remove this attribute.

@adidahiya adidahiya changed the title a11y: remove tabIndex from separator [core] fix(MenuDivider): remove tabIndex attr to resolve a11y failure Oct 12, 2023
@adidahiya adidahiya merged commit c509634 into palantir:develop Oct 12, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants