-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
a11y(Alert): give Alert alertdialog role #6846
Conversation
Generate changelog in
|
@@ -172,6 +172,7 @@ export class Alert extends AbstractPureComponent<AlertProps> { | |||
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.
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:
Dialog
role
prop configurable (default="dialog"
)Alert
'sDialog
'srole
toalertdialog
: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/alertdialog_role