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

Add checkbox aria role to toggleable commands #149

Merged
merged 3 commits into from
Jan 19, 2021

Conversation

marthacryan
Copy link
Member

Follow-up to #129 to add the aria role of checkbox and the checked state to toggleable commands in the command palette.

@blink1073
Copy link
Contributor

Thanks for working on this! Is it still WIP?

@marthacryan
Copy link
Member Author

@manfromjupyter If you get a chance would you mind trying this one as well? I'm not able to get it working on my screen reader but I'm not very experienced with screen readers, so I might be missing something. I see the role and the aria-checked labels as attributes when I inspect in the browser.

@marthacryan marthacryan marked this pull request as ready for review January 15, 2021 18:14
@manfromjupyter
Copy link

@marthacryan absolutely. Still triple checking the other one, but after will jump on this one. How can I test? Same binder link just change the branch name? New to binder (but think it's super cool - thank you for setting that up.. makes it so easy to test things).

@marthacryan
Copy link
Member Author

@manfromjupyter Oh right I forgot this is in Lumino so it's a little more complicated to test. I think last accessibility meeting we were talking with @tonyfast about setting up the ability to create binders that open JupyterLab with changes made to Lumino but as far as I know we don't have anything like that working yet.

This section of the documentation https://jupyterlab.readthedocs.io/en/latest/developer/contributing.html#linking-unlinking-packages-to-jupyterlab explains how to set it up, but if you don't have the dev env set up it might be more work than it's worth (but if you are willing to do that, go ahead!). I do have a VM with NVDA set up and I could give testing it a second go, if you advise on how I should be doing it.

@manfromjupyter
Copy link

@marthacryan my time was a little limited so unfortunately wasn't able to get a dev environment up and running for this. Was a little confused by step 2: "Register a link to the modified external package". Does that mean lumino and if so what directory would that be in? Anyway I can try again on Tuesday if not, I can always test it once it hits JupyterLab or pre-release as there will undoubtedly be other things we can improve also in that general region, or perhaps, for just now until we/I find a more long term solution, could I have you just take a screenshot of the Inspect element view of the element when it is "checked" and then when it's not.

If the latter, I can reconstruct it super quick and could then get back to you by tonight potentially.

@marthacryan
Copy link
Member Author

@manfromjupyter Oh sorry yes the external package is a lumino package, but you only need to do the one that I'm changing here, which is widgets. So from lumino/packages/widgets, you'd run jlpm link, then from jupyterlab, you'd run jlpm link @lumino/widgets and then rebuild JupyterLab and make sure to run jupyter lab --dev-mode to see the changes.

Here's a screenshot though, all of the li elements that can be toggled have both role="checkbox" and aria-checked="{isToggled}"in the command palette ul (class list: "lm-CommandPalette-content p-CommandPalette-content"):
Here's checked:
image
Here's not checked:
image

@manfromjupyter
Copy link

manfromjupyter commented Jan 16, 2021

@marthacryan, sorry for the delay. Thank you for the history lesson and for the screenshots. Checked this out and it's perfect.

Will read for both states as follows for screenreaders:

  • "List of 9 items"
    • "Autosave Documented checkbox checked"
    • "Download"
    • "Open from Path..."
    • ...
    • "Trust HTML File checkbox unchecked"

(if I'm understanding correctly what elements can be checked here). Anyway, nice work. All good on my end.

@marthacryan
Copy link
Member Author

@blink1073 Looks like the label works, so if the code looks okay then it should be ready to be merged. (If anyone else wants to try locally and make sure they see role="checkbox" and aria-checked={true/false} on all toggleable commands, that'd be appreciated!)

Copy link
Contributor

@blink1073 blink1073 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!

@blink1073 blink1073 merged commit e9c5dce into jupyterlab:master Jan 19, 2021
@blink1073
Copy link
Contributor

@lumino/widgets@1.18.0 released

@marthacryan marthacryan deleted the checkbox branch April 23, 2021 15:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants