Fix navigation links in the header not being announced by screen readers #1967
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1967
In 18771ce (released as part of v3.9.0) we changed the logic of the header javascript to set the aria-expanded and aria-hidden attributes on the menu button and navigation respectively. However, the logic is applied at all breakpoints, not just the mobile breakpoint where the navigation is hidden, and so the navigation is currently not visible to screen reader users. As noted by @adamsilver, in this instance when the navigation is hidden it is already removed from the accessibility tree through the use of `display: none;`. As such, the `aria-hidden` attribute is redundant and can be removed: > If an interactive element is hidden using display:none or visibility:hidden (either on the element itself, or any of the element's ancestors), it won't be focusable, and it will also be removed from the accessibility tree. This makes the addition of aria-hidden="true" or explicitly setting tabindex="-1" unnecessary. > > https://www.w3.org/TR/using-aria/#4thrule This means that the only aria attribute we need to keep in sync is the `aria-expanded` attribute – which is applied to the menu control which is hidden when the menu is not collapsible, so it being ‘out of sync’ on the desktop breakpoint is not an issue. Fixes #1966.
36degrees
force-pushed
the
header-aria-hidden
branch
from
c85a332 to
98ccef3
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1967
36degrees
changed the title
vanitabarrett
approved these changes
Sep 22, 2020
hannalaakso
approved these changes
Sep 22, 2020
Merged
|
The turnaround time on this issue is really impressive! Thanks for fixing @36degrees! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 18771ce (released as part of v3.9.0) we changed the logic of the header javascript to set the aria-expanded and aria-hidden attributes on the menu button and navigation respectively.
However, the logic is applied at all breakpoints, not just the mobile breakpoint where the navigation is hidden, and so the navigation is currently not visible to screen reader users.
As noted by @adamsilver, in this instance when the navigation is hidden it is already removed from the accessibility tree through the use of
display: none;. As such, thearia-hiddenattribute is redundant and can be removed:This means that the only aria attribute we need to keep in sync is the
aria-expandedattribute – which is applied to the menu control which is hidden when the menu is not collapsible, so it being ‘out of sync’ on the desktop breakpoint is not an issue.Fixes #1966.