Skip to content

fix(modal/drawer): prevent modal/drawer from closing on press instead… #5623

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

Closed
anshumandev2025 wants to merge 4 commits into from
Closed

fix(modal/drawer): prevent modal/drawer from closing on press instead… #5623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d7e6e5
fix(modal/drawer): prevent modal/drawer from closing on press instead…
anshumandev2025 Aug 23, 2025
23bbbc4
docs: add changeset for modal/drawer interaction fix
anshumandev2025 Aug 23, 2025
fe85fd1
test(modal/drawer): correctly simulate outside pointer events in Moda…
anshumandev2025 Aug 23, 2025
ed5be13
fix(useAriaOverlay): handle edge case for select and modal/drawer to …
anshumandev2025 Aug 23, 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
7 changes: 7 additions & 0 deletions .changeset/shy-garlics-peel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@heroui/use-aria-overlay": patch
---

fix: prevent premature closing of modal/drawer on press start

Removes `onHide()` from `onInteractOutsideStart` to fix an issue where modals and drawers closed as soon as the user pressed outside them, instead of after a full press and release.
24 changes: 24 additions & 0 deletions packages/components/drawer/__tests__/drawer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,28 @@ describe("Drawer", () => {
fireEvent.keyDown(drawer, {key: "Escape"});
expect(onClose).toHaveBeenCalledTimes(1);
});

test("should not close drawer on press start outside, only on release", async () => {
const onClose = jest.fn();
const user = userEvent.setup();

render(
<>
<div data-testid="outside">Outside</div>
<Drawer isOpen onClose={onClose}>
<DrawerContent>
<DrawerHeader>Drawer header</DrawerHeader>
<DrawerBody>Drawer body</DrawerBody>
<DrawerFooter>Drawer footer</DrawerFooter>
</DrawerContent>
</Drawer>
</>,
);

// Simulate full click outside (press + release)
await user.pointer({keys: "[MouseLeft>]", target: document.body});
await user.pointer({keys: "[/MouseLeft]", target: document.body});

expect(onClose).toHaveBeenCalledTimes(1);
});
Comment on lines +114 to +136

@coderabbitai coderabbitai Bot Aug 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Same gap as Modal test: assert zero calls on press start and click an explicit outside node.

The test’s final “called once” assertion doesn’t guard against a premature call on press start. Add an assertion after the pointer down, and target the outside div instead of document.body to make intent unambiguous.

Apply this diff:

-    render(
+    const {getByTestId} = render(
       <>
         <div data-testid="outside">Outside</div>
         <Drawer isOpen onClose={onClose}>
           <DrawerContent>
             <DrawerHeader>Drawer header</DrawerHeader>
             <DrawerBody>Drawer body</DrawerBody>
             <DrawerFooter>Drawer footer</DrawerFooter>
           </DrawerContent>
         </Drawer>
       </>,
     );
 
-    // Simulate full click outside (press + release)
-    await user.pointer({keys: "[MouseLeft>]", target: document.body});
+    const outside = getByTestId("outside");
+    // Simulate full click outside (press + release)
+    await user.pointer({keys: "[MouseLeft>]", target: outside});
+    // Must not close on press start
+    expect(onClose).toHaveBeenCalledTimes(0);
-    await user.pointer({keys: "[/MouseLeft]", target: document.body});
+    await user.pointer({keys: "[/MouseLeft]", target: outside});
 
     expect(onClose).toHaveBeenCalledTimes(1);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test("should not close drawer on press start outside, only on release", async () => {
const onClose = jest.fn();
const user = userEvent.setup();
render(
<>
<div data-testid="outside">Outside</div>
<Drawer isOpen onClose={onClose}>
<DrawerContent>
<DrawerHeader>Drawer header</DrawerHeader>
<DrawerBody>Drawer body</DrawerBody>
<DrawerFooter>Drawer footer</DrawerFooter>
</DrawerContent>
</Drawer>
</>,
);
// Simulate full click outside (press + release)
await user.pointer({keys: "[MouseLeft>]", target: document.body});
await user.pointer({keys: "[/MouseLeft]", target: document.body});
expect(onClose).toHaveBeenCalledTimes(1);
});
test("should not close drawer on press start outside, only on release", async () => {
const onClose = jest.fn();
const user = userEvent.setup();
const { getByTestId } = render(
<>
<div data-testid="outside">Outside</div>
<Drawer isOpen onClose={onClose}>
<DrawerContent>
<DrawerHeader>Drawer header</DrawerHeader>
<DrawerBody>Drawer body</DrawerBody>
<DrawerFooter>Drawer footer</DrawerFooter>
</DrawerContent>
</Drawer>
</>,
);
const outside = getByTestId("outside");
// Simulate full click outside (press + release)
await user.pointer({ keys: "[MouseLeft>]", target: outside });
// Must not close on press start
expect(onClose).toHaveBeenCalledTimes(0);
await user.pointer({ keys: "[/MouseLeft]", target: outside });
expect(onClose).toHaveBeenCalledTimes(1);
});
🤖 Prompt for AI Agents 
In packages/components/drawer/__tests__/drawer.test.tsx around lines 114 to 136,
the test currently only asserts onClose was called once after a full click on
document.body, which doesn't ensure the handler wasn't invoked on the initial
pointer down and uses an ambiguous target; update the test to (1) use the
explicit outside div as the pointer target, (2) after the pointer down event
assert onClose has not been called, and (3) then issue the pointer up and assert
onClose was called exactly once.

});
24 changes: 24 additions & 0 deletions packages/components/modal/__tests__/modal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,28 @@ describe("Modal", () => {
expect(document.documentElement.clientHeight).toBe(1080);
expect(modal.style.transform).toBe("translate(2000px, 1500px)");
});

