Skip to content

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

Merged
joshblack merged 3 commits into
mainfrom
dialogv2-aria-modal
Feb 7, 2023
Merged

Add aria-modal attribute to Dialog (V2) element#2851
joshblack merged 3 commits into
mainfrom
dialogv2-aria-modal

Conversation

@iansan5653

Copy link
Copy Markdown
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 siddharthkpFebruary 1, 2023 17:22
@changeset-bot

changeset-botBot commented Feb 1, 2023

Copy link
Copy Markdown

🦋 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
NameType
@primer/reactPatch

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

Comment threadsrc/Dialog/Dialog.tsx
role={role}
aria-labelledby={dialogLabelId}
aria-describedby={dialogDescriptionId}
aria-modal

@iansan5653iansan5653Feb 1, 2023

Copy link
Copy Markdown
ContributorAuthor

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

github-actionsBot commented Feb 1, 2023

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js89.67 KB (-0.01% 🔽)
dist/browser.umd.js90.29 KB (+0.01% 🔺)

@github-actions
github-actionsBot 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-actionsBot temporarily deployed to storybook-preview-2851 February 1, 2023 17:33 Inactive
@joshblack
joshblack enabled auto-merge (squash) February 7, 2023 18:33
@github-actions
github-actionsBottemporarily deployed to storybook-preview-2851 February 7, 2023 18:33 Inactive
@joshblack
joshblack merged commit d8f1843 into mainFeb 7, 2023
@joshblack
joshblack deleted the dialogv2-aria-modal branch February 7, 2023 18:37
@primer-cssprimer-css mentioned this pull request Feb 7, 2023
Sign up for freeto 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

@iansan5653@joshblack@colebemis