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 6 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
2 changes: 2 additions & 0 deletions src-docs/src/views/modal/confirm_modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export default () => {
if (isModalVisible) {
modal = (
<EuiConfirmModal
aria-label="EuiModal confirm example"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about the aria-label examples that you added. These don't feel like useful production examples to me that a developer would be able to extrapolate from and write something themselves for their own modals.

  1. Why would the aria-label for the modal be different from the title? Should we not use aria-labelledby instead and point that at the title?
  2. If the modal label should be different from the title, then let's make the copy actually specific to the modal intent. e.g., for this one, something like Confirm subscription

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally the accessible label uses the title and an ID to create an aria-labelledby situation. Looking at the code as it sits, we can't count on a title always being present, at least not in the basic EuiModal. The confirmation modal is a bit easier to reason about where we're checking if the title prop is present.

I'm at a crossroads what is the best approach here. I can either keep this one scoped tightly to fix the immediate problem of VoiceOver escaping the modal and make a follow on item to improve accessible labels, or widen the scope of this issue and possibly it becomes a breaking change. Either seems an acceptable path forward.

Copy link
Contributor

@cee-chen cee-chen Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When in doubt, always keep the scope smaller :)

style={{ width: 600 }}
title="Update subscription to Platinum?"
onCancel={closeModal}
Expand All @@ -44,6 +45,7 @@ export default () => {
if (isDestroyModalVisible) {
destroyModal = (
<EuiConfirmModal
aria-label="EuiModal confirm example two"
title="Discard dashboard changes?"
onCancel={closeDestroyModal}
onConfirm={closeDestroyModal}
Expand Down
1 change: 1 addition & 0 deletions src-docs/src/views/modal/confirm_modal_loading.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export default () => {
if (isModalVisible) {
modal = (
<EuiConfirmModal
aria-label="EuiModal confirm loading"
title="Delete the EUI repo?"
onCancel={closeModal}
onConfirm={() => {
Expand Down
2 changes: 1 addition & 1 deletion src-docs/src/views/modal/modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default () => {

if (isModalVisible) {
modal = (
<EuiModal onClose={closeModal}>
<EuiModal aria-label="EuiModal example" onClose={closeModal}>
<EuiModalHeader>
<EuiModalHeaderTitle>Modal title</EuiModalHeaderTitle>
</EuiModalHeader>
Expand Down
6 changes: 5 additions & 1 deletion src-docs/src/views/modal/modal_form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,11 @@ export default () => {

if (isModalVisible) {
modal = (
<EuiModal onClose={closeModal} initialFocus="[name=popswitch]">
<EuiModal
aria-label="EuiModal form example"
onClose={closeModal}
initialFocus="[name=popswitch]"
>
<EuiModalHeader>
<EuiModalHeaderTitle>Modal title</EuiModalHeaderTitle>
</EuiModalHeader>
Expand Down
6 changes: 5 additions & 1 deletion src-docs/src/views/modal/modal_width.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ export default () => {

if (isModalVisible) {
modal = (
<EuiModal style={{ width: 800 }} onClose={closeModal}>
<EuiModal
aria-label="EuiModal modal width"
style={{ width: 800 }}
onClose={closeModal}
>
<EuiModalHeader>
<EuiModalHeaderTitle>Modal title</EuiModalHeaderTitle>
</EuiModalHeader>
Expand Down
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="alertdialog"
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="alertdialog"
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: 7 additions & 1 deletion src/components/modal/confirm_modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,13 @@ export const EuiConfirmModal: FunctionComponent<EuiConfirmModalProps> = ({
}

return (
<EuiModal className={classes} css={cssStyles} onClose={onCancel} {...rest}>
<EuiModal
className={classes}
css={cssStyles}
onClose={onCancel}
role="alertdialog"
{...rest}
>
{modalTitle}

{message && (
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".
*/
role?: 'dialog' | 'alertdialog';
}

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