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: prevent parent drawers from closing when canceling nested drawers #1748

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

unrenamed
Copy link
Contributor

What does this PR do?

Fixes #1624 — an issue where clicking the "Cancel" button in a nested drawer unintentionally closed its parent drawer.

⚠️ Key Dub.co & Vaul Implementation Details

  1. React Portals Usage: Both Radix dialogs and Vaul drawers are rendered using React portals, allowing them to appear outside their parent component in the DOM hierarchy.
  2. Pseudo-Nested Drawers: While Vaul drawers may appear visually nested, they are not implemented using the <Drawer.NestedRoot /> API. Instead, they are treated as siblings in the DOM structure. Refer to the Vaul documentation on nested drawers for details.
  3. onPointerDownOutside Behavior: Radix dialogs and Vaul drawers trigger the onPointerDownOutside callback when a click occurs outside these components. This callback can be leveraged to prevent unintended dismissals.

⚠️ Reproduction

Based on my analysis, the issue occurs under the following conditions:

  1. Vaul Drawers Only: The issue is specific to Vaul drawers; Radix dialogs do not exhibit this behavior.
  2. First Nested Drawer: The problem arises only when opening the first nested drawer. Deeper (2+ level) nested drawers are unaffected and require no fixes.

The exact reason for this behavior remains unclear. It may vary across different machines or operating systems, likely depending on Vaul's internal implementation details.

Probable root cause...

lies in event bubbling and the fact that both "parent" and "child" drawers are rendered in a portal, making them DOM siblings instead of a parent-child relationship. When clicking a button in the child drawer triggers the event, it bubbles up to the parent drawer's onClick handler (if one exists) or a handler managing the parent drawer's state.

Now, if we take into account that additional detail about the onPointerDownOutside callback, we might see that the core problem arises from how the browser and event listeners interpret removing elements from the DOM during event propagation.

Why the Parent Drawer Detects an "Outside Click"

When you click the button in the child drawer, the following sequence occurs:

  1. The pointerdown event is dispatched and captured in the DOM.
  2. The event starts bubbling up from the target (button) to the ancestors.
  3. The child drawer is removed synchronously during event propagation, as it is triggered by the onclick callback. While React queues state updates internally (i.e. removal is in fact asynchronous), the process is executed extremely quickly.
  4. The removal of the child drawer's DOM causes the button (event target) to no longer exist in the DOM when the onPointerDownOutside check is executed.

As a result, the parent drawer interprets the pointerdown event as happening outside its boundaries, even though the event originated from inside the child drawer.

Key Takeaway

In a nutshell,

  1. When you click the button, it triggers the onclick and pointerdown events.
  2. onClick removes the child drawer synchronously during the event handling phase.
  3. By the time the onPointerDownOutside callback checks for "outside clicks," the child drawer no longer exists in the DOM, and the event target (button) appears "outside" the parent drawer.

This happens because:

  1. The DOM is modified mid-event propagation.
  2. Event propagation and DOM updates are asynchronous: React’s DOM updates (e.g., removing the child drawer) can cause the element's parentage to "shift" during the event lifecycle, leading to unexpected behavior.

Possible Fixes

Prevent Synchronous Drawer Removal

Defer the removal of the child drawer slightly, allowing the onPointerDownOutside check to complete before the DOM changes:

const closeChildDrawer = () => {
  setTimeout(() => setChildDrawerVisible(false), 0);
};

This ensures the parent drawer still sees the event as occurring "inside" during the check.

Stop Event Propagation

Use e.stopPropagation() in the child drawer's button click handler to prevent the event from being detected by the parent's onPointerDownOutside logic:

<button
  onClick={(e) => {
    e.stopPropagation();
    closeChildDrawer();
  }}
>
  Close Child
</button>

Customize onPointerDownOutside

We're using libraries (Radix UI, Vaul) that provide the onPointerDownOutside callback, hence we can customize the behavior to avoid treating clicks inside the child drawer as "outside" clicks.

For example, we can add a condition to the callback:

onPointerDownOutside={(e) => {
  // Prevent dismissal when clicking inside a toast
  if (
    e.target instanceof Element &&
    (e.target.closest("[data-sonner-toast]") ||
      e.target.closest("[data-vaul-drawer]"))
  ) {
    e.preventDefault();
  }
}}

In Summary

The fixes (like e.stopPropagation() or setTimeout()) address symptoms, but the root cause is how the bubbling events interact with our drawer closure logic. Ideally, the best solution would be to leverage Vaul's <Drawer.NestedRoot /> API. However, this would require a significant refactoring and restructuring of the modal system. Such a task is time-intensive and best handled by the core team to ensure it does not disrupt the app's core functionality.

Another potential solution involves preventing unintended event bubbling at its source. However, since Vaul is an external library, this approach is not feasible for us and I'm not quite sure this is something Vaul should cover.

As a result, this PR aims to highlight the issue while implementing the most straightforward, controlled, and efficient fix: stopping event propagation in child modals' "Cancel" button events. This approach resolves the bug promptly without overhauling the existing structure.

Testing

This fix was tested by:

  1. Opening a parent drawer with a nested drawer.
  2. Cancelling the nested drawer and verifying the parent drawer remains open.
  3. Confirming other functionalities of both parent and nested drawers remain unaffected.

Ensure the "Cancel" button in nested drawers stops event propagation to avoid unintentionally closing parent drawers. Addresses unintended event bubbling within Vaul's drawer components. Has the same effect on Radix dialog components, although does not affect the UI behavior.
Copy link

vercel bot commented Nov 24, 2024

@unrenamed is attempting to deploy a commit to the Dub Team on Vercel.

A member of the Team first needs to authorize it.

@unrenamed
Copy link
Contributor Author

P.S. some uncertainties remain, such as why the issue doesn't occur with deeper nested drawers or whether the pointer-events: none styling on parent drawers contributes to the bug. However, these points don't change the core issue, and the proposed fixes effectively address the problem. The primary goal of this PR is to raise awareness of the bug and share my findings, which may aid the core team's future analysis if this PR is deemed a workaround rather than a permanent fix.

@steven-tey feel free to review this and ping me if you got any questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI Issue on UTM Drawer when user clicks cancel on Mobile
1 participant