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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collapsed content is still visible to screen readers #12

Closed
bobwise opened this issue Mar 24, 2020 · 6 comments
Closed

Collapsed content is still visible to screen readers #12

bobwise opened this issue Mar 24, 2020 · 6 comments
Assignees

Comments

@bobwise
Copy link

bobwise commented Mar 24, 2020

Hello! 馃憢

We're using dom-slider in @sparkdesignsystem and we're running into an accessibility issue. Because dom-slider does not use display: none when hiding the content, the content is still available to screen readers when it should be hidden. This is only the case after the contents are hidden by dom-slider; you may need to expand and then collapse a component to see this issue in action.

Steps to Reproduce using VoiceOver on macOS

  1. Open example.html in a browser
  2. Turn on VoiceOver (Cmd+F5, by default)
  3. Refresh example.html to put VoiceOver focus in the page.
  4. Use the VoiceOver navigation (Ctrl+Option+Arrows, by default) to scroll around the page. Note that the collapsed contents are not read by the screen reader.
  5. Expand and Collapse a toggle using the VoiceOver trigger (Ctrl+Option+Space, by default)
  6. Scroll around with VO navigation again and observe that the contents are still visible to screen readers even when hidden.
@BrentonCozby
Copy link
Owner

BrentonCozby commented Mar 25, 2020

Hi @bobwise

Thanks for the awesome bug recreation steps. I'll be honest...I forget why I chose not to use display: none when I first made this.

That being said, I've got three thoughts on how to solve this. Could you help me through this?

  1. Use display: none
  2. Provide a new option to each of the dom-slider methods: a boolean named hideWithDisplayNone
  3. Provide an init or config function that receives a config object with options including hideWithDisplayNone; this init/config function would apply to all the methods all the time so you don't have to use my second solution every time you use a dom-slider method

Thoughts?

@BrentonCozby BrentonCozby self-assigned this Mar 25, 2020
@bobwise
Copy link
Author

bobwise commented Mar 25, 2020

Hey @BrentonCozby -

I suspect setting display:none might interfere with the opening/closing animation. That may be a reason to not add it.

I think we may actually be able to get the same result for screen readers by adding aria-hidden='true' to the element when we hide it. This will remove it and its descendants from the accessibility tree, which is exactly what we want for a collapsed disclosure like this.

Something like this:

if (slideDirection === 'down') {
   // set aria-hidden='false' on the content
   element.setAttribute('aria-hidden', 'false');
}
if (slideDirection === 'up') {
   // set aria-hidden='true' on the content
   element.setAttribute('aria-hidden', 'true');
}

I just tested this out locally and at first glance it looks like it works.

@BrentonCozby
Copy link
Owner

@bobwise

What you said about display: none sounds familiar. I suspect you're correct.

Anyway, good find on aria-hidden. I don't see this breaking anything, so I'll create a PR for it.

Thanks for the collaboration!

@BrentonCozby
Copy link
Owner

@bobwise

I accidentally merged into master (out of habit). Nevertheless, the fix is added and published to npm (version 2.1.0).

Here is the commit: 35ad666

@bobwise
Copy link
Author

bobwise commented Mar 25, 2020

Thanks @BrentonCozby! I'll check it out

@bobwise
Copy link
Author

bobwise commented Mar 27, 2020

I did some testing and this is working great. Thanks again @BrentonCozby!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants