Set aria-expanded and aria-hidden attributes on header menu button and menu when page loads #1942
Conversation
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1942
36degrees
changed the title
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1942
There was a problem hiding this comment.
Nice improvements 👍 Left one comment which could be good to address.
Try as I might, I wasn't able to test this on Mac Voiceover to see whether the issue reported by the user in #1932 is resolved. However I think this might be something to do with my VoiceOver (Safari 13.1.2) as I couldn't get it to read the collapsed state of some other things either. And it was also late 😅 So as long as someone else has been able to check the behaviour on Mac VoiceOver that's good for me.
|
|
||
| function Header ($module) { | ||
| this.$module = $module | ||
| this.$menuButton = $module && $module.querySelector('.govuk-js-header-toggle') | ||
| this.$menu = this.$menuButton && document.getElementById( |
There was a problem hiding this comment.
Using document.getElementById makes me a bit wary as there's a chance that we'd be grabbing something unrelated with it from the DOM. Could we get the menu using $module here?
There was a problem hiding this comment.
I've updated this to use $module.querySelector( '#' + this.$menuButton.getAttribute('aria-controls')) but I'd like to update this in 4.0 alongside #1809 and #1808?
I think generally where we're getting an element using an ID we should be able to assume that there will only be one element on the page with that ID, but agree that it could be a breaking change in some situations, where users currently have elements with duplicate IDs.
There was a problem hiding this comment.
👍 for the fix. As just discussed offline I'm not 100% sure about changing this to document.getElementById in 4.0 but we can discuss it down the line.
36degrees
force-pushed
the
header-aria-fixes
branch
from
9519665
to
df98431
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1942
'Flipping' the different state attributes individually allows for them to get out of sync with each other. Instead, define the visible 'state' once, based on the return value from classList.toggle (true if the `--open` modifier is now present, false if it's removed) and set the other attributes based on this.
Expose the hidden / collapsed state of the navigation on page load, rather than only after the menu button to be interacted with for the first time. Find the menu button and menu when the module is initialised and store them in the instance so we can easily sync them without having to pass references to the button and menu between functions. Add tests to check the aria state of the menu and menu button when the page is first loaded.
- Perform navigations once in a beforeAll, rather than in every test individually - Use page.$eval rather than page.evaluate - Re-format to avoid long lines - Improve test descriptions
36degrees
force-pushed
the
header-aria-fixes
branch
from
df98431
to
4e77059
Compare
Expose the hidden / collapsed state of the navigation on page load, rather than only after the menu button to be interacted with for the first time.
I've ended up re-writing most of the component to try and simplify it, and to avoid duplicating any logic that sets the aria attributes.
I've also updated the tests to use
page.$evaland to avoid unnecessary navigations, by navigating and interacting with the menu button in a singlebeforeAllblock and then making the assertions in individual tests.Fixes #1932