Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Modal): allow accepting custom arias, allow passing ReactNode to ModalHeader's title #2702

Merged
merged 2 commits into from
Jan 6, 2025
Merged

feat(Modal): allow accepting custom arias, allow passing ReactNode to ModalHeader's title #2702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ade6acd
feat(ModalHeader): allow passing ReactNode to modal's title 8129253861
YossiSaadi Jan 5, 2025
c174bef
feat(Modal): allow passing custom aria-labelledby and aria-describedb…
YossiSaadi Jan 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions packages/core/src/components/Modal/Modal/Modal.tsx
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, very detailed explanation

* - 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ElementContent? Or we don't want it because it's less descriptive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can it be an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good point, let's see if a request arises, and we can add it later.

I believe most of the use cases would be wrapped under a single div, to flex it and position it properly, and in worst case people can just use fragment.

I generally don't like the ElementContent rather than just saying ReactNode, I think it is a bit redundant.

} & (WithDescription | WithoutDescription) &
VibeComponentProps;
Loading
Loading