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.

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

Conversation

joe-replin
Copy link
Contributor

@joe-replin joe-replin commented Jul 5, 2023

Fix

Testing

  1. Tab through drawer until drawer heading is heard
  2. Test that the aria-label is the same within Firefox and then Chrome for good measure

@joe-replin joe-replin added bug Something isn't working accessibility labels Jul 5, 2023
@joe-replin joe-replin self-assigned this Jul 5, 2023
Copy link
Contributor

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>
Copy link
Member

@oliverfoster oliverfoster Jul 17, 2023

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.

a11y.focusFirst(this.$el, { defer: true });

It should be. Maybe the defer needs changing to false?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility bug Something isn't working
Projects
Status: Awaiting Response
Development

Successfully merging this pull request may close these issues.

Drawer header label not announced
4 participants