-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Add ARIA roles to tabs - lumino update #132
Add ARIA roles to tabs - lumino update #132
Conversation
According to experts (including both an invited expert advisor and a co-chair of ARIA committee), aria-controls is not needed, and actually annoying in JAWS.
Also use the tab bar orientation setter to set the default orientation.
Before, we needed to clamp down on these types for aria attributes. This is (a) backwards incompatible and (b) not needed anymore.
Just tried out these changes in JupyterLab - it does add aria roles to the tab bars, although I think there is overlap with this PR: jupyterlab/jupyterlab#9622 that might cause issues. Should we move those changes over to this PR instead? Looks like we'd be able to pass the There's also a comment on the other PR about an issue with the way these tabs are created that might be good to address as part of this PR: jupyterlab/jupyterlab#9622 (comment) @manfromjupyter I know you can't try these changes out easily so here's what attributes this PR adds to the tab bars:
And these attributes are added to the tabs:
Should there be an |
@marthacryan, I am assuming you are referring to this tablist in the screenshot below. If so it doesn't need the aria-orientation. The sidebars shouldn't receive this as their order does not matter to screereaders. It's vertical in design but screenreaders read it horizontally (it's just an unordered list of buttons) and reading it is read the same way (with a down arrow or an eventual tab once we get that working in one of the other open tickets). The I agree the The example they provide. Only thing that should change, in my opinion, is the tabs (buttons) in this example have tabindex="-1" but they should still be reachable by the screenreader in case the user wanted to switch to a different tab.
Side Note: The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion on the conflicts between this PR and the JupyterLab PR, I think this PR's aria labels are correct and this PR is ready to merge. I tested locally and it worked well - we should still look into the issue with tab bars I mentioned above, but that can be in a later PR. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
From the original PR:
This PR takes all of the work done for phosphorjs/phosphor#406, a PR onto the old phosphor repo, and rebases it onto the latest lumino master.
This PR covers all of the following commits: jasongrout/phosphor@aria-menus...jasongrout:aria-tabs. In theory, all of the other commits listed in phosphorjs/phosphor#406 should be covered by #131, which merged all of the work from phosphorjs/phosphor#405 into lumino.
@jasongrout Can you please take a look and make sure this reflects your original work? Also, what would be a good way to double check that this work is still working as intended in the context of lumino?