From 996477cb280cc135ae5226b44c6839133cd00867 Mon Sep 17 00:00:00 2001 From: Arturo Castillo Delgado Date: Wed, 4 Dec 2024 13:00:38 +0100 Subject: [PATCH 1/7] [EuiModal] Adjust DOM order for close button --- packages/eui/src/components/modal/modal.tsx | 59 +++++++++++++++------ 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/packages/eui/src/components/modal/modal.tsx b/packages/eui/src/components/modal/modal.tsx index 36fbdbc5c52..989f5224680 100644 --- a/packages/eui/src/components/modal/modal.tsx +++ b/packages/eui/src/components/modal/modal.tsx @@ -6,13 +6,20 @@ * Side Public License, v 1. */ -import React, { FunctionComponent, ReactNode, HTMLAttributes } from 'react'; +import React, { + FunctionComponent, + ReactNode, + ReactElement, + HTMLAttributes, + Children, +} from 'react'; import classnames from 'classnames'; import { keys, useEuiTheme } from '../../services'; import { isDOMNode } from '../../utils'; import { EuiButtonIcon } from '../button'; +import { EuiModalHeader } from './modal_header'; import { EuiFocusTrap } from '../focus_trap'; import { EuiOverlayMask } from '../overlay_mask'; @@ -91,6 +98,39 @@ export const EuiModal: FunctionComponent = ({ const cssCloseIconStyles = [styles.euiModal__closeIcon]; + const closeButton = ( + + {(closeModal: string) => ( + + )} + + ); + + // Note: for accessibility reasons we need the close button + // to be placed **after** the header, so screen reader users will + // know _what_ they're closing before they reach the button + // https://github.com/elastic/eui/pull/7156#discussion_r1334931714 + const childrenArray = Children.toArray(children); + const headerIndex = childrenArray.findIndex( + (child) => (child as ReactElement).type === EuiModalHeader + ); + if (headerIndex > -1) { + childrenArray.splice(headerIndex + 1, 0, closeButton); + } else { + childrenArray.push(closeButton); + } + return ( @@ -104,22 +144,7 @@ export const EuiModal: FunctionComponent = ({ aria-modal={true} {...rest} > - - {(closeModal: string) => ( - - )} - - {children} + {childrenArray} From 4f5f1f2aa0d78ffbddf520935de31bb7a947dead Mon Sep 17 00:00:00 2001 From: Arturo Castillo Delgado Date: Wed, 4 Dec 2024 13:01:00 +0100 Subject: [PATCH 2/7] [EuiModal] Update snapshots --- .../__snapshots__/confirm_modal.test.tsx.snap | 40 +++++++++---------- .../modal/__snapshots__/modal.test.tsx.snap | 2 +- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/packages/eui/src/components/modal/__snapshots__/confirm_modal.test.tsx.snap b/packages/eui/src/components/modal/__snapshots__/confirm_modal.test.tsx.snap index 8817c20828f..76770c4012b 100644 --- a/packages/eui/src/components/modal/__snapshots__/confirm_modal.test.tsx.snap +++ b/packages/eui/src/components/modal/__snapshots__/confirm_modal.test.tsx.snap @@ -18,6 +18,16 @@ exports[`EuiConfirmModal renders EuiConfirmModal 1`] = ` role="alertdialog" tabindex="0" > +
+

+ A confirmation modal +

+
-
-

- A confirmation modal -

-
@@ -118,6 +118,16 @@ exports[`EuiConfirmModal renders EuiConfirmModal without EuiModalBody, if empty role="alertdialog" tabindex="0" > +
+

+ A confirmation modal +

+
-
-

- A confirmation modal -

-
diff --git a/packages/eui/src/components/modal/__snapshots__/modal.test.tsx.snap b/packages/eui/src/components/modal/__snapshots__/modal.test.tsx.snap index 60b64fc75fa..43a3ee0b9d6 100644 --- a/packages/eui/src/components/modal/__snapshots__/modal.test.tsx.snap +++ b/packages/eui/src/components/modal/__snapshots__/modal.test.tsx.snap @@ -24,6 +24,7 @@ exports[`EuiModal renders 1`] = ` role="dialog" tabindex="0" > + children - children
Date: Wed, 4 Dec 2024 13:12:51 +0100 Subject: [PATCH 3/7] Update changelog --- packages/eui/changelogs/upcoming/8140.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/eui/changelogs/upcoming/8140.md b/packages/eui/changelogs/upcoming/8140.md index ccd548863f5..9d5a7f2634a 100644 --- a/packages/eui/changelogs/upcoming/8140.md +++ b/packages/eui/changelogs/upcoming/8140.md @@ -1,3 +1,4 @@ **Accessibility** - When the tooltips components (`EuiTooltip`, `EuiIconTip`) are used inside components that handle the Escape key (like `EuiFlyout` or `EuiModal`), pressing the Escape key will now only close the tooltip and not the entire wrapping component. +- In `EuiModal` the close button is now placed **after** the header, in the DOM, for correctness in reading order. From 3a5501324b67208d3ecd5baf2aec4fbe145682fc Mon Sep 17 00:00:00 2001 From: Arturo Castillo Delgado Date: Wed, 4 Dec 2024 13:25:04 +0100 Subject: [PATCH 4/7] [EuiModal] Unit test for button placement --- packages/eui/src/components/modal/modal.test.tsx | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/eui/src/components/modal/modal.test.tsx b/packages/eui/src/components/modal/modal.test.tsx index d2843fdef27..3a79e0f082e 100644 --- a/packages/eui/src/components/modal/modal.test.tsx +++ b/packages/eui/src/components/modal/modal.test.tsx @@ -12,6 +12,7 @@ import { requiredProps } from '../../test'; import { shouldRenderCustomStyles } from '../../test/internal'; import { EuiModal } from './modal'; +import { EuiHeader } from '../header'; describe('EuiModal', () => { shouldRenderCustomStyles( {}}>children); @@ -80,4 +81,18 @@ describe('EuiModal', () => { expect(modalB.getAttribute('style')).toEqual('max-inline-size: 50%;'); }); }); + + describe('reading order', () => { + test('button is placed after header', () => { + const { getByTestSubject } = render( + null}> + Title + children + + ); + + const header = getByTestSubject('header'); + expect(header.nextElementSibling?.tagName).toBe('BUTTON'); + }); + }); }); From 6e24515e8296768f2bd8dd3f75d83ac91877b126 Mon Sep 17 00:00:00 2001 From: Arturo Castillo Delgado Date: Wed, 4 Dec 2024 15:48:02 +0100 Subject: [PATCH 5/7] Revert "Update changelog" This reverts commit cccecc6d852fcb5a7b1655e180be44eef8b83451. --- packages/eui/changelogs/upcoming/8140.md | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/eui/changelogs/upcoming/8140.md b/packages/eui/changelogs/upcoming/8140.md index 9d5a7f2634a..ccd548863f5 100644 --- a/packages/eui/changelogs/upcoming/8140.md +++ b/packages/eui/changelogs/upcoming/8140.md @@ -1,4 +1,3 @@ **Accessibility** - When the tooltips components (`EuiTooltip`, `EuiIconTip`) are used inside components that handle the Escape key (like `EuiFlyout` or `EuiModal`), pressing the Escape key will now only close the tooltip and not the entire wrapping component. -- In `EuiModal` the close button is now placed **after** the header, in the DOM, for correctness in reading order. From 529ca3e45a438eef87dd966a23deccc5a05d90e4 Mon Sep 17 00:00:00 2001 From: Arturo Castillo Delgado Date: Wed, 4 Dec 2024 15:49:57 +0100 Subject: [PATCH 6/7] Changelog, properly --- packages/eui/changelogs/upcoming/8202.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 packages/eui/changelogs/upcoming/8202.md diff --git a/packages/eui/changelogs/upcoming/8202.md b/packages/eui/changelogs/upcoming/8202.md new file mode 100644 index 00000000000..113a73a0c5d --- /dev/null +++ b/packages/eui/changelogs/upcoming/8202.md @@ -0,0 +1,3 @@ +**Accessibility** + +- In `EuiModal` the close button is now placed **after** the header, in the DOM, for correctness in reading order. From 7715ebec912c4209c2f7321dcd3d7362f5366666 Mon Sep 17 00:00:00 2001 From: Arturo Castillo Delgado Date: Wed, 4 Dec 2024 16:40:03 +0100 Subject: [PATCH 7/7] [EuiModal] Clean up test --- .../eui/src/components/modal/modal.test.tsx | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/eui/src/components/modal/modal.test.tsx b/packages/eui/src/components/modal/modal.test.tsx index 3a79e0f082e..2fe65fb83ba 100644 --- a/packages/eui/src/components/modal/modal.test.tsx +++ b/packages/eui/src/components/modal/modal.test.tsx @@ -82,17 +82,15 @@ describe('EuiModal', () => { }); }); - describe('reading order', () => { - test('button is placed after header', () => { - const { getByTestSubject } = render( - null}> - Title - children - - ); + test('button is placed after header', () => { + const { getByTestSubject } = render( + null}> + Title + children + + ); - const header = getByTestSubject('header'); - expect(header.nextElementSibling?.tagName).toBe('BUTTON'); - }); + const header = getByTestSubject('header'); + expect(header.nextElementSibling?.tagName).toBe('BUTTON'); }); });