BrentonCozby / dom-slider

Plain JavaScript version of jQuery's slideToggle(), slideDown(), & slideUp(), but does not use display: none.
MIT License
61 stars 7 forks source link

Collapsed content is still visible to screen readers #12

Closed bobwise closed 4 years ago

bobwise commented 4 years ago

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 commented 4 years ago

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?

bobwise commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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: 35ad66692fbf8d7f861e4b3923d2d55829136808

bobwise commented 4 years ago

Thanks @BrentonCozby! I'll check it out

bobwise commented 4 years ago

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