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

1Copenut · 2024-03-08T15:07:32Z

Summary

I've recently noticed I'm able to escape the EuiModal focus trap to the inert page when using VoiceOver + Safari. This only happens when I'm navigating by virtual cursor chords Ctrl + Opt + ARROW_KEY, but this is a primary navigation pattern so it needed to be remediated.

PR closes #7532 and closes #7563.

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

Browser QA
- Checked in both light and dark modes
- Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
- Checked for accessibility including keyboard-only and screenreader modes
Docs site QA
- Added documentation
- Props have proper autodocs (using @default if default values are missing) and playground toggles
- Checked Code Sandbox works for any docs examples
Code quality checklist
- Added or updated jest and cypress tests
Release checklist
- A changelog entry exists and is marked appropriately.
- ~~If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~

Screen reader test pairings

MacOS Safari + VoiceOver
Win10 + Firefox + NVDA
Win10 + Chrome + NVDA
Win10 + Chrome + JAWS

All pairings trapped focus correctly. This represents an improved UX for Safari + VO, and no regression for the other pairings.

1Copenut

Two clarifying comments into my "why".

src/components/modal/__snapshots__/confirm_modal.test.tsx.snap

1Copenut · 2024-03-08T15:51:57Z

src/components/modal/modal.tsx

+   * 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';


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"].

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?

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

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?

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.

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.

cee-chen · 2024-03-08T19:01:30Z

src-docs/src/views/modal/confirm_modal.tsx

