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.

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

fix: stop click event propagation in Modal #1058

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 69 additions & 0 deletions src/components/ConfirmationModal/ConfirmationModal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,73 @@ describe("ConfirmationModal ", () => {
await userEvent.click(screen.getByText("Proceed"));
expect(onConfirm).toHaveBeenCalled();
});

it("should stop click event propagation on cancel by default", async () => {
const handleExternalClick = jest.fn();
render(
<div onClick={handleExternalClick}>
<ConfirmationModal
cancelButtonLabel="Go back"
confirmButtonLabel="Proceed"
onConfirm={jest.fn()}
>
Test click propagation
</ConfirmationModal>
</div>
);

await userEvent.click(screen.getByText("Go back"));
expect(handleExternalClick).not.toHaveBeenCalled();
});

it("should propagate click event on cancel", async () => {
const handleExternalClick = jest.fn();
render(
<div onClick={handleExternalClick}>
<ConfirmationModal
cancelButtonLabel="Go back"
confirmButtonLabel="Proceed"
onConfirm={jest.fn()}
shouldPropagateClickEvent={true}
>
Test click propagation
</ConfirmationModal>
</div>
);

await userEvent.click(screen.getByText("Go back"));
expect(handleExternalClick).toHaveBeenCalledTimes(1);
});

it("should stop click event propagation on confirm by default", async () => {
const handleExternalClick = jest.fn();
render(
<div onClick={handleExternalClick}>
<ConfirmationModal confirmButtonLabel="Proceed" onConfirm={jest.fn()}>
Test click propagation
</ConfirmationModal>
</div>
);

await userEvent.click(screen.getByText("Proceed"));
expect(handleExternalClick).not.toHaveBeenCalled();
});

it("should propagate click event on confirm", async () => {
const handleExternalClick = jest.fn();
render(
<div onClick={handleExternalClick}>
<ConfirmationModal
confirmButtonLabel="Proceed"
onConfirm={jest.fn()}
shouldPropagateClickEvent={true}
>
Test click propagation
</ConfirmationModal>
</div>
);

await userEvent.click(screen.getByText("Proceed"));
expect(handleExternalClick).toHaveBeenCalledTimes(1);
});
});
20 changes: 17 additions & 3 deletions src/components/ConfirmationModal/ConfirmationModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export type Props = PropsWithSpread<
/**
* Function to perform the action prompted by the modal.
*/
onConfirm: (e: MouseEvent<HTMLElement>) => void;
onConfirm: (event: MouseEvent<HTMLElement>) => void;
},
Omit<ModalProps, "buttonRow">
>;
Expand All @@ -43,18 +43,32 @@ export const ConfirmationModal = ({
onConfirm,
...props
}: Props): ReactElement => {
const handleClick =
<A extends Function>(action: A | null | undefined) =>
(event: MouseEvent<HTMLButtonElement>) => {
if (!props.shouldPropagateClickEvent) {
event.stopPropagation();
}
if (action) {
action(event);
}
};

return (
<Modal
buttonRow={
<>
{confirmExtra}
<Button className="u-no-margin--bottom" onClick={props.close}>
<Button
className="u-no-margin--bottom"
onClick={handleClick(props.close)}
>
{cancelButtonLabel}
</Button>
<Button
appearance={confirmButtonAppearance}
className="u-no-margin--bottom"
onClick={onConfirm}
onClick={handleClick(onConfirm)}
>
{confirmButtonLabel}
</Button>
Expand Down
32 changes: 32 additions & 0 deletions src/components/Modal/Modal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,17 @@
Are you sure?
</Modal>
);
expect(container.querySelector(".p-modal__button-row")).toBeNull();

Check warning on line 36 in src/components/Modal/Modal.test.tsx

View workflow job for this annotation

GitHub Actions / Lint, build and test

Avoid using container methods. Prefer using the methods from Testing Library, such as "getByRole()"

Check warning on line 36 in src/components/Modal/Modal.test.tsx

View workflow job for this annotation

GitHub Actions / Lint, build and test

Avoid direct Node access. Prefer using the methods from Testing Library
});

it("does not display a header if a title is not provided", () => {
const { container } = render(<Modal>Bare bones.</Modal>);
expect(container.querySelector(".p-modal__header")).toBeNull();

Check warning on line 41 in src/components/Modal/Modal.test.tsx

View workflow job for this annotation

GitHub Actions / Lint, build and test

Avoid using container methods. Prefer using the methods from Testing Library, such as "getByRole()"

Check warning on line 41 in src/components/Modal/Modal.test.tsx

View workflow job for this annotation

GitHub Actions / Lint, build and test

Avoid direct Node access. Prefer using the methods from Testing Library
});

it("does not display a close button if no close handler is provided", () => {
const { container } = render(<Modal title="some title">Bare bones.</Modal>);
expect(container.querySelector(".p-modal__close")).toBeNull();

Check warning on line 46 in src/components/Modal/Modal.test.tsx

View workflow job for this annotation

GitHub Actions / Lint, build and test

Avoid using container methods. Prefer using the methods from Testing Library, such as "getByRole()"

Check warning on line 46 in src/components/Modal/Modal.test.tsx

View workflow job for this annotation

GitHub Actions / Lint, build and test

Avoid direct Node access. Prefer using the methods from Testing Library
});

