From 8b4ba0980f3ec44d262626380eb64eb1e660895a Mon Sep 17 00:00:00 2001
From: Rajat <rd.rajat23@gmail.com>
Date: Fri, 12 Jan 2024 00:59:52 +0530
Subject: [PATCH] fix: Modal Close button a11y issues (#15057)

* fix: modal close button

* fix: spacing

* fix: changes

* chore: refactor closeButtonLabel default

* feat(IconButton): add TypeScript types

* chore(modal): improve types

* fix(Button): adjust typescript types of Button and IconButton

* test(react): update index-test.js with new export

* chore(test): update snapshots

* style(ComposedModal): add new classes to ComposedModal to fix alignment

---------

Co-authored-by: Taylor Jones <taylor.jones826@gmail.com>
Co-authored-by: Taylor Jones <tay1orjones@users.noreply.github.com>
Co-authored-by: Joseph D. Harvey <jdharvey@us.ibm.com>
Co-authored-by: TJ Egan <tw15egan@gmail.com>
Co-authored-by: Joe Harvey <51208233+jdharvey-ibm@users.noreply.github.com>
---
 .../__snapshots__/PublicAPI-test.js.snap      |   6 +
 packages/react/src/__tests__/index-test.js    |   1 +
 .../react/src/components/Button/Button.tsx    |  27 +++--
 .../ComposedModal/ComposedModal.tsx           |   1 -
 .../components/ComposedModal/ModalHeader.tsx  |  25 ++--
 .../components/DangerButton/DangerButton.tsx  |   5 +-
 .../IconButton/{index.js => index.tsx}        | 108 ++++++++++++++++--
 packages/react/src/components/Modal/Modal.tsx |  34 +++---
 .../styles/scss/components/modal/_modal.scss  |  12 +-
 9 files changed, 171 insertions(+), 48 deletions(-)
 rename packages/react/src/components/IconButton/{index.js => index.tsx} (56%)

diff --git a/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap b/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap
index 8b4192c9b5f6..2b9fdb161faa 100644
--- a/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap
+++ b/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap
@@ -4133,6 +4133,12 @@ Map {
     },
     "render": [Function],
   },
