-
Notifications
You must be signed in to change notification settings - Fork 2.2k
a11y(Alert): give Alert alertdialog role #6846
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
a11y(Alert): give Alert alertdialog role #6846
Conversation
Generate changelog in
|
| return ( | ||
| <Dialog | ||
| {...overlayProps} | ||
| role="alertdialog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also want to set aria-modal="true" when setting this role
We may also want to follow this guidance if adding this role
The alert dialog must have at least one focusable control — such as Confirm, Close, and Cancel — and focus must be moved to that control when the alert dialog appears. Alertdialogs can have additional interactive controls such as text fields and checkboxes.
From my logging it appears focus is initially on the start of the dialog focus trap

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Tied aria-modal to the enforceFocus prop in Dialog, these seemed to be one in the same. 1a9709b
| /** | ||
| * @default "dialog" | ||
| */ | ||
| role?: React.AriaRole; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have an explicit list of values we expect to be valid given the setup of this component? "dialog" | "alertdialog" feels like a reasonable place to start that can be expanded as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
evansjohnson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Checklist
Changes proposed in this pull request:
Dialogroleprop configurable (default="dialog")Alert'sDialog'sroletoalertdialog: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/alertdialog_role