-
Notifications
You must be signed in to change notification settings - Fork 347
Fix VoiceOver access to hidden Service Navigation menu button
#6342
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
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for f714395 |
d8088e5 to
f2446a2
Compare
aria-controls when Service Navivation menu is visiblehidden Service Navigation menu button
romaricpascal
left a comment
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.
@36degrees The issue also affects the button in the Header component. Given its navigation is deprecated, though, I'm not sure if we want to publish a fix for it there or not. Let me know and it should be fairly easy to port the code.
|
|
||
| // Wait that the page got updated after the resize, | ||
| // as sometimes the tests run too early | ||
| await page.waitForSelector(`${navigationSelector}[hidden]`) |
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.
note This effectively doubles up with the first test within the describe, but I couldn't find another selector to wait on to guarantee that the media query listener had ran.
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.
Is it worth expanding the comment to note that that's why there are 2 instances of await selector[hidden]?
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.
Whoops, that's actually a copy paste in that first test. The selector in the first test should just be navigationSelector not `${navigationSelector}[hidden]`, I'll update.
| const attributesForHidingButton = { | ||
| hidden: '', | ||
| // Prevent activating the button with JavaScript APIs while hidden | ||
| disabled: '', |
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.
note Happy to drop this one if we feel it's over the top.
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'm leaning towards saying delete it and we save a handful of bytes. I'm interested to know from you if this is a reckon or if there's a potential case of this getting activated via js whilst hidden.
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've ported that from the Design System's menu navigation, where the buttons controlling the visibility of the sub-sections causing chaos if you activated them in JavaScript while hidden on desktop (revealing sub-navigation items which wrecked the horizontal service nav).
Given the menu button shows/hide the items of the service nav themselves, and these items are already shown on desktop, we can remove it here.
owenatgov
left a comment
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.
A few comments and responses to your comments. This also needs a changelog entry. I'd recommend after addressing my recommendation that this be a recommended change with a view to apply it full in v7. I'm mainly thinking about us backporting this to 5.14 and keeping v6's breaking changes lean. Happy to discuss further off github.
| const attributesForHidingButton = { | ||
| hidden: '', | ||
| // Prevent activating the button with JavaScript APIs while hidden | ||
| disabled: '', |
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'm leaning towards saying delete it and we save a handful of bytes. I'm interested to know from you if this is a reckon or if there's a potential case of this getting activated via js whilst hidden.
| await page.emulate(iPhone) | ||
| }) | ||
|
|
||
| describe('on page load', () => { |
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.
Depending on the outcome of the other thread, we'd also want a test checking for disabled here as well
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.
Good spot!
|
|
||
| // Wait that the page got updated after the resize, | ||
| // as sometimes the tests run too early | ||
| await page.waitForSelector(`${navigationSelector}[hidden]`) |
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.
Is it worth expanding the comment to note that that's why there are 2 instances of await selector[hidden]?
| } | ||
|
|
||
| const menuId = $menuButton.getAttribute('aria-controls') | ||
| const menuId = $menuButton.getAttribute('data-aria-controls') |
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.
When testing locally, if you remove data-aria-controls then the class aborts early as it can't find the button, meaning the button doesn't show and the show/hide functionality doesn't get applied.
Could we also check for aria-controls still to avoid this being a breaking change?
| const menuId = $menuButton.getAttribute('data-aria-controls') | |
| const menuId = $menuButton.getAttribute('data-aria-controls') ?? $menuButton.getAttribute('aria-controls') |
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 aria-controls is unfortunately the source of the issue, as it's what makes VoiceOver still maintain access to the button even when it has the hidden attribute. This means the button cannot have the aria-controls attribute in the HTML.
I'd like to propose a different approach where the button is not in the HTML at all and injected by JavaScript, using the ID of the menu to fill aria-controls. However, that's a whole separate piece of work.
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.
Unfortuantely unless we're willing to release that breakage and stop the mobile menu working then we'll have to delay this being resolved 😕 It also means we can't backport this, but it sounds like we can't anyway since this either will break the design on mobile or won't actually solve the problem.
Sounds like this isn't backportable but we should make a call now on if we release it in v6 or not.
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 think the JavaScript change remains backportable if it uses aria-controls instead of the new data-aria-controls for v5. It's not as thorough a fix as for v6 as if JavaScript does not load, the hidden menu button is still accessible through Voice Over, but still an improvement on the situation that it gets hidden properly once JavaScript kicks in 🤔
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.
Regarding v6, I guess our choices are:
- implement your proposition to smoothen the upgrade path. Users who haven't upgraded their Service Navigation's HTML to use
data-aria-controlswould get a fix when JavaScript is loaded, but not if it doesn't load. Then in v7, we'd remove the support foraria-controls. - take advantage of the breaking release in v6.0, potentially preceeded by a 5.14 release to completely remove the support for the button having
aria-controls
I'll bring this up at dev catch-up as we're once more in the blurry land of deprecations at the time of a breaking release.
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.
Trying to write a CHANGELOG for this to guide users' updates, I'm wondering if it'd be easier to remove the button than the attribute 😓
We could still ease the update path by checking if a button already exists, and backport to v5 with the same caveat that the fix is only when JavaScript has loaded.
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 thing I'm still not 100% is if it's an incrimental improvement or if the fact that aria-controls will still be there in v5 means that it's almost not worth doing. Thinking about your comment above:
The aria-controls is unfortunately the source of the issue, as it's what makes VoiceOver still maintain access to the button even when it has the hidden attribute. This means the button cannot have the aria-controls attribute in the HTML.
For the purpose of having a way forward, my proposal is:
- proceed with this change with the smoothed upgrade path as a fix that incriments the issue but doesn't solve it
- backport as planned
- pin a change to v7 to remove that button
romaricpascal
left a comment
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.
Cheers for the review @owenatgov . I'll make the couple of updates (dropping the disabled and fixing the selector in the first text) on Thursday hopefully and we can merge this. I'll port the changes on the v5 branch as well 😊
| } | ||
|
|
||
| const menuId = $menuButton.getAttribute('aria-controls') | ||
| const menuId = $menuButton.getAttribute('data-aria-controls') |
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 aria-controls is unfortunately the source of the issue, as it's what makes VoiceOver still maintain access to the button even when it has the hidden attribute. This means the button cannot have the aria-controls attribute in the HTML.
I'd like to propose a different approach where the button is not in the HTML at all and injected by JavaScript, using the ID of the menu to fill aria-controls. However, that's a whole separate piece of work.
| const attributesForHidingButton = { | ||
| hidden: '', | ||
| // Prevent activating the button with JavaScript APIs while hidden | ||
| disabled: '', |
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've ported that from the Design System's menu navigation, where the buttons controlling the visibility of the sub-sections causing chaos if you activated them in JavaScript while hidden on desktop (revealing sub-navigation items which wrecked the horizontal service nav).
Given the menu button shows/hide the items of the service nav themselves, and these items are already shown on desktop, we can remove it here.
| await page.emulate(iPhone) | ||
| }) | ||
|
|
||
| describe('on page load', () => { |
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.
Good spot!
|
|
||
| // Wait that the page got updated after the resize, | ||
| // as sometimes the tests run too early | ||
| await page.waitForSelector(`${navigationSelector}[hidden]`) |
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.
Whoops, that's actually a copy paste in that first test. The selector in the first test should just be navigationSelector not `${navigationSelector}[hidden]`, I'll update.
This avoids the button to be listed in VoiceOver's rotor if JavaScript doesn't run. https://bugs.webkit.org/show_bug.cgi?id=300899
In anticipation of testing the presence of `aria-hidden`
`aria-expanded` and `aria-controls` make VoiceOver keep the button visible in its rotor, so we add `aria-hidden` as a complement to `hidden`. This is prefered to manually adding and removing each of `aria-expanded` and `aria-controls` as we'd need to remember their value when resetting them. It also future proofs in case other `aria` attributes make VoiceOver keep the button in the rotor.
f2446a2 to
f714395
Compare
Because of the
aria-controlsandaria-expandedattributes on it, VoiceOver on wide enough screens:To work around this without having to manage each
aria-...attribute individually, this PR:aria-controlfrom being in the HTML before JavaScript kicks in, in case JavaScript fails to loadaria-hiddenin complement to thehiddenattribute to hide the button from VoiceOverWhile in the space of 'fully hidding the button', it also adds a
disabledattribute so the button cannot be clicked programmatically in JavaScript.Technical considerations
In order to maintain a single source of truth for the attributes being updated, the PR introduces introduces a little object with the attributes to set on the
<button>which is then used to set or unset attributes in bulk through two small helpers.The PR also adds an extra set of tests for the component around how it reacts to the screen being resized, as it is during the resizes that the menu button gets shown or hidden.
Fixes #6294