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

[BUG][EuiModal] Fix VoiceOver + Safari escaping focus trap #7564

Merged
merged 11 commits into from
Mar 12, 2024
Merged

[BUG][EuiModal] Fix VoiceOver + Safari escaping focus trap #7564

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a76a501
Added ARIA role and modal attrs to EuiModal.
1Copenut Mar 8, 2024
315f1dc
Updated Jest snapshots for a11y attrs.
1Copenut Mar 8, 2024
2afd81c
Adding changelog for PR #7564.
1Copenut Mar 8, 2024
1fe835b
Renaming ariaRole to match HTML attr.
1Copenut Mar 8, 2024
fcee44e
Bumping two snapshot tests.
1Copenut Mar 8, 2024
52622ce
Adding aria-labels to example modals.
1Copenut Mar 8, 2024
3223244
Added id to EuiModalHeaderTitleProps for aria-labelledby relationship…
1Copenut Mar 11, 2024
efaad76
Merge branch 'main' into feature/euiModal-a11y-markup
cee-chen Mar 11, 2024
525b314
changelog copy tweaks
cee-chen Mar 11, 2024
10d59cd
[PR feedback] Remove unnecessary type
cee-chen Mar 11, 2024
9e9cffa
Docs pass
cee-chen Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/upcoming/7564.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed `EuiModal` to properly trap Safari + VoiceOver virtual cursor
4 changes: 4 additions & 0 deletions src/components/modal/__snapshots__/confirm_modal.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ Array [
>
<div
aria-label="aria-label"
aria-modal="true"
class="euiModal euiModal--confirmation testClass1 testClass2 emotion-euiModal-defaultMaxWidth-confirmation-euiTestCss"
data-test-subj="test subject string"
role="dialog"
tabindex="0"
1Copenut marked this conversation as resolved.
Show resolved Hide resolved
>
<button
Expand Down Expand Up @@ -110,8 +112,10 @@ Array [
>
<div
aria-label="aria-label"
aria-modal="true"
class="euiModal euiModal--confirmation testClass1 testClass2 emotion-euiModal-defaultMaxWidth-confirmation-euiTestCss"
data-test-subj="test subject string"
role="dialog"
tabindex="0"
>
<button
Expand Down
2 changes: 2 additions & 0 deletions src/components/modal/__snapshots__/modal.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ exports[`EuiModal renders 1`] = `
>
<div
aria-label="aria-label"
aria-modal="true"
class="euiModal testClass1 testClass2 emotion-euiModal-defaultMaxWidth-euiTestCss"
data-test-subj="test subject string"
role="dialog"
tabindex="0"
>
<button
Expand Down
8 changes: 8 additions & 0 deletions src/components/modal/modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ export interface EuiModalProps extends HTMLAttributes<HTMLDivElement> {
* Can be a DOM node, or a selector string (which will be passed to document.querySelector() to find the DOM node), or a function that returns a DOM node.
*/
initialFocus?: HTMLElement | (() => HTMLElement) | string;
/**
* Identifies a modal dialog to screen readers. Modal dialogs that confirm destructive actions
* or need a user's attention should use "alertdialog".
*/
ariaRole?: 'dialog' | 'alertdialog';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this b/c there are a handful of cases where I'd like the modal to break user flow and announce itself immediately. Thinking specifically about modals that confirm deleting or removing an object here. By adding it as a new prop and setting a standard default, I thought we could capture the best of both worlds without accidentally regressing the container to a div[role="presentation"].

Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is there a specific reason this needs to be named ariaRole or can we just go with role to match the DOM attribute?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, bonus/optional question: is there ever a situation where consumers would need to unset the role or set a non dialog role? I'm leaning towards no, but just wanted to at least raise the question and get people's thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

I added this b/c there are a handful of cases where I'd like the modal to break user flow and announce itself immediately. Thinking specifically about modals that confirm deleting or removing an object here

Should we should go ahead and add this prop to EuiConfirmModal as well with a default of alertdialogue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason to name it ariaRole. I was thinking role at first but talked myself out of it. I like your point that role is the HTML attribute name, and changing to that feels fitting.

I also like the suggestion to make EuiConfirmModal default to alertdialog. I'll update and retest those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and added aria-label attributes to each example in docs. The axe-core plugin was emphasizing elements with role="dialog | alertdialog" should have accessible labels. Ideally we enforce having an EuiModalHeader and title in each modal, but that's increasing scope more than the original PR set out to do.

}

export const EuiModal: FunctionComponent<EuiModalProps> = ({
Expand All @@ -51,6 +56,7 @@ export const EuiModal: FunctionComponent<EuiModalProps> = ({
initialFocus,
onClose,
maxWidth = true,
ariaRole = 'dialog',
style,
...rest
}) => {
Expand Down Expand Up @@ -88,6 +94,8 @@ export const EuiModal: FunctionComponent<EuiModalProps> = ({
onKeyDown={onKeyDown}
tabIndex={0}
style={newStyle}
role={ariaRole}
aria-modal={true}
{...rest}
>
<EuiI18n
Expand Down
Loading