-
Notifications
You must be signed in to change notification settings - Fork 261
Fix VoiceOver listing mobile menu toggles on desktop #4892
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
✅ You can preview this change here:
To edit notification comments on pull requests, go to your Netlify project configuration. |
971b5a2 to
66aa4bb
Compare
| /** | ||
| * Collection of attributes that needs setting on a `<button>` | ||
| * to fully hide it, both visually and from screen-readers, | ||
| * and prevent its activation while hidden | ||
| */ | ||
| const attributesForHidingButton = { | ||
| 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 To ensure we set an unset the same list of attributes, this object acts as the source of truth.
|
I've tried to see if it was a more general issue with Reduced test page source<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Document</title>
</head>
<body>
<h1>A web page</h1>
<button>A visible button</button>
<button hidden>A button with <code>hidden</code></button>
<button data-hide>A button to which <code>hidden</code> is added in JavaScript</button>
</body>
<script type="module">
// Hide an existing button
document.querySelector('[data-hide]').setAttribute('hidden', '')
// Add an already hidden button
const hiddenButton = document.createElement('button');
hiddenButton.textContent = 'A button'
hiddenButton.hidden = true
document.body.append(hiddenButton)
// Add a button then hides it immediately
const buttonImmediatelyHidden = document.createElement('button');
buttonImmediatelyHidden.textContent = 'A button'
document.body.append(buttonImmediatelyHidden)
buttonImmediatelyHidden.setAttribute('hidden', true)
// Add a button hidden at next frame
const buttonHiddenNextFrame = document.createElement('button');
buttonHiddenNextFrame.textContent = 'A button'
document.body.append(buttonHiddenNextFrame)
requestAnimationFrame(() => {
buttonHiddenNextFrame.setAttribute('hidden', true)
})
// Add a button hidden after 1s
const buttonHiddenNextSecond = document.createElement('button');
buttonHiddenNextSecond.textContent = 'A button'
document.body.append(buttonHiddenNextSecond)
setTimeout(() => {
buttonHiddenNextSecond.setAttribute('hidden', true)
}, 1000)
</script>
</html> |
@romaricpascal, I've just tested this in my iPad and that has indeed fixed the problem. 🎉 There is still the "menu" button that is visually hidden but VoiceOver can access. I assume that needs the same treatment. |
| * to fully hide it, both visually and from screen-readers, | ||
| * and prevent its activation while hidden | ||
| */ | ||
| const attributesForHidingButton = { |
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 wonder if there is a point in making this more global and generic? I assume every element that uses hidden would benefit from this?
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.
Yeah, thinking we may want a hideFormControl function and a corresponding showFormControl function. This feels like something for GOV.UK Frontend though, but it'd need to be in the public JS API for other teams (and our website to benefit from it). @36degrees does that sound like expanding our API too much?
🥳 🥳 🥳 Glad to hear it fixes things on the iPad as well
That one needs to be fixed in GOV.UK Frontend itself as that menu is managed by the Service Navigation component. That'll mean waiting for a release before we can benefit from it on the Design System site. For now, I've created an issue for the Service Navigation button. |
66aa4bb to
5b6f38d
Compare
Adds an `aria-hidden` attribute in complement to the `hidden` attribute already added to the toggles on desktop viewports. While in there, also adds a `disabled` attribute while the toggles are hidden, to prevent their activation in case another bug keeps them accessible (and to prevent their activation through `.click()` in JavaScript).
5b6f38d to
0a05b2f
Compare
On desktop, with Safari, prevents Voice Over's 'Form controls' section of the rotor from showing the toggles revealing/hiding the sections of the mobile menu.
Note
Given the markup, only the buttons have extra aria attributes. The other hidden elements from that component are:
That makes me think we're OK in terms of what Voice Over sees
How it works
Adds an
aria-hiddenattribute in complement to thehiddenattribute already added to the toggles on desktop viewports.While in there, it also adds a
disabledattribute while the toggles are hidden, to prevent their activation in case another bug keeps them accessible (and to prevent their activation through.click()in JavaScript) as triggering a programmatic click leads to a fairly broken display on wide viewports.Thoughts
@selfthinker This might partially address #4879. A similar fix to the Service Navigation should be applied in GOV.UK Frontend so the button toggling the whole mobile menu is appropriately hidden from VoiceOver.