+  "IconButtonKinds" => Object {
+    "0": "primary",
+    "1": "secondary",
+    "2": "ghost",
+    "3": "tertiary",
+  },
   "IconSkeleton" => Object {
     "propTypes": Object {
       "className": Object {
diff --git a/packages/react/src/__tests__/index-test.js b/packages/react/src/__tests__/index-test.js
index ad6feeee9876..78eacda17385 100644
--- a/packages/react/src/__tests__/index-test.js
+++ b/packages/react/src/__tests__/index-test.js
@@ -93,6 +93,7 @@ describe('Carbon Components React', () => {
         "HeaderSideNavItems",
         "Heading",
         "IconButton",
+        "IconButtonKinds",
         "IconSkeleton",
         "IconSwitch",
         "IconTab",
diff --git a/packages/react/src/components/Button/Button.tsx b/packages/react/src/components/Button/Button.tsx
index 260a92943ddb..4bd08a0215fc 100644
--- a/packages/react/src/components/Button/Button.tsx
+++ b/packages/react/src/components/Button/Button.tsx
@@ -8,7 +8,7 @@
 import PropTypes from 'prop-types';
 import React, { useRef } from 'react';
 import classNames from 'classnames';
-import { IconButton } from '../IconButton';
+import { IconButton, IconButtonKind } from '../IconButton';
 import { composeEventHandlers } from '../../tools/events';
 import { usePrefix } from '../../internal/usePrefix';
 import { useId } from '../../internal/useId';
@@ -107,11 +107,20 @@ export type ButtonProps<T extends React.ElementType> = PolymorphicProps<
   ButtonBaseProps
 >;
 
-export interface ButtonComponent {
-  <T extends React.ElementType>(
-    props: ButtonProps<T>,
-    context?: any
-  ): React.ReactElement<any, any> | null;
+export type ButtonComponent = <T extends React.ElementType>(
+  props: ButtonProps<T>,
+  context?: any
+) => React.ReactElement<any, any> | null;
+
+function isIconOnlyButton(
+  hasIconOnly: ButtonBaseProps['hasIconOnly'],
+  _kind: ButtonBaseProps['kind']
+): _kind is IconButtonKind {
+  if (hasIconOnly === true) {
+    return true;
+  }
+
+  return false;
 }
 
 const Button = React.forwardRef(function Button<T extends React.ElementType>(
@@ -149,7 +158,6 @@ const Button = React.forwardRef(function Button<T extends React.ElementType>(
     // Prevent clicks on the tooltip from triggering the button click event
     if (evt.target === tooltipRef.current) {
       evt.preventDefault();
-      return;
     }
   };
 
@@ -221,7 +229,7 @@ const Button = React.forwardRef(function Button<T extends React.ElementType>(
     otherProps = anchorProps;
   }
 
-  if (!hasIconOnly) {
+  if (!isIconOnlyButton(hasIconOnly, kind)) {
     return React.createElement(
       component,
       {
@@ -272,7 +280,7 @@ const Button = React.forwardRef(function Button<T extends React.ElementType>(
         {...rest}
         {...commonProps}
         {...otherProps}>
-        {iconOnlyImage ? iconOnlyImage : children}
+        {iconOnlyImage ?? children}
       </IconButton>
     );
   }
@@ -346,6 +354,7 @@ Button.propTypes = {
   /**
    * Specify the kind of Button you want to create
    */
+  // TODO: this should be either ButtonKinds or IconButtonKinds based on the value of "hasIconOnly"
   kind: PropTypes.oneOf(ButtonKinds),
 
   /**
diff --git a/packages/react/src/components/ComposedModal/ComposedModal.tsx b/packages/react/src/components/ComposedModal/ComposedModal.tsx
index 8a44f827ef73..35d3f4ab4dfe 100644
--- a/packages/react/src/components/ComposedModal/ComposedModal.tsx
+++ b/packages/react/src/components/ComposedModal/ComposedModal.tsx
@@ -13,7 +13,6 @@ import { isElement } from 'react-is';
 import PropTypes, { ReactNodeLike } from 'prop-types';
 import { ModalHeader, type ModalHeaderProps } from './ModalHeader';
 import { ModalFooter, type ModalFooterProps } from './ModalFooter';
-
 import cx from 'classnames';
 
 import toggleClass from '../../tools/toggleClass';
diff --git a/packages/react/src/components/ComposedModal/ModalHeader.tsx b/packages/react/src/components/ComposedModal/ModalHeader.tsx
index 1ddbc044454f..0f57c94ba9a2 100644
--- a/packages/react/src/components/ComposedModal/ModalHeader.tsx
+++ b/packages/react/src/components/ComposedModal/ModalHeader.tsx
@@ -14,6 +14,7 @@ import PropTypes from 'prop-types';
 import cx from 'classnames';
 import { Close } from '@carbon/icons-react';
 import { usePrefix } from '../../internal/usePrefix';
+import { IconButton } from '../IconButton';
 
 type DivProps = Omit<HTMLAttributes<HTMLDivElement>, 'title'>;
 export interface ModalHeaderProps extends DivProps {
@@ -127,14 +128,22 @@ export const ModalHeader = React.forwardRef<HTMLDivElement, ModalHeaderProps>(
 
         {children}
 
-        <button
-          onClick={handleCloseButtonClick}
-          className={closeClass}
-          title={iconDescription}
-          aria-label={iconDescription}
-          type="button">
-          <Close size={20} className={closeIconClass} />
-        </button>
+        <div className={`${prefix}--modal-close-button`}>
+          <IconButton
+            className={closeClass}
+            label={iconDescription}
+            onClick={handleCloseButtonClick}
+            title={iconDescription}
+            aria-label={iconDescription}
+            align="left">
+            <Close
+              size={20}
+              aria-hidden="true"
+              tabIndex="-1"
+              className={closeIconClass}
+            />
+          </IconButton>
+        </div>
       </div>
     );
   }
diff --git a/packages/react/src/components/DangerButton/DangerButton.tsx b/packages/react/src/components/DangerButton/DangerButton.tsx
index 5e863c5bf15c..c9cd236783d5 100644
--- a/packages/react/src/components/DangerButton/DangerButton.tsx
+++ b/packages/react/src/components/DangerButton/DangerButton.tsx
@@ -10,6 +10,9 @@ import Button, { ButtonComponent, ButtonProps } from '../Button';
 
 const DangerButton: ButtonComponent = <T extends React.ElementType>(
   props: ButtonProps<T>
-) => <Button kind="danger" {...props} />;
+  // TODO: I got a SonarCloud warning here saying that kind would always be overridden. Is the
+  // expected behavior here to spread the props and then force "kind" to be "danger"? If so,
+  // swapping these props is likely the way to go.
+) => <Button {...props} kind="danger" />;
 
 export default DangerButton;
diff --git a/packages/react/src/components/IconButton/index.js b/packages/react/src/components/IconButton/index.tsx
similarity index 56%
rename from packages/react/src/components/IconButton/index.js
rename to packages/react/src/components/IconButton/index.tsx
index a4bb37662393..f67a459fd6b8 100644
--- a/packages/react/src/components/IconButton/index.js
+++ b/packages/react/src/components/IconButton/index.tsx
@@ -5,16 +5,104 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import PropTypes from 'prop-types';
-import React from 'react';
-import Button from '../Button';
+import PropTypes, { ReactNodeLike } from 'prop-types';
+import React, { ForwardedRef } from 'react';
+import Button, { ButtonSize } from '../Button';
 import classNames from 'classnames';
 import { Tooltip } from '../Tooltip';
 import { usePrefix } from '../../internal/usePrefix';
-import cx from 'classnames';
 
-const IconButton = React.forwardRef(function IconButton(props, ref) {
-  const {
+export const IconButtonKinds = [
+  'primary',
+  'secondary',
+  'ghost',
+  'tertiary',
+] as const;
+
+export type IconButtonKind = (typeof IconButtonKinds)[number];
+
+interface IconButtonProps
+  extends React.ButtonHTMLAttributes<HTMLButtonElement> {
+  /**
+   * Specify how the trigger should align with the tooltip
+   */
+  align?:
+    | 'top'
+    | 'top-left'
+    | 'top-right'
+    | 'bottom'
+    | 'bottom-left'
+    | 'bottom-right'
+    | 'left'
+    | 'right';
+
+  /**
+   * Provide an icon or asset to be rendered inside of the IconButton
+   */
+  children?: React.ReactNode;
+
+  /**
+   * Specify an optional className to be added to your Button
+   */
+  className?: string;
+
+  /**
+   * Determines whether the tooltip should close when inner content is activated (click, Enter or Space)
+   */
+  closeOnActivation?: boolean;
+
+  /**
+   * Specify whether the tooltip should be open when it first renders
+   */
+  defaultOpen?: boolean;
+
+  /**
+   * Specify whether the Button should be disabled, or not
+   */
+  disabled?: boolean;
+
+  /**
+   * Specify the duration in milliseconds to delay before displaying the tooltip
+   */
+  enterDelayMs?: number;
+
+  /**
+   * Specify whether the IconButton is currently selected
+   */
+
+  isSelected?: boolean;
+
+  /**
+   * Specify the type of button to be used as the base for the IconButton
+   */
+  kind?: IconButtonKind;
+
+  /**
+   * Provide the label to be rendered inside of the Tooltip. The label will use
+   * `aria-labelledby` and will fully describe the child node that is provided.
+   * This means that if you have text in the child node it will not be
+   * announced to the screen reader.
+   */
+  label: ReactNodeLike;
+
+  /**
+   * Specify the duration in milliseconds to delay before hiding the tooltip
+   */
+  leaveDelayMs?: number;
+
+  /**
+   * Specify the size of the Button. Defaults to `md`.
+   */
+  size?: ButtonSize;
+
+  /**
+   * Specify an optional className to be added to your Tooltip wrapper
+   */
+  wrapperClasses?: string;
+}
+
+const IconButton = React.forwardRef(function IconButton(
+  {
     align,
     children,
     className,
@@ -29,7 +117,9 @@ const IconButton = React.forwardRef(function IconButton(props, ref) {
     size,
     isSelected,
     ...rest
-  } = props;
+  }: IconButtonProps,
+  ref: ForwardedRef<unknown> // TODO: this is unknown on Button, so should it be here as well?
+) {
   const prefix = usePrefix();
 
   const tooltipClasses = classNames(wrapperClasses, `${prefix}--icon-tooltip`, {
@@ -51,7 +141,7 @@ const IconButton = React.forwardRef(function IconButton(props, ref) {
         kind={kind}
         ref={ref}
         size={size}
-        className={cx(
+        className={classNames(
           `${prefix}--btn--icon-only`,
           {
             [`${prefix}--btn--selected`]: isSelected,
@@ -118,7 +208,7 @@ IconButton.propTypes = {
   /**
    * Specify the type of button to be used as the base for the IconButton
    */
-  kind: PropTypes.oneOf(['primary', 'secondary', 'ghost', 'tertiary']),
+  kind: PropTypes.oneOf(IconButtonKinds),
 
   /**
    * Provide the label to be rendered inside of the Tooltip. The label will use
diff --git a/packages/react/src/components/Modal/Modal.tsx b/packages/react/src/components/Modal/Modal.tsx
index 3284ad207f69..f86fb44286be 100644
--- a/packages/react/src/components/Modal/Modal.tsx
+++ b/packages/react/src/components/Modal/Modal.tsx
@@ -20,6 +20,7 @@ import wrapFocus, {
 import setupGetInstanceId from '../../tools/setupGetInstanceId';
 import { usePrefix } from '../../internal/usePrefix';
 import { keys, match } from '../../internal/keyboard';
+import { IconButton } from '../IconButton';
 import { noopFn } from '../../internal/noopFn';
 import { Text } from '../Text';
 import { ReactAttr } from '../../types/common';
@@ -236,7 +237,7 @@ const Modal = React.forwardRef(function Modal(
     shouldSubmitOnEnter,
     size,
     hasScrollingContent = false,
-    closeButtonLabel,
+    closeButtonLabel = 'Close',
     preventCloseOnClickOutside = false,
     isFullWidth,
     launcherButtonRef,
@@ -434,20 +435,23 @@ const Modal = React.forwardRef(function Modal(
   }
 
   const modalButton = (
-    <button
-      className={modalCloseButtonClass}
-      type="button"
-      onClick={onRequestClose}
-      title={ariaLabel}
-      aria-label={closeButtonLabel ? closeButtonLabel : 'close'}
-      ref={button}>
-      <Close
-        size={20}
-        aria-hidden="true"
-        tabIndex="-1"
-        className={`${modalCloseButtonClass}__icon`}
-      />
-    </button>
+    <div className={`${prefix}--modal-close-button`}>
+      <IconButton
+        className={modalCloseButtonClass}
+        label={closeButtonLabel}
+        onClick={onRequestClose}
+        title={closeButtonLabel}
+        aria-label={closeButtonLabel}
+        align="left"
+        ref={button}>
+        <Close
+          size={20}
+          aria-hidden="true"
+          tabIndex="-1"
+          className={`${modalCloseButtonClass}__icon`}
+        />
+      </IconButton>
+    </div>
   );
 
   const modalBody = (
diff --git a/packages/styles/scss/components/modal/_modal.scss b/packages/styles/scss/components/modal/_modal.scss
index e1cd4736baf5..a5a45e7f1b09 100644
--- a/packages/styles/scss/components/modal/_modal.scss
+++ b/packages/styles/scss/components/modal/_modal.scss
@@ -434,18 +434,20 @@
   // -----------------------------
   // Modal close btn
   // -----------------------------
-  .#{$prefix}--modal-close {
+
+  .#{$prefix}--modal-close-button {
     position: absolute;
-    z-index: 2;
-    overflow: hidden;
+    inset-block-start: 0;
+    inset-inline-end: 0;
+  }
+
+  .#{$prefix}--modal-close {
     padding: convert.to-rem(12px);
     border: 2px solid transparent;
     background-color: transparent;
     block-size: 3rem;
     cursor: pointer;
     inline-size: 3rem;
-    inset-block-start: 0;
-    inset-inline-end: 0;
     transition: background-color $duration-fast-02 motion(standard, productive);
 
     &:hover {