@@ -22,6 +22,7 @@ export default () => {
  if (isModalVisible) {
    modal = (
      <EuiConfirmModal
+        aria-label="EuiModal confirm example"


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.

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?

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

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.

When in doubt, always keep the scope smaller :)

… to heading.

1Copenut · 2024-03-11T15:49:16Z

I refactored the examples to use explicit IDs and aria-labelledby to target the heading text. This works much better after discussing with @cee-chen and is the best practice for labeling modals. I had to add an id: String; to EuiModalHeaderTitleProps so I could pass an ID into titleProps and have it spread onto the correct element. LMK if that's too far afield and I'll dial it back.

src/components/modal/modal_header_title.tsx

- already inherited from `HTMLAttributes`

- use generator ID util instead of static strings - misc cleanup - replace `<div>`s with fragments, replace `let` jsx with inline conditional JSX

cee-chen

@1Copenut Code changes LGTM at this point - feel free to re-test that the aria-labelledby works as expected for you in VO/etc before merging!

kibanamachine · 2024-03-11T21:13:19Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7564/
Storybook: https://eui.elastic.co/pr_7564/storybook

elasticmachine · 2024-03-11T21:13:22Z

💚 Build Succeeded

Buildkite Build
Commit: 9e9cffa

History

💚 Build #1467 succeeded 3223244
💚 Build #1457 succeeded 52622ce
💚 Build #1455 succeeded 2afd81c
💚 Build #1454 succeeded 315f1dc

cc @1Copenut

1Copenut · 2024-03-11T22:26:12Z

UPDATE March 12:
After final screen reader testing, I noticed this fix improves the virtual cursor trapping for all three screen readers. I'd never noticed it before on NVDA and JAWS, but they were capable of escaping behind the smoke layer in our examples. Now that we've added a role and accessible name, screen readers are properly trapped by TAB keypress and virtual cursor navigation patterns. This is a nice net improvement.

Thank you @cee-chen. I'll give this one more pass with screen readers in the morning and merge.

`v93.3.0`⏩ `v93.4.0` --- ## [`v93.4.0`](https://github.com/elastic/eui/releases/v93.4.0) - Added the following properties to `EuiButtonGroup`'s `options` configs: `toolTipContent`, `toolTipProps`, and `title`. These new properties allow wrapping buttons in `EuiToolTips`, and additionally customizing or disabling the native browser `title` tooltip. ([#7461](elastic/eui#7461)) - Enhanced `EuiResizeObserver` and `useResizeObserver`'s performance to not trigger page reflows on resize event ([#7575](elastic/eui#7575)) - Updated `EuiSuperUpdateButton` to support custom button text via an optional `children` prop ([#7576](elastic/eui#7576)) **Bug fixes** - Fixed `EuiFlyout` to not repeatedly remove/add a body class on resize ([#7462](elastic/eui#7462)) - Fixed `EuiToast` title text to wrap instead of overflowing out of the container ([#7568](elastic/eui#7568)) - Fixed a visual bug with `EuiHeaderBreadcrumbs` with popovers ([#7580](elastic/eui#7580)) **Deprecations** - Deprecated `euiPalettePositive` and `euiPaletteNegative` in favour of a more culturally inclusive `euiPaletteGreen` and `euiPaletteRed` ([#7570](elastic/eui#7570)) - Deprecated all charts theme exports in favor of `@elastic/charts` exports: ([#7572](elastic/eui#7572)) - Deprecated `EUI_CHARTS_THEME_<DARK|LIGHT>` in favor of `<DARK|LIGHT>_THEME` from `@elastic/charts`. ([#7572](elastic/eui#7572)) - Deprecated `EUI_SPARKLINE_THEME_PARTIAL` in favor of `useSparklineOverrides` theme from the kibana `charts` plugin `theme` service. **Accessibility** - Updated `EuiModal` to set an `aria-modal` attribute and a default `dialog` role ([#7564](elastic/eui#7564)) - Updated `EuiConfirmModal` to set a default `alertdialog` role ([#7564](elastic/eui#7564)) - Fixed `EuiModal` and `EuiConfirmModal` to properly trap Safari+VoiceOver's virtual cursor ([#7564](elastic/eui#7564))

1Copenut added 2 commits March 7, 2024 19:36

Added ARIA role and modal attrs to EuiModal.

a76a501

Updated Jest snapshots for a11y attrs.

315f1dc

1Copenut added bug accessibility - keyboard labels Mar 8, 2024

1Copenut self-assigned this Mar 8, 2024

Adding changelog for PR #7564.

2afd81c

1Copenut marked this pull request as ready for review March 8, 2024 15:48

1Copenut requested a review from a team as a code owner March 8, 2024 15:48

1Copenut commented Mar 8, 2024

View reviewed changes

1Copenut added 3 commits March 8, 2024 12:15

Renaming ariaRole to match HTML attr.

1fe835b

Bumping two snapshot tests.

fcee44e

Adding aria-labels to example modals.

52622ce

1Copenut requested a review from cee-chen March 8, 2024 18:58

cee-chen reviewed Mar 8, 2024

View reviewed changes

Added id to EuiModalHeaderTitleProps for aria-labelledby relationship…

3223244

… to heading.

1Copenut requested a review from cee-chen March 11, 2024 15:46

cee-chen reviewed Mar 11, 2024

View reviewed changes

src/components/modal/modal_header_title.tsx Outdated Show resolved Hide resolved

cee-chen added 4 commits March 11, 2024 13:20

Merge branch 'main' into feature/euiModal-a11y-markup

efaad76

changelog copy tweaks

525b314

[PR feedback] Remove unnecessary type

10d59cd

- already inherited from `HTMLAttributes`

Docs pass

9e9cffa

- use generator ID util instead of static strings - misc cleanup - replace `<div>`s with fragments, replace `let` jsx with inline conditional JSX

cee-chen approved these changes Mar 11, 2024

View reviewed changes

1Copenut merged commit f9ff62e into elastic:main Mar 12, 2024
7 checks passed

1Copenut deleted the feature/euiModal-a11y-markup branch March 12, 2024 15:55

cee-chen mentioned this pull request Mar 18, 2024

Upgrade EUI to v93.4.0 elastic/kibana#178890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

1Copenut commented Mar 8, 2024 •

edited

Loading

1Copenut left a comment

1Copenut Mar 8, 2024

cee-chen Mar 8, 2024

cee-chen Mar 8, 2024

cee-chen Mar 8, 2024

1Copenut Mar 8, 2024

1Copenut Mar 8, 2024

cee-chen Mar 8, 2024

1Copenut Mar 8, 2024

cee-chen Mar 8, 2024 •

edited

Loading

1Copenut commented Mar 11, 2024

cee-chen left a comment

kibanamachine commented Mar 11, 2024

elasticmachine commented Mar 11, 2024

1Copenut commented Mar 11, 2024 •

edited

Loading

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

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

Conversation

1Copenut commented Mar 8, 2024 • edited Loading

Summary

QA

General checklist

Screen reader test pairings

1Copenut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

1Copenut commented Mar 11, 2024

cee-chen left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 11, 2024

elasticmachine commented Mar 11, 2024

💚 Build Succeeded

History

1Copenut commented Mar 11, 2024 • edited Loading

1Copenut commented Mar 8, 2024 •

edited

Loading

cee-chen Mar 8, 2024 •

edited

Loading

1Copenut commented Mar 11, 2024 •

edited

Loading