Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix: Create aria-label for drawer heading (Fixes: #400) #401

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion templates/drawer.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
Copy link
Member

@oliverfoster oliverfoster Jul 17, 2023 •
edited
Loading

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.

Copy link
Contributor

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 });
    }

Copy link
Member

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?


<div class="drawer__toolbar">

Expand Down