react-modal icon indicating copy to clipboard operation
react-modal copied to clipboard

Accessibility issues with the codepen demos

Open patrickhlauke opened this issue 3 years ago • 7 comments

Summary:

Many of the demos linked to from the README have accessibility issues:

  • Minimal example - once the modal is opened, aria-hidden="true" is set on the <body>. this also hides the modal itself (which is a child element of the <body>) from screen readers; when the modal is closed, focus is not returned explicitly to the "Trigger modal" button
  • Using setAppElement - same issues as the minimal example
  • Using inline styles - when the modal is closed, focus is not returned explicitly to the "Trigger modal" button
  • Customizing the default styles - while the #1 modal is fine, when triggering #2 modal the <body> is once again hidden with aria-hidden="true" and, as a result, the #2 modal is also hidden from screen readers

Steps to reproduce:

  1. Use a screen reader
  2. Open the codepen demos
  3. Open/close the various modals

Expected behavior:

  • don't set aria-hidden="true" on the <body> itself, as this will hide everything from screen readers, including the dialog
  • make sure that focus is returned to the button that first opened the modal

Review how the working demos like Using inline styles do it, and apply the same behavior to the broken demos.

patrickhlauke avatar Jun 21 '22 22:06 patrickhlauke

Lastly, all codepen demo modals seem to lack role="dialog" and aria-modal="true". However, just trying out this package locally on a test page, I see that those attributes are present. Are the codepens simply using an outdated version?

patrickhlauke avatar Jun 21 '22 23:06 patrickhlauke

@patrickhlauke Many examples are outdated. The issue with <body /> is historically, it was used as the default element to receive the attributes.

diasbruno avatar Jun 22 '22 00:06 diasbruno

I'm looking for a first issue to tackle and wouldn't mind taking this one. Couple questions though:

  • The Using setAppExample is adding aria-hidden="true" to #main but the modal element lies outside #main. So this should be fine already, yeah?
  • Is there a need for a class based component example?
  • Do you have any new examples you'd like me to add?

neilhsmith avatar Mar 15 '23 18:03 neilhsmith

@neilhsmith A good place to start is to update the available examples using the correct accessibility functionality.

Other examples would be really nice, specially combining libraries like styled components and others to see how react-modal is compatible. This will help investigate bugs...

diasbruno avatar Mar 16 '23 12:03 diasbruno

Cool, thanks. I'll take a crack at it.

neilhsmith avatar Mar 16 '23 16:03 neilhsmith

Great. Let me know if you need anything.

diasbruno avatar Mar 17 '23 12:03 diasbruno

Apologies for the delay but I have an updated collection here. I removed the setAppElement example since react-modal now warns you in console and instead set the app element in every example. I also added a new styled-components example. Otherwise, the examples are mostly the same as they were - just updated to today's version of react-modal which fixes the accessibility issues automatically.

I'll open a PR but I have plans to add at least one more example later today for tailwind so I wanted to update you here in case you have opinions.

neilhsmith avatar Mar 30 '23 18:03 neilhsmith