-
Notifications
You must be signed in to change notification settings - Fork 60k
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
Fix accessibility issues on homepage and individual article pages #2160
Conversation
Thanks for opening this pull request! A GitHub docs team member should be by to give feedback soon. In the meantime, please check out the contributing guidelines. |
@barrymcgee Thanks so much for opening a PR! I'll get this triaged for review 💖 |
This PR is stale because it has been open 7 days with no activity and will be automatically closed in 3 days. To keep this PR open, update the PR by adding a comment or pushing a commit. |
Holiday break 🎅 |
This comment has been minimized.
This comment has been minimized.
This PR is stale because it has been open 7 days with no activity and will be automatically closed in 3 days. To keep this PR open, update the PR by adding a comment or pushing a commit. |
Bumping this once more as we're now just out of the holiday period and I'm hopeful it might get reviewed this week. |
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.
👋🏼 Hi @barrymcgee , this looks pretty great. I think this checks one or two boxes in #141 as well.
We have an automated accessibility script we run once a day, pa11y-ci
in the repo as well. I checked out this branch and ran npm run pa11y-test
on it. I'm seeing the form issue you've mentioned:
• This form does not contain a submit button, which creates issues for those
who cannot submit the form using the keyboard. Submit buttons are INPUT
elements with type attribute "submit" or "image", or BUTTON elements with
type "submit" or omitted/invalid.
(html > body > main > div:nth-child(1) > header > div:nth-child(2) > div >
div > div > div:nth-child(2) > div:nth-child(2) > form)
<form class="mb-0"> <div id="search-input-contai...</form>
I think this is coming from https://github.com/algolia/instantsearch.js/. That said, the actual form element in question is here:
https://github.com/github/docs/blob/main/includes/search-form.html So I think we could move up the aria-hidden
to the form element and no longer get that message.
But this is definitely an upgrade as is.
@heiskr Thank you for your review! I've made your suggested change and the |
It looks like the heading level change had the side effect of breaking a few "rendering" tests |
Headings should only ever increment by one https://dequeuniversity.com/rules/axe/4.1/heading-order
All page content must be contained by landmarks: https://dequeuniversity.com/rules/axe/4.1/region
This will provide semantic context to the unordered navigation list. Ref: All page content must be contained by landmarks: https://dequeuniversity.com/rules/axe/4.1/region
<ul> and <ol> must only directly contain <li>, <script> or <template> elements https://dequeuniversity.com/rules/axe/4.1/list
Main landmark must not be contained in another landmark https://dequeuniversity.com/rules/axe/4.1/landmark-main-is-top-level
This hides the whole form element as it's third party and not accessible.
@heiskr Sorry, now rebased and all fixed up. |
Thanks very much for contributing! Your pull request has been merged 🎉 You should see your changes appear on the site in approximately 24 hours. |
Thanks for the contribution @barrymcgee! |
Why:
Accessibility can nearly always be improved. This site was already built with accessibility clearly in mind but I noticed some remaining issues flagged in my aXe accessibility browser plugin so I thought I'd address them.
Fixes #2159
What's being changed:
aria-hidden
link wrapping the Github logo mark in the site header from the tab index so it's no longer keyboard accessible. This means keyboard users will skip straight to the next link around 'Github docs' which is the same.<nav>
sectioning element to confer semantic meaning.<ul>
's in the sidebar navigation<main>
elements on article pages - there should only be one per page.Each commit for each change has a link containing full supplementary rationale.
The biggest remaining issues are coming from the third party Agolia search box which is out of my control and there are also some color contrast issues but that will require collaboration and sign off from a designer.
Lighthouse Accessibilty score on production:
Lighthouse Accessibilty score on this Heroku demo:
Check off the following:
Not applicable:
For content changes, I have reviewed the localization checklistFor content changes, I have reviewed the Content style guide for GitHub Docs.