diff --git a/packages/core/src/components/Modal/Modal/Modal.tsx b/packages/core/src/components/Modal/Modal/Modal.tsx index 9a85966f85..720e1320f5 100644 --- a/packages/core/src/components/Modal/Modal/Modal.tsx +++ b/packages/core/src/components/Modal/Modal/Modal.tsx @@ -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 ) => { @@ -56,8 +58,20 @@ const Modal = forwardRef( const [titleId, setTitleId] = useState(); const [descriptionId, setDescriptionId] = useState(); - 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( () => ({ @@ -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} diff --git a/packages/core/src/components/Modal/Modal/Modal.types.tsx b/packages/core/src/components/Modal/Modal/Modal.types.tsx index 61765264a3..a732be78b6 100644 --- a/packages/core/src/components/Modal/Modal/Modal.types.tsx +++ b/packages/core/src/components/Modal/Modal/Modal.types.tsx @@ -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; } diff --git a/packages/core/src/components/Modal/Modal/__tests__/Modal.test.tsx b/packages/core/src/components/Modal/Modal/__tests__/Modal.test.tsx index a56c9e2114..249328e2d4 100644 --- a/packages/core/src/components/Modal/Modal/__tests__/Modal.test.tsx +++ b/packages/core/src/components/Modal/Modal/__tests__/Modal.test.tsx @@ -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("framer-motion"); @@ -21,7 +22,7 @@ describe("Modal", () => { My content ); - it("renders the modal with the correct role", () => { + it("should render the modal with the correct role", () => { const { getByTestId } = render( {childrenContent} @@ -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( {childrenContent} @@ -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( {childrenContent} @@ -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( {childrenContent} @@ -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(); const { getByTestId } = render( @@ -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( {childrenContent} @@ -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( {childrenContent} @@ -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( @@ -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( @@ -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( @@ -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( @@ -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( @@ -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(); @@ -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( <> @@ -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( <> @@ -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( - - - - - - - - ); - 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( + + + + + + + + ); + 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( + + + + ); + + expect(getByRole("dialog")).toHaveAttribute("aria-labelledby", `${id}_label`); + }); + + it("should use auto-generated aria-describedby when none is provided", () => { + const { getByRole } = render( + + + + ); + + 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( + + + + ); + + 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( + + + + ); + + 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( + + + + ); - 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( + + + + ); + + expect(getByRole("dialog")).not.toHaveAttribute("aria-describedby"); + }); + }); }); diff --git a/packages/core/src/components/Modal/ModalHeader/ModalHeader.tsx b/packages/core/src/components/Modal/ModalHeader/ModalHeader.tsx index 5ac973b5eb..153c1bcf86 100644 --- a/packages/core/src/components/Modal/ModalHeader/ModalHeader.tsx +++ b/packages/core/src/components/Modal/ModalHeader/ModalHeader.tsx @@ -36,9 +36,14 @@ const ModalHeader = forwardRef( id={id} data-testid={dataTestId || getTestId(ComponentDefaultTestId.MODAL_NEXT_HEADER, id)} > - - {title} - + {typeof title === "string" ? ( + + {title} + + ) : ( + title + )} + {description && ( {descriptionIcon && ( diff --git a/packages/core/src/components/Modal/ModalHeader/ModalHeader.types.ts b/packages/core/src/components/Modal/ModalHeader/ModalHeader.types.ts index 61aa13250c..181a315c5c 100644 --- a/packages/core/src/components/Modal/ModalHeader/ModalHeader.types.ts +++ b/packages/core/src/components/Modal/ModalHeader/ModalHeader.types.ts @@ -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; /** @@ -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; diff --git a/packages/core/src/components/Modal/ModalHeader/__tests__/ModalHeader.test.tsx b/packages/core/src/components/Modal/ModalHeader/__tests__/ModalHeader.test.tsx index f706c7579c..fda0f2bd79 100644 --- a/packages/core/src/components/Modal/ModalHeader/__tests__/ModalHeader.test.tsx +++ b/packages/core/src/components/Modal/ModalHeader/__tests__/ModalHeader.test.tsx @@ -13,7 +13,6 @@ const useModalMocked = jest.mocked(useModal); describe("ModalHeader", () => { const title = "Test Modal Header"; const simpleDescription = "This is a description"; - const descriptionIcon = TextIcon; const useModalMockedReturnedValue: ModalContextProps = { modalId: "modal-id", @@ -25,28 +24,37 @@ describe("ModalHeader", () => { useModalMocked.mockReturnValue(useModalMockedReturnedValue); }); - it("renders the title correctly", () => { - const { getByText } = render(); + it("should render a Heading component when title is a string", () => { + const { getByRole } = render(); + const headingElement = getByRole("heading", { name: title }); + expect(headingElement).toBeInTheDocument(); + }); + + it("should not wrap in Heading if title is a ReactNode", () => { + const CustomTitle = () =>
My Custom Title
; - expect(getByText(title)).toBeInTheDocument(); + const { getByTestId, queryByRole } = render(} />); + + expect(getByTestId("custom-title")).toBeInTheDocument(); + expect(queryByRole("heading")).not.toBeInTheDocument(); }); - it("renders the description correctly", () => { + it("should render the description correctly", () => { const { getByText } = render(); expect(getByText(simpleDescription)).toBeInTheDocument(); }); - it("renders the description icon when provided", () => { + it("should render the description icon when provided", () => { const { getByText, getByTestId } = render( - + ); expect(getByText(simpleDescription)).toBeInTheDocument(); expect(getByTestId("icon")).toBeInTheDocument(); }); - it("renders custom description node", () => { + it("should render custom description node", () => { const customDescription = Custom description content; const { getByTestId } = render(); @@ -54,13 +62,13 @@ describe("ModalHeader", () => { expect(getByTestId("custom-description")).toBeInTheDocument(); }); - it("does not render description when not provided", () => { + it("should not render description when not provided", () => { const { queryByText } = render(); expect(queryByText(simpleDescription)).not.toBeInTheDocument(); }); - it("renders with description icon when descriptionIcon is an object", () => { + it("should render with description icon when descriptionIcon is an object", () => { const descriptionIconObject = { name: TextIcon, className: "with-custom-icon-class" @@ -74,37 +82,36 @@ describe("ModalHeader", () => { expect(icon).toHaveClass(descriptionIconObject.className); }); - it("sets the titleId and descriptionId in the context when rendered", () => { - const { getByText } = render(); - + it("should call setTitleId if modalId is available and title is provided", () => { + render(); expect(useModalMockedReturnedValue.setTitleId).toHaveBeenCalledWith("modal-id_label"); - expect(useModalMockedReturnedValue.setDescriptionId).toHaveBeenCalledWith("modal-id_desc"); - expect(getByText(title)).toBeInTheDocument(); - expect(getByText(simpleDescription)).toBeInTheDocument(); }); - it("does not set descriptionId if no description is provided", () => { - render(); + it("should call setDescriptionId if modalId is available and description is provided", () => { + render(); + expect(useModalMockedReturnedValue.setDescriptionId).toHaveBeenCalledWith("modal-id_desc"); + }); - expect(useModalMockedReturnedValue.setTitleId).toHaveBeenCalledWith("modal-id_label"); + it("should not call setDescriptionId if no description is provided", () => { + render(); expect(useModalMockedReturnedValue.setDescriptionId).not.toHaveBeenCalled(); }); - it("renders the title with the correct id", () => { + it("should renders the title with the correct id", () => { const { getByText } = render(); const titleElement = getByText(title); expect(titleElement).toHaveAttribute("id", "modal-id_label"); }); - it("renders the description container with the correct id when provided", () => { + it("should render the description container with the correct id when provided", () => { const { getByText } = render(); const descriptionElement = getByText(simpleDescription); expect(descriptionElement.parentElement).toHaveAttribute("id", "modal-id_desc"); }); - it("calls setTitleId and setDescriptionId with a custom id if provided", () => { + it("should call setTitleId and setDescriptionId with a custom id if provided", () => { const customId = "custom-header-id"; render();