test("should not close modal on press start outside, only on release", async () => {
const onClose = jest.fn();
const user = userEvent.setup();

render(
<>
<div data-testid="outside">Outside</div>
<Modal isOpen onClose={onClose}>
<ModalContent>
<ModalHeader>Modal header</ModalHeader>
<ModalBody>Modal body</ModalBody>
<ModalFooter>Modal footer</ModalFooter>
</ModalContent>
</Modal>
</>,
);

// Simulate full click outside (press + release)
await user.pointer({keys: "[MouseLeft>]", target: document.body});
await user.pointer({keys: "[/MouseLeft]", target: document.body});

expect(onClose).toHaveBeenCalledTimes(1);
});
Comment on lines +219 to +241

@coderabbitai coderabbitai Bot Aug 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Test doesn’t prove “no close on press start” — it would pass even if close triggers on press.

Right now you only assert a single call after release. If onClose fires on press start and not on release, the test still passes (1 call total). Add an assertion immediately after pointer down to ensure zero calls, and target the explicit outside element for clarity (instead of document.body).

Apply this diff to strengthen the test:

-    render(
+    const {getByTestId} = render(
       <>
         <div data-testid="outside">Outside</div>
         <Modal isOpen onClose={onClose}>
           <ModalContent>
             <ModalHeader>Modal header</ModalHeader>
             <ModalBody>Modal body</ModalBody>
             <ModalFooter>Modal footer</ModalFooter>
           </ModalContent>
         </Modal>
       </>,
     );
 
-    // Simulate full click outside (press + release)
-    await user.pointer({keys: "[MouseLeft>]", target: document.body});
+    const outside = getByTestId("outside");
+    // Simulate full click outside (press + release)
+    await user.pointer({keys: "[MouseLeft>]", target: outside});
+    // Must not close on press start
+    expect(onClose).toHaveBeenCalledTimes(0);
-    await user.pointer({keys: "[/MouseLeft]", target: document.body});
+    await user.pointer({keys: "[/MouseLeft]", target: outside});
 
     expect(onClose).toHaveBeenCalledTimes(1);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test("should not close modal on press start outside, only on release", async () => {
const onClose = jest.fn();
const user = userEvent.setup();
render(
<>
<div data-testid="outside">Outside</div>
<Modal isOpen onClose={onClose}>
<ModalContent>
<ModalHeader>Modal header</ModalHeader>
<ModalBody>Modal body</ModalBody>
<ModalFooter>Modal footer</ModalFooter>
</ModalContent>
</Modal>
</>,
);
// Simulate full click outside (press + release)
await user.pointer({keys: "[MouseLeft>]", target: document.body});
await user.pointer({keys: "[/MouseLeft]", target: document.body});
expect(onClose).toHaveBeenCalledTimes(1);
});
test("should not close modal on press start outside, only on release", async () => {
const onClose = jest.fn();
const user = userEvent.setup();
const { getByTestId } = render(
<>
<div data-testid="outside">Outside</div>
<Modal isOpen onClose={onClose}>
<ModalContent>
<ModalHeader>Modal header</ModalHeader>
<ModalBody>Modal body</ModalBody>
<ModalFooter>Modal footer</ModalFooter>
</ModalContent>
</Modal>
</>,
);
const outside = getByTestId("outside");
// Simulate full click outside (press + release)
await user.pointer({ keys: "[MouseLeft>]", target: outside });
// Must not close on press start
expect(onClose).toHaveBeenCalledTimes(0);
await user.pointer({ keys: "[/MouseLeft]", target: outside });
expect(onClose).toHaveBeenCalledTimes(1);
});
🤖 Prompt for AI Agents 
In packages/components/modal/__tests__/modal.test.tsx around lines 219 to 241,
the test only asserts onClose after the full click so it would still pass if
onClose fired on press; change the test to (1) target the explicit outside
element (query by data-testid "outside") instead of document.body, (2) perform
the pointer down via user.pointer({keys: "[MouseLeft>]", target:
outsideElement}), then immediately assert
expect(onClose).not.toHaveBeenCalled(), and (3) perform the pointer up via
user.pointer({keys: "[/MouseLeft]", target: outsideElement}) and assert
expect(onClose).toHaveBeenCalledTimes(1).

});
18 changes: 18 additions & 0 deletions packages/hooks/use-aria-overlay/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ export function useAriaOverlay(props: UseAriaOverlayProps, ref: RefObject<Elemen
e.preventDefault();
}
}
}

if (getOverlayTypeFromRef(ref) !== "modalOrDrawer") {
onHide();
}
};
Expand Down Expand Up @@ -130,6 +133,21 @@ export function useAriaOverlay(props: UseAriaOverlayProps, ref: RefObject<Elemen
}
};

const getOverlayTypeFromRef = (
ref: RefObject<Element>,
): "modalOrDrawer" | "select" | "unknown" => {
const el = ref.current;

if (!el) return "unknown";

const tag = el.tagName.toLowerCase();

if (tag === "section") return "modalOrDrawer";
if (tag === "div") return "select"; // assumption based on current usage

return "unknown";
};

return {
overlayProps: {
onKeyDown,
Expand Down