Skip to content

Add aria-modal attribute to Dialog (V2) element #2851

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

Merged
merged 3 commits into from
Feb 7, 2023
Merged

Conversation

iansan5653
Copy link
Contributor

Adds the aria-modal attribute to the container for the DialogV2 element. This is required because the dialog always instantiates a focus trap. The only way to interact with the underlying page is to close the dialog:

Relevant only on dialog and alertdialog containers, setting aria-modal="true" tells assistive technologies to let the user know the ability to interact with, or access other content on the page requires the modal dialog to be closed or otherwise lose focus. (MDN)

Adding aria-modal prevents screen readers from continuing past the dialog and starting to read the underlying content, which they will do without the attribute.

Closes #2814

@iansan5653 iansan5653 requested review from a team and siddharthkp February 1, 2023 17:22
@changeset-bot
Copy link

changeset-bot bot commented Feb 1, 2023

🦋 Changeset detected

Latest commit: d8745e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -302,6 +302,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
role={role}
aria-labelledby={dialogLabelId}
aria-describedby={dialogDescriptionId}
aria-modal
Copy link
Contributor Author

@iansan5653 iansan5653 Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered adding a prop to allow users to override this, but since we don't allow them to override the focus zone or exit behavior, there is no valid scenario in which this dialog is not modal. And the role prop is restricted to dialog or alertdialog so there is no potential for conflict there.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 89.67 KB (-0.01% 🔽)
dist/browser.umd.js 90.29 KB (+0.01% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-2851 February 1, 2023 17:29 Inactive
@iansan5653 iansan5653 temporarily deployed to github-pages February 1, 2023 17:33 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2851 February 1, 2023 17:33 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DialogV2 should have aria-modal="true"
3 participants