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

transition header element to div.header for accessibility #9648

Merged
merged 9 commits into from
Feb 8, 2021

Conversation

tonyfast
Copy link
Contributor

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

🤷

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@github-actions github-actions bot added Design System CSS pkg:extensionmanager pkg:running tag:CSS For general CSS related issues and pecadilloes labels Jan 20, 2021
@marthacryan
Copy link
Member

Code LGTM - @manfromjupyter Do you want to try out the changes?

@goanpeca
Copy link
Member

Thanks for working on this @tonyfast 🎉

@manfromjupyter
Copy link
Contributor

@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 header shouldn't be a class but it technically breaks no rules. In my mind it should be actual headings ex. <h2>'s probably so screenreaders can jump around by heading and it's better and more accurate HTML anyway, not just everything styled to look like it. If we were insistent on using classes, that's fine, just think a more detailed classname might want to be used, ex. .left-stack-panel-header or .side-header or something else. Or... perhaps more ideally an h2 and said class name so this can be styled independently from other site <h2> 's

@manfromjupyter
Copy link
Contributor

manfromjupyter commented Jan 21, 2021

@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 <header> improper html in there. They also already have a <h2> there so need to consolidate and do some cleanup there probably if the header providers no additional benefit or convert those specific ones to divs.
headers found in debugger section

Also the top section has one as well for some reason:
also top section

The rest is fine in terms of accessibility landmarks but could improve the experience for screenreaders by switching those divs to headings (e.g. <h2> 's) and then have a class on there for stylesheets and theme to easily customize for.

@tonyfast
Copy link
Contributor Author

more detailed classname might want to be used, ex. .left-stack-panel-header or .side-header or something else. Or... perhaps more ideally an h2 and said class name so this can be styled independently from other site <h2> 's

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.

for accessibility with the exception of the debugger panel that I don't have in default Jupyter but has that same <header> improper html in there. They also already have a <h2> there so need to consolidate and do some cleanup there probably if the header providers no additional benefit or convert those specific ones to divs.

darn! i thought i found them all! thanks for catching @manfromjupyter i pushed these changes to the debugger package.

@manfromjupyter
Copy link
Contributor

manfromjupyter commented Jan 21, 2021

@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 :)

@isabela-pf
Copy link
Contributor

Speaking without any authority, .left-stack-panel-header doesn't feel too far off in terms of names. Taking a look, it seems like this might need a jp prefix, and I agree with @manfromjupyter that since left-stack is unique to the area it would make sense to continue that label where relevant.

Thanks for working on this @tonyfast and thanks to everyone else for reviewing it!

@tonyfast
Copy link
Contributor Author

Speaking without any authority, .left-stack-panel-header doesn't feel too far off in terms of names. Taking a look, it seems like this might need a jp prefix, and I agree with @manfromjupyter that since left-stack is unique to the area it would make sense to continue that label where relevant.

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.

@manfromjupyter
Copy link
Contributor

@tonyfast Agree, and smart. Perhaps just stack-panel-header or stack-header?

Copy link
Member

@telamonian telamonian left a 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

@telamonian
Copy link
Member

@manfromjupyter The code changes look clean enough to me, so if you think this PR I'll merge it in

@manfromjupyter
Copy link
Contributor

@telamonian Other than the debugger fixes that I think @tonyfast may have already submitted, it looks great. Thank you guys!

Copy link
Member

@telamonian telamonian left a 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:

image

I'll try and fix that right now

Copy link
Member

@telamonian telamonian left a 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

@telamonian telamonian merged commit 5340e71 into jupyterlab:master Feb 8, 2021
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jan 18, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design System CSS enhancement pkg:debugger pkg:extensionmanager pkg:running status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:CSS For general CSS related issues and pecadilloes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants