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

Accessibility: role attributes #507

Merged
merged 3 commits into from
Dec 30, 2022
Merged

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Dec 29, 2022

Related to jupyter/notebook#6671.

Changes the roles of (1) accordion title and (2) toggleable items of command palette according to the W3C recommendation .

@welcome
Copy link

welcome bot commented Dec 29, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@krassowski
Copy link
Member

Regarding the command palette, the specs say:

Authors MUST ensure that menu item checkboxes are owned by an element with role menu or menubar in order to identify that they are related widgets. Authors MAY separate menu items into sets by use of a separator or an element with an equivalent role from the native markup language.

If I am reading this correctly, changing the role of checkbox to menuitemcheckbox may not suffice to fix the issue here. I would agree that command palette can be seen as a menu but then it currently has role="region" in JupyterLab (on . lm-CommandPalette). I think we should consider adding role="menu" on .lm-CommandPalette-content (as the searchbar is not a menu itself). Additionally we have .lm-CommandPalette-header (not sure if those count as separators).

@brichet
Copy link
Contributor Author

brichet commented Dec 29, 2022

Thanks @krassowski for reviewing it.

You're right, we should add the role="menu" on .lm-CommandPalette-content, and probably also a role="menuitem" for all those without checkbox.

Not sure about the header, maybe we can leave it without role.

@krassowski krassowski added bug Something isn't working accessibility Addresses accessibility needs labels Dec 29, 2022
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@krassowski krassowski merged commit 16a9f4a into jupyterlab:main Dec 30, 2022
@welcome
Copy link

welcome bot commented Dec 30, 2022

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@brichet brichet deleted the accessibility branch January 17, 2023 07:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Addresses accessibility needs bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants