Skip to content

Commit

Permalink
feat(Modal): allow accepting custom arias, allow passing ReactNode to…
Browse files Browse the repository at this point in the history 
… ModalHeader's title (#2702)
  • Loading branch information
@YossiSaadi
YossiSaadi authored Jan 6, 2025
1 parent 1bb3fbe commit 8201d7f
Show file tree
Hide file tree
Showing 6 changed files with 180 additions and 69 deletions.
24 changes: 19 additions & 5 deletions packages/core/src/components/Modal/Modal/Modal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ const Modal = forwardRef(
style,
zIndex,
className,
"data-testid": dataTestId
"data-testid": dataTestId,
"aria-labelledby": ariaLabelledby,
"aria-describedby": ariaDescribedby
}: ModalProps,
ref: React.ForwardedRef<HTMLDivElement>
) => {
Expand All @@ -56,8 +58,20 @@ const Modal = forwardRef(
const [titleId, setTitleId] = useState<string>();
const [descriptionId, setDescriptionId] = useState<string>();

const setTitleIdCallback = useCallback((id: string) => setTitleId(id), []);
const setDescriptionIdCallback = useCallback((id: string) => setDescriptionId(id), []);
const setTitleIdCallback = useCallback(
(newId: string) => {
if (ariaLabelledby) return;
setTitleId(newId);
},
[ariaLabelledby]
);
const setDescriptionIdCallback = useCallback(
(newId: string) => {
if (ariaDescribedby) return;
setDescriptionId(newId);
},
[ariaDescribedby]
);

const contextValue = useMemo<ModalProviderValue>(
() => ({
Expand Down Expand Up @@ -128,8 +142,8 @@ const Modal = forwardRef(
data-testid={dataTestId || getTestId(ComponentDefaultTestId.MODAL_NEXT, id)}
role="dialog"
aria-modal
aria-labelledby={titleId}
aria-describedby={descriptionId}
aria-labelledby={ariaLabelledby || titleId}
aria-describedby={ariaDescribedby || descriptionId}
style={modalStyle}
onKeyDown={onModalKeyDown}
tabIndex={-1}
Expand Down
8 changes: 8 additions & 0 deletions packages/core/src/components/Modal/Modal/Modal.types.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,12 @@ export interface ModalProps extends VibeComponentProps {
* The z-index to be used for the modal and overlay.
*/
zIndex?: number;
/**
* If provided, overrides the automatically generated aria-labelledby, that is assigned when used with ModalHeader.
*/
"aria-labelledby"?: string;
/**
* If provided, overrides the automatically generated aria-describedby, that is assigned when used with ModalHeader.
*/
"aria-describedby"?: string;
}
136 changes: 100 additions & 36 deletions packages/core/src/components/Modal/Modal/__tests__/Modal.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { render, fireEvent } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import Modal from "../Modal";
import ModalContent from "../../ModalContent/ModalContent";
import ModalHeader from "../../ModalHeader/ModalHeader";

jest.mock("framer-motion", () => {
const actual = jest.requireActual<typeof import("framer-motion")>("framer-motion");
Expand All @@ -21,7 +22,7 @@ describe("Modal", () => {
<span>My content</span>
</div>
);
it("renders the modal with the correct role", () => {
it("should render the modal with the correct role", () => {
const { getByTestId } = render(
<Modal id={id} show data-testid="modal">
{childrenContent}
Expand All @@ -31,7 +32,7 @@ describe("Modal", () => {
expect(getByTestId("modal")).toHaveAttribute("role", "dialog");
});

it("renders the modal with the correct aria-modal", () => {
it("should render the modal with the correct aria-modal", () => {
const { getByTestId } = render(
<Modal id={id} show data-testid="modal">
{childrenContent}
Expand All @@ -41,7 +42,7 @@ describe("Modal", () => {
expect(getByTestId("modal")).toHaveAttribute("aria-modal", "true");
});

it("does not render when 'show' is false", () => {
it("should not render when 'show' is false", () => {
const { queryByRole } = render(
<Modal id={id} show={false}>
{childrenContent}
Expand All @@ -51,7 +52,7 @@ describe("Modal", () => {
expect(queryByRole("dialog")).not.toBeInTheDocument();
});

it("renders the children content correctly", () => {
it("should render the children content correctly", () => {
const { getByText } = render(
<Modal id={id} show>
{childrenContent}
Expand All @@ -61,7 +62,7 @@ describe("Modal", () => {
expect(getByText("My content")).toBeInTheDocument();
});

it("ensures the ref prop does not return null when modal is shown", () => {
it("should ensure the ref prop does not return null when modal is shown", () => {
const ref = React.createRef<HTMLDivElement>();

const { getByTestId } = render(
Expand All @@ -74,7 +75,7 @@ describe("Modal", () => {
expect(ref.current).not.toBeNull();
});

it("applies default size as 'medium' when not supplied with a size", () => {
it("should apply default size as 'medium' when not supplied with a size", () => {
const { getByRole } = render(
<Modal id={id} show>
{childrenContent}
Expand All @@ -84,7 +85,7 @@ describe("Modal", () => {
expect(getByRole("dialog")).toHaveClass("sizeMedium");
});

it("applies the correct given 'large' size", () => {
it("should apply the correct given 'large' size", () => {
const { getByRole } = render(
<Modal id={id} show size="large">
{childrenContent}
Expand All @@ -94,7 +95,7 @@ describe("Modal", () => {
expect(getByRole("dialog")).toHaveClass("sizeLarge");
});

it("calls onClose when the close button is clicked with mouse", () => {
it("should call onClose when the close button is clicked with mouse", () => {
const mockOnClose = jest.fn();
const { getByLabelText } = render(
<Modal id={id} show onClose={mockOnClose} closeButtonAriaLabel={closeButtonAriaLabel}>
Expand All @@ -106,7 +107,7 @@ describe("Modal", () => {
expect(mockOnClose).toHaveBeenCalled();
});

it("calls onClose when the close button is clicked with keyboard", () => {
it("should call onClose when the close button is clicked with keyboard", () => {
const mockOnClose = jest.fn();
const { getByLabelText } = render(
<Modal id={id} show onClose={mockOnClose} closeButtonAriaLabel={closeButtonAriaLabel}>
Expand All @@ -119,7 +120,7 @@ describe("Modal", () => {
expect(mockOnClose).toHaveBeenCalled();
});

it("calls onClose when the backdrop is clicked", () => {
it("should call onClose when the backdrop is clicked", () => {
const mockOnClose = jest.fn();
const { getByTestId } = render(
<Modal id={id} show onClose={mockOnClose}>
Expand All @@ -131,7 +132,7 @@ describe("Modal", () => {
expect(mockOnClose).toHaveBeenCalled();
});

it("calls onClose when the Escape key is pressed while modal loads with auto-focusable content", () => {
it("should call onClose when the Escape key is pressed while modal loads with auto-focusable content", () => {
const mockOnClose = jest.fn();
render(
<Modal id={id} show onClose={mockOnClose}>
Expand All @@ -143,7 +144,7 @@ describe("Modal", () => {
expect(mockOnClose).toHaveBeenCalled();
});

it("calls onClose when the Escape key is pressed while modal loads without an auto-focusable content", () => {
it("should call onClose when the Escape key is pressed while modal loads without an auto-focusable content", () => {
const mockOnClose = jest.fn();
render(
<Modal id={id} show onClose={mockOnClose}>
Expand All @@ -155,7 +156,7 @@ describe("Modal", () => {
expect(mockOnClose).toHaveBeenCalled();
});

it("closes only the top most modal when Escape is pressed with multiple modals open", () => {
it("should close only the top most modal when Escape is pressed with multiple modals open", () => {
const mockOnCloseModal1 = jest.fn();
const mockOnCloseModal2 = jest.fn();

Expand All @@ -176,7 +177,7 @@ describe("Modal", () => {
expect(mockOnCloseModal2).toHaveBeenCalled();
});

it("traps focus inside the modal when opened and move it to first non top-actions element", () => {
it("should trap focus inside the modal when opened and move it to first non top-actions element", () => {
const { getByText, getByLabelText } = render(
<>
<button type="button">Focusable outside</button>
Expand All @@ -192,7 +193,7 @@ describe("Modal", () => {
expect(getByText("Test button content")).toHaveFocus();
});

it("releases focus lock inside the modal when closed", () => {
it("should release focus lock from inside the modal when closed", () => {
const { rerender, getByText } = render(
<>
<button type="button">Focusable outside 1</button>
Expand Down Expand Up @@ -240,32 +241,95 @@ describe("Modal", () => {
expect(getByText("Focusable 1")).toHaveFocus();
});

it("traps and moves focus to focusable element inside ModalContent and cycle through full focus flow", () => {
const { getByLabelText, getByText } = render(
<Modal id={id} show closeButtonAriaLabel={closeButtonAriaLabel}>
<button type="button">Focusable 1</button>
<ModalContent>
<button type="button">Focusable inside ModalContent</button>
</ModalContent>
<button type="button">Focusable 2</button>
</Modal>
);
expect(getByText("Focusable inside ModalContent")).toHaveFocus();
describe("integrated with ModalContent", () => {
it("should trap and moves focus to focusable element inside ModalContent and to cycle through full focus flow", () => {
const { getByLabelText, getByText } = render(
<Modal id={id} show closeButtonAriaLabel={closeButtonAriaLabel}>
<button type="button">Focusable 1</button>
<ModalContent>
<button type="button">Focusable inside ModalContent</button>
</ModalContent>
<button type="button">Focusable 2</button>
</Modal>
);
expect(getByText("Focusable inside ModalContent")).toHaveFocus();

userEvent.tab();
expect(getByText("Focusable 2")).toHaveFocus();
userEvent.tab();
expect(getByText("Focusable 2")).toHaveFocus();

userEvent.tab();
expect(getByLabelText(closeButtonAriaLabel)).toHaveFocus();
userEvent.tab();
expect(getByLabelText(closeButtonAriaLabel)).toHaveFocus();

userEvent.tab();
expect(getByText("Focusable 1")).toHaveFocus();
userEvent.tab();
expect(getByText("Focusable 1")).toHaveFocus();

userEvent.tab();
expect(getByText("Focusable inside ModalContent")).toHaveFocus();
userEvent.tab();
expect(getByText("Focusable inside ModalContent")).toHaveFocus();
});
});

it.todo("renders the correct aria-labelledby");
describe("integrated with ModalHeader", () => {
it("should use auto-generated aria-labelledby when none is provided", () => {
const { getByRole } = render(
<Modal show id={id}>
<ModalHeader title="Title from Header" />
</Modal>
);

expect(getByRole("dialog")).toHaveAttribute("aria-labelledby", `${id}_label`);
});

it("should use auto-generated aria-describedby when none is provided", () => {
const { getByRole } = render(
<Modal show id={id}>
<ModalHeader title="Title" description="Some description" />
</Modal>
);

expect(getByRole("dialog")).toHaveAttribute("aria-describedby", `${id}_desc`);
});

it("should respect user-provided aria-labelledby and should not use the auto-generated ID", () => {
const customAriaLabelId = "myCustomTitleId";
const { getByRole } = render(
<Modal show id={id} aria-labelledby={customAriaLabelId}>
<ModalHeader title="Header Title" />
</Modal>
);

expect(getByRole("dialog")).toHaveAttribute("aria-labelledby", customAriaLabelId);
});

it("should respect user-provided aria-describedby and should not generate an ID", () => {
const customAriaDescId = "myCustomDescriptionId";
const { getByRole } = render(
<Modal show id={id} aria-describedby={customAriaDescId}>
<ModalHeader title="Header Title" description="I am a description" />
</Modal>
);

expect(getByRole("dialog")).toHaveAttribute("aria-describedby", customAriaDescId);
});

it("should respect user-provided aria-describedby even if description isn't supplied to ModalHeader", () => {
const customAriaDescId = "myCustomDescriptionId";
const { getByRole } = render(
<Modal show id={id} aria-describedby={customAriaDescId}>
<ModalHeader title="Header Title" />
</Modal>
);

it.todo("renders the correct aria-describedby");
expect(getByRole("dialog")).toHaveAttribute("aria-describedby", customAriaDescId);
});

it("should not generate aria-describedby if there is no description in ModalHeader and the user provided none", () => {
const { getByRole } = render(
<Modal show id={id}>
<ModalHeader title="Just a title, no description" />
</Modal>
);

expect(getByRole("dialog")).not.toHaveAttribute("aria-describedby");
});
});
});
11 changes: 8 additions & 3 deletions packages/core/src/components/Modal/ModalHeader/ModalHeader.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,14 @@ const ModalHeader = forwardRef(
id={id}
data-testid={dataTestId || getTestId(ComponentDefaultTestId.MODAL_NEXT_HEADER, id)}
>
<Heading id={titleId} align="inherit" type="h2" weight="medium" maxLines={2} className={styles.title}>
{title}
</Heading>
{typeof title === "string" ? (
<Heading id={titleId} align="inherit" type="h2" weight="medium" maxLines={2} className={styles.title}>
{title}
</Heading>
) : (
title
)}

{description && (
<Flex id={descriptionId}>
{descriptionIcon && (
Expand Down
19 changes: 16 additions & 3 deletions packages/core/src/components/Modal/ModalHeader/ModalHeader.types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ interface WithoutDescription {
interface WithDescription {
/**
* Descriptive text or content below the title.
* When supplied, would also add an aria-describedby attribute to the modal dialog element.
* - If you pass a **string**, this will automatically set an internally generated `aria-describedby` on the parent Modal.
* - If you pass a **ReactNode** (e.g., a complex component), we recommend assigning an **`id`** to that component (or a nested element),
* and then pass that same ID in `aria-describedby` to the **Modal** (overriding the internal ID).
*
* This ensures that assistive technologies know which element is the modal's descriptive content.
* @see [WAI-ARIA Authoring Practices for Dialog (Modal)](https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/#wai-ariaroles,states,andproperties)
*/
description: string | React.ReactNode;
/**
Expand All @@ -26,8 +31,16 @@ interface WithDescription {
export type ModalHeaderProps = {
/**
* Main heading text of the modal.
* When supplied, would also add an aria-labelledby attribute to the modal dialog element.
*
* - If you pass a **string**, `ModalHeader` will generate an internal ID and communicate it to the parent `Modal`
* so that `aria-labelledby` is set automatically (unless `Modal` receives `aria-labelledby` prop).
* - If you pass a **ReactNode** (such as a custom component), **you must**:
* 1. Assign an **`id`** to that element (or a nested element), and
* 2. Pass that **same `id`** as the `aria-labelledby` prop to the `Modal`.
*
* This ensures that assistive technologies know which element is the modal's title.
* @see [WAI-ARIA Authoring Practices for Dialog (Modal)](https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/#wai-ariaroles,states,andproperties)
*/
title: string;
title: string | React.ReactNode;
} & (WithDescription | WithoutDescription) &
VibeComponentProps;
Loading

0 comments on commit 8201d7f

Please sign in to comment.