it("can pass extra classes to the wrapper", () => {
Expand Down Expand Up @@ -139,4 +139,36 @@
expect(mockHandleModalClose).toHaveBeenCalledTimes(1);
expect(mockHandleEscPress).not.toHaveBeenCalled();
});

it("should stop click event propagation on close by default", async () => {
const handleExternalClick = jest.fn();
const { container } = render(
<div onClick={handleExternalClick}>
<Modal title="Test" close={jest.fn()}>
Bare bones
</Modal>
</div>
);

const closeButton = container.querySelector("button.p-modal__close");
expect(closeButton).not.toBeNull();
await userEvent.click(closeButton!);
expect(handleExternalClick).not.toHaveBeenCalled();
});

it("should propagate click event on close", async () => {
const handleExternalClick = jest.fn();
const { container } = render(
<div onClick={handleExternalClick}>
<Modal title="Test" close={jest.fn()} shouldPropagateClickEvent={true}>
Bare bones
</Modal>
</div>
);

const closeButton = container.querySelector("button.p-modal__close");
expect(closeButton).not.toBeNull();
await userEvent.click(closeButton!);
expect(handleExternalClick).toHaveBeenCalledTimes(1);
});
});
58 changes: 38 additions & 20 deletions src/components/Modal/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ export type Props = PropsWithSpread<
* The title of the modal.
*/
title?: ReactNode | null;
/**
* Whether the button click event should propagate.
*/
shouldPropagateClickEvent?: boolean;
},
HTMLProps<HTMLDivElement>
>;
Expand All @@ -35,6 +39,7 @@ export const Modal = ({
className,
close,
title,
shouldPropagateClickEvent = false,
...wrapperProps
}: Props): ReactElement => {
// list of focusable selectors is based on this Stack Overflow answer:
Expand All @@ -47,37 +52,39 @@ export const Modal = ({

const modalRef: MutableRefObject<HTMLElement> = useRef(null);
const focusableModalElements = useRef(null);
const handleTabKey = (e: React.KeyboardEvent<HTMLDivElement>) => {
const handleTabKey = (event: React.KeyboardEvent<HTMLDivElement>) => {
if (focusableModalElements.current.length > 0) {
const firstElement = focusableModalElements.current[0];
const lastElement =
focusableModalElements.current[
focusableModalElements.current.length - 1
];

if (!e.shiftKey && document.activeElement === lastElement) {
if (!event.shiftKey && document.activeElement === lastElement) {
(firstElement as HTMLElement).focus();
e.preventDefault();
event.preventDefault();
}

if (e.shiftKey && document.activeElement === firstElement) {
if (event.shiftKey && document.activeElement === firstElement) {
(lastElement as HTMLElement).focus();
return e.preventDefault();
return event.preventDefault();
}
}
};

const handleEscKey = (
e: KeyboardEvent | React.KeyboardEvent<HTMLDivElement>
event: KeyboardEvent | React.KeyboardEvent<HTMLDivElement>
) => {
if ("nativeEvent" in e && e.nativeEvent.stopImmediatePropagation) {
e.nativeEvent.stopImmediatePropagation();
} else if ("stopImmediatePropagation" in e) {
e.stopImmediatePropagation();
} else if (e.stopPropagation) {
e.stopPropagation();
if ("nativeEvent" in event && event.nativeEvent.stopImmediatePropagation) {
event.nativeEvent.stopImmediatePropagation();
} else if ("stopImmediatePropagation" in event) {
event.stopImmediatePropagation();
} else if (event.stopPropagation) {
event.stopPropagation();
}
if (close) {
close();
}
close();
};

const keyListenersMap = new Map([
Expand All @@ -102,9 +109,9 @@ export const Modal = ({
}, [close]);

useEffect(() => {
const keyDown = (e) => {
const listener = keyListenersMap.get(e.code);
return listener && listener(e);
const keyDown = (event: KeyboardEvent) => {
const listener = keyListenersMap.get(event.code);
return listener && listener(event);
};

document.addEventListener("keydown", keyDown);
Expand All @@ -121,18 +128,29 @@ export const Modal = ({
shouldClose.current = false;
};

const handleOverlayOnMouseDown = (event) => {
const handleOverlayOnMouseDown = (
event: React.MouseEvent<HTMLDivElement>
) => {
if (event.target === modalRef.current) {
shouldClose.current = true;
}
};

const handleOverlayOnClick = () => {
if (shouldClose.current) {
const handleClose = (event: React.MouseEvent) => {
if (!shouldPropagateClickEvent) {
event.stopPropagation();
}
if (close) {
close();
}
};

const handleOverlayOnClick = (event: React.MouseEvent<HTMLDivElement>) => {
if (shouldClose.current) {
handleClose(event);
}
};

return (
<div
className={classNames("p-modal", className)}
Expand All @@ -159,7 +177,7 @@ export const Modal = ({
<button
className="p-modal__close"
aria-label="Close active modal"
onClick={close}
onClick={handleClose}
>
Close
</button>
Expand Down