From 9307cc86f22e8a93441e0cfb9ea218fcf6276d2d Mon Sep 17 00:00:00 2001
From: Giordano Ricci <me@giordanoricci.com>
Date: Tue, 22 Feb 2022 14:12:34 +0000
Subject: [PATCH] UI: Improve modal a11y by setting role & using title as label
 (#45472)

* UI: Improve modal a11y by setting role & using title as label

* remove wrapping div for cutom title components

* Fix typo
---
 .betterer.results                             |  3 -
 .../src/components/Modal/Modal.test.tsx       | 25 ++++----
 .../grafana-ui/src/components/Modal/Modal.tsx | 59 +++++++++----------
 .../src/components/Modal/ModalHeader.tsx      |  7 ++-
 4 files changed, 43 insertions(+), 51 deletions(-)

diff --git a/.betterer.results b/.betterer.results
index 235ffb75cae..9bbf07ee673 100644
--- a/.betterer.results
+++ b/.betterer.results
@@ -44,9 +44,6 @@ exports[`no enzyme tests`] = {
     "packages/grafana-ui/src/components/Logs/LogRows.test.tsx:2288254498": [
       [3, 17, 13, "RegExp match", "2409514259"]
     ],
-    "packages/grafana-ui/src/components/Modal/Modal.test.tsx:4235780832": [
-      [1, 17, 13, "RegExp match", "2409514259"]
-    ],
     "packages/grafana-ui/src/components/QueryField/QueryField.test.tsx:1906163280": [
       [1, 19, 13, "RegExp match", "2409514259"]
     ],
diff --git a/packages/grafana-ui/src/components/Modal/Modal.test.tsx b/packages/grafana-ui/src/components/Modal/Modal.test.tsx
index 4001fbfeee5..f629c8fb54d 100644
--- a/packages/grafana-ui/src/components/Modal/Modal.test.tsx
+++ b/packages/grafana-ui/src/components/Modal/Modal.test.tsx
@@ -1,27 +1,24 @@
 import React from 'react';
-import { mount } from 'enzyme';
 import { Modal } from './Modal';
+import { render, screen } from '@testing-library/react';
 
 describe('Modal', () => {
-  it('renders without error', () => {
-    mount(<Modal title={'Some Title'} isOpen={true} />);
-  });
-
   it('renders nothing by default or when isOpen is false', () => {
-    const wrapper = mount(<Modal title={'Some Title'} />);
-    expect(wrapper.html()).toBe(null);
+    render(<Modal title="Some Title" />);
 
-    wrapper.setProps({ ...wrapper.props(), isOpen: false });
-    expect(wrapper.html()).toBe(null);
+    expect(screen.queryByRole('dialog')).not.toBeInTheDocument();
   });
 
   it('renders correct contents', () => {
-    const wrapper = mount(
-      <Modal title={'Some Title'} isOpen={true}>
-        <div id={'modal-content'}>Content</div>
+    render(
+      <Modal title="Some Title" isOpen>
+        <div data-testid="modal-content">Content</div>
       </Modal>
     );
-    expect(wrapper.find('div#modal-content').length).toBe(1);
-    expect(wrapper.contains('Some Title')).toBeTruthy();
+
+    expect(screen.getByRole('dialog')).toBeInTheDocument();
+    expect(screen.getByLabelText('Some Title')).toBeInTheDocument();
+
+    expect(screen.getByTestId('modal-content')).toBeInTheDocument();
   });
 });
diff --git a/packages/grafana-ui/src/components/Modal/Modal.tsx b/packages/grafana-ui/src/components/Modal/Modal.tsx
index f053d665b59..a159a2825ec 100644
--- a/packages/grafana-ui/src/components/Modal/Modal.tsx
+++ b/packages/grafana-ui/src/components/Modal/Modal.tsx
@@ -1,7 +1,9 @@
 import { cx } from '@emotion/css';
 import { FocusScope } from '@react-aria/focus';
-import { OverlayContainer } from '@react-aria/overlays';
-import React, { PropsWithChildren, useCallback, useEffect } from 'react';
+import { useDialog } from '@react-aria/dialog';
+
+import { OverlayContainer, useOverlay } from '@react-aria/overlays';
+import React, { PropsWithChildren, useRef } from 'react';
 
 import { useTheme2 } from '../../themes';
 import { IconName } from '../../types';
@@ -39,33 +41,24 @@ export function Modal(props: PropsWithChildren<Props>) {
     closeOnBackdropClick = true,
     className,
     contentClassName,
-    onDismiss: propsOnDismiss,
+    onDismiss,
     onClickBackdrop,
     trapFocus = true,
   } = props;
   const theme = useTheme2();
   const styles = getModalStyles(theme);
-  const onDismiss = useCallback(() => {
-    if (propsOnDismiss) {
-      propsOnDismiss();
-    }
-  }, [propsOnDismiss]);
 
-  useEffect(() => {
-    const onEscKey = (ev: KeyboardEvent) => {
-      if (ev.key === 'Esc' || ev.key === 'Escape') {
-        onDismiss();
-      }
-    };
-    if (isOpen && closeOnEscape) {
-      document.addEventListener('keydown', onEscKey, false);
-    } else {
-      document.removeEventListener('keydown', onEscKey, false);
-    }
-    return () => {
-      document.removeEventListener('keydown', onEscKey, false);
-    };
-  }, [closeOnEscape, isOpen, onDismiss]);
+  const ref = useRef<HTMLDivElement>(null);
+
+  // Handle interacting outside the dialog and pressing
+  // the Escape key to close the modal.
+  const { overlayProps, underlayProps } = useOverlay(
+    { isKeyboardDismissDisabled: closeOnEscape, isOpen, onClose: onDismiss },
+    ref
+  );
+
+  // Get props for the dialog and its title
+  const { dialogProps, titleProps } = useDialog({}, ref);
 
   if (!isOpen) {
     return null;
@@ -78,16 +71,17 @@ export function Modal(props: PropsWithChildren<Props>) {
       <div
         className={styles.modalBackdrop}
         onClick={onClickBackdrop || (closeOnBackdropClick ? onDismiss : undefined)}
+        {...underlayProps}
       />
       <FocusScope contain={trapFocus} autoFocus restoreFocus>
-        {/*
-          tabIndex=-1 is needed here to support highlighting text within the modal when using FocusScope
-          see https://github.com/adobe/react-spectrum/issues/1604#issuecomment-781574668
-        */}
-        <div tabIndex={-1} className={cx(styles.modal, className)}>
+        <div className={cx(styles.modal, className)} ref={ref} {...overlayProps} {...dialogProps}>
           <div className={headerClass}>
-            {typeof title === 'string' && <DefaultModalHeader {...props} title={title} />}
-            {typeof title !== 'string' && title}
+            {typeof title === 'string' && <DefaultModalHeader {...props} title={title} id={titleProps.id} />}
+            {
+              // FIXME: custom title components won't get an accessible title.
+              // Do we really want to support them or shall we just limit this ModalTabsHeader?
+              typeof title !== 'string' && title
+            }
             <div className={styles.modalHeaderClose}>
               <IconButton aria-label="Close dialogue" surface="header" name="times" size="xl" onClick={onDismiss} />
             </div>
@@ -130,11 +124,12 @@ function ModalButtonRow({ leftItems, children }: { leftItems?: React.ReactNode;
 Modal.ButtonRow = ModalButtonRow;
 
 interface DefaultModalHeaderProps {
+  id?: string;
   title: string;
   icon?: IconName;
   iconTooltip?: string;
 }
 
-function DefaultModalHeader({ icon, iconTooltip, title }: DefaultModalHeaderProps): JSX.Element {
-  return <ModalHeader icon={icon} iconTooltip={iconTooltip} title={title} />;
+function DefaultModalHeader({ icon, iconTooltip, title, id }: DefaultModalHeaderProps): JSX.Element {
+  return <ModalHeader icon={icon} iconTooltip={iconTooltip} title={title} id={id} />;
 }
diff --git a/packages/grafana-ui/src/components/Modal/ModalHeader.tsx b/packages/grafana-ui/src/components/Modal/ModalHeader.tsx
index 4873b394c73..6d5c2c1acaf 100644
--- a/packages/grafana-ui/src/components/Modal/ModalHeader.tsx
+++ b/packages/grafana-ui/src/components/Modal/ModalHeader.tsx
@@ -5,6 +5,7 @@ import { useStyles2 } from '../../themes';
 
 interface Props {
   title: string;
+  id?: string;
   /** @deprecated */
   icon?: IconName;
   /** @deprecated */
@@ -12,12 +13,14 @@ interface Props {
 }
 
 /** @internal */
-export const ModalHeader: React.FC<Props> = ({ icon, iconTooltip, title, children }) => {
+export const ModalHeader: React.FC<Props> = ({ icon, iconTooltip, title, children, id }) => {
   const styles = useStyles2(getModalStyles);
 
   return (
     <>
-      <h2 className={styles.modalHeaderTitle}>{title}</h2>
+      <h2 className={styles.modalHeaderTitle} id={id}>
+        {title}
+      </h2>
       {children}
     </>
   );