fix(modal/drawer): prevent modal/drawer from closing on press instead…#5623
Closed
anshumandev2025 wants to merge 4 commits into
Closed
fix(modal/drawer): prevent modal/drawer from closing on press instead…#5623anshumandev2025 wants to merge 4 commits into
anshumandev2025 wants to merge 4 commits into
Conversation
… of press release
🦋 Changeset detectedLatest commit: fe85fd1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@anshumandev2025 is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
WalkthroughRemoved immediate closure on interaction start for overlays: onHide() is no longer invoked during onInteractOutsideStart for modal/drawer elements; overlays still call onHide() on onInteractOutside (release). No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Pointer/Touch
participant Overlay as Overlay component
participant Hook as use-aria-overlay handlers
User->>Overlay: Press down outside
Overlay->>Hook: onInteractOutsideStart(event)
Note right of Hook #d3f9d8: onHide() skipped for modal/drawer on start
Hook-->>Overlay: (may stopPropagation/preventDefault)
User-->>Overlay: Release outside
Overlay->>Hook: onInteractOutside(event)
Note right of Hook #cfe8ff: onHide() invoked on release (close)
Hook->>Overlay: onHide()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
wingkwong
requested changes
Aug 23, 2025
Member
wingkwong
left a comment
wingkwong
left a comment
There was a problem hiding this comment.
- please add changeset
- please add test case
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-aria-overlay
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-form-reset
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/use-viewport-size
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
Author
|
Thanks for you suggestion @wingkwong i added the changeset and testcases. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.changeset/shy-garlics-peel.md (1)
5-7: Tighten phrasing and clarify interaction type.Minor copy edit to be more explicit and formal; also prefer “press + release” or “pointerdown/pointerup” over “press start”.
Apply this diff:
-fix: prevent premature closing of modal/drawer on press start +fix: prevent premature closing of modal/drawer on pointer down @@ -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. +Removes calling `onHide()` in `onInteractOutsideStart`, resolving an issue where modals and drawers closed on initial outside press instead of after the full interaction (press + release).packages/components/modal/__tests__/modal.test.tsx (1)
240-241: Optionally guard for async scheduling with waitFor.If the close handler is scheduled (microtask/raf), wrap the final assertion in
waitFor.Within this test:
- expect(onClose).toHaveBeenCalledTimes(1); + await waitFor(() => expect(onClose).toHaveBeenCalledTimes(1));And add the import at the top of the file:
import {render, fireEvent, waitFor} from "@testing-library/react";packages/components/drawer/__tests__/drawer.test.tsx (1)
135-136: Optionally use waitFor for stability.Mirror the optional async guard from the Modal test.
- expect(onClose).toHaveBeenCalledTimes(1); + await waitFor(() => expect(onClose).toHaveBeenCalledTimes(1));Top-level import:
import {render, fireEvent, waitFor} from "@testing-library/react";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/shy-garlics-peel.md(1 hunks)packages/components/drawer/__tests__/drawer.test.tsx(1 hunks)packages/components/modal/__tests__/modal.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/modal/__tests__/modal.test.tsx (1)
packages/components/modal/src/index.ts (5)
Modal(24-24)ModalContent(24-24)ModalHeader(24-24)ModalBody(24-24)ModalFooter(24-24)
🪛 LanguageTool
.changeset/shy-garlics-peel.md
[style] ~7-~7: Consider using a different verb for a more formal wording.
Context: ...ide()fromonInteractOutsideStart` to fix an issue where modals and drawers close...
(FIX_RESOLVE)
🔇 Additional comments (1)
.changeset/shy-garlics-peel.md (1)
1-7: No action needed: Changesets will patch all downstream packagesThe
.changeset/config.jsonis configured with"updateInternalDependencies": "patch"which ensures that when
@heroui/use-aria-overlayis bumped, all internal dependents receive a patch version bump. In this repo, direct dependents include:
@heroui/popover@heroui/tooltip@heroui/use-aria-modal-overlayBecause changesets cascades these updates,
@heroui/modal(which depends onuse-aria-modal-overlay) and then@heroui/drawer(which depends onmodal) will also be bumped automatically. No manual version bumps are required.
Comment on lines
+114
to
+136
| 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); | ||
| }); |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+219
to
+241
| 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); | ||
| }); |
Contributor
There was a problem hiding this comment.
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5616
📝 Description
This PR fixes a bug where the Modal/Drawer was closing on press instead of on press release, resulting in an unintended and abrupt user experience.
Root Cause
The issue was caused by the onHide function being triggered inside the onInteractOutsideStart event within useAriaOverlay. This event fires as soon as the user presses outside the modal, rather than waiting for the full press interaction (press + release), which led to the modal closing prematurely.
Solution
To align with expected behaviour—where a modal or drawer should only close after the complete interaction (i.e. press and release)—the onHide call has been removed from onInteractOutsideStart.
Summary by CodeRabbit
Bug Fixes
Tests
Chores