-
Notifications
You must be signed in to change notification settings - Fork 18
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: Create aria-label for drawer heading (Fixes: #400) #401
base: master
Are you sure you want to change the base?
Conversation
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.
👀
@@ -1,6 +1,6 @@ | |||
<div class="drawer__inner"> | |||
|
|||
<span id="drawer-heading" class="aria-label">{{_globals._accessibility._ariaLabels.drawer}}</span> | |||
<span id="drawer-heading" class="aria-label" aria-label="{{_globals._accessibility._ariaLabels.drawer}}"></span> |
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.
This isn't how these work.
You shouldn't put an aria-label
attribute on a non-focusable element.
This is exactly why we have <span>aria label</span>
elements, visually hidden with the .aria-label
class, such that they read in place as normal text.
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.
<span class="aria-label">hidden text</span>
is fine as is as we use this elsewhere in Adapt. I think we typically have the two use cases:
1, injecting additional context for on screen text (see Slider example).
2, to provide a label for a parent element which we want to do here.
I can replicate the issue reported on Mac running voiceOver in FF. Looking into this, Drawer accessibility is handled similar to Notify. Comparing the two, I've noticed Notify sets focus to the first accessible element on opening but Drawer doesn't. Applying the same to Drawer resolves the issue for me. @oliverfoster, do you see any issues with doing this?
Setting focus on showDrawer:
if (!this._isVisible) {
a11y.popupOpened(this.$el);
a11y.scrollDisable('body');
this._isVisible = true;
// Set focus to first accessible element
a11y.focusFirst(this.$('.drawer__inner'), { defer: false });
}
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.
adapt-contrib-core/js/views/drawerView.js
Line 161 in 3f9b44d
a11y.focusFirst(this.$el, { defer: true }); |
It should be. Maybe the defer needs changing to false?
Fix
Testing