-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
transition header element to div.header for accessibility #9648
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
Code LGTM - @manfromjupyter Do you want to try out the changes? |
Thanks for working on this @tonyfast 🎉 |
@marthacryan I will gladly test it. Can jump on it in 2-3 hours probably. @tonyfast At a glance just from a UI/UX point of view I feel like |
@tonyfast I tested it and all works for accessibility with the exception of the debugger panel that I don't have in default Jupyter but has that same Also the top section has one as well for some reason: The rest is fine in terms of accessibility landmarks but could improve the experience for screenreaders by switching those divs to headings (e.g. |
totally agree we can have better names. a few of these headers have h2s in them. maybe one of the jupyter folks have better naming suggestions, i'm happy to make the changes.
darn! i thought i found them all! thanks for catching @manfromjupyter i pushed these changes to the debugger package. |
@tonyfast that region in terms of classes/ids is only uniquely labeled as the "Left stack" but agree the suggestion doesn't look great lol. Thank you for the debugger changes :) |
Speaking without any authority, Thanks for working on this @tonyfast and thanks to everyone else for reviewing it! |
agree that the prefix would be good to add. i think one of the things i want to avoid are spatial or any cardinal references because the side bars could be configured on the left or right. |
@tonyfast Agree, and smart. Perhaps just |
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.
LGTM! nice work tony
@manfromjupyter The code changes look clean enough to me, so if you think this PR I'll merge it in |
@telamonian Other than the debugger fixes that I think @tonyfast may have already submitted, it looks great. Thank you guys! |
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.
the debugger fixes
@manfromjupyter Thanks for pointing that out. I just checked the debugger panel, and it looks like a bit of styling was left out:
I'll try and fix that right now
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.
i fixed the titles in the debugger panels. i'll pull the PR in as soon as the CI runs
References
these changes remediate overlapping issues mentioned in #9491 #9399 #9622
these changes complement those in #9622 by @marthacryan and effect different files.
Code changes
there are
<header>
tags that impact usability with assistive devices. these changes use<div class="header">
as an alternative. the css style are updated accordingly.User-facing changes
There are no visual impacts on this PR, only benefits to folks with assistive devices.
Backwards-incompatible changes
🤷