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

Jump to bottom

Accessibility issues with the codepen demos #955

Open
patrickhlauke opened this issue Jun 21, 2022 · 7 comments · May be fixed by #1012
Open

Accessibility issues with the codepen demos #955

patrickhlauke opened this issue Jun 21, 2022 · 7 comments · May be fixed by #1012
Labels
documentation examples good first issue

Comments

@patrickhlauke
Copy link

patrickhlauke commented Jun 21, 2022 •
edited
Loading

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

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?

@diasbruno
Copy link
Collaborator

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

@neilhsmith
Copy link

neilhsmith commented Mar 15, 2023 •
edited
Loading

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?

@diasbruno
Copy link
Collaborator

@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...

@neilhsmith
Copy link

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

@diasbruno
Copy link
Collaborator

Great. Let me know if you need anything.

@neilhsmith
Copy link

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 neilhsmith linked a pull request Mar 31, 2023 that will close this issue
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
documentation examples good first issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[chore] update readme and docs demos neilhsmith/react-modal
3 participants
@diasbruno @patrickhlauke @neilhsmith