[bug]: closing dialog by click the dialog overlay will trigger click event by the parent

bao-io commented 2 weeks ago

Describe the bug

export default function Page() {
  const [open, setOpen] = useState(false);
  return (
    <div
      onClick={(e) => {
        console.log("trigger");
      }}
    >
      <Dialog open={open} onOpenChange={setOpen}>
        <DialogTrigger asChild>
          <Button
            size="sm"
            variant="outline"
            onClick={(e) => e.stopPropagation()}
          >
            test
          </Button>
        </DialogTrigger>
        <DialogContent>
          <DialogHeader>121321</DialogHeader>
        </DialogContent>
      </Dialog>
    </div>
  );
}

I have already blocked the bubble of the event with the button. Why does it trigger the parent click event when closing the pop-up window

Affected component/components

Dialog

How to reproduce

copy my example code above and open the develop consloe
click the button to open the dialog, then close it
you can see the log in the console

Codesandbox/StackBlitz link

No response

Logs

No response

System Info

macos, google chrome

Before submitting

[X] I've made research efforts and searched the documentation
[X] I've searched for existing issues

OmarAljoundi commented 2 weeks ago

Hi bao-io

I found a solution for your issue, you almost solve it by passing the stopPropagation to the onClick, but the place is not correct. You need to pass the stopPropagation to the DialogOverlay that lives in your components/ui/dialog.tsx

Here is what you need to do:

In your Dialog Component adjust it to stop Propagation for DialogOverlay element:

const DialogContent = React.forwardRef<
  React.ElementRef<typeof DialogPrimitive.Content>,
  React.ComponentPropsWithoutRef<typeof DialogPrimitive.Content>
>(({ className, children, ...props }, ref) => (
  <DialogPortal>
    <DialogOverlay onClick={(event) => event.stopPropagation()} /> // ADD ONCLICK HERE
    <DialogPrimitive.Content
      ref={ref}
      className={cn(
        'fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg',
        className
      )}
      {...props}
    >
      {children}
      <DialogPrimitive.Close className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground">
        <X className="h-4 w-4" />
        <span className="sr-only">Close</span>
      </DialogPrimitive.Close>
    </DialogPrimitive.Content>
  </DialogPortal>
))
DialogContent.displayName = DialogPrimitive.Content.displayName

I hope it works for you!

bao-io commented 2 weeks ago

Hi bao-io

I found a solution for your issue, you almost solve it by passing the stopPropagation to the onClick, but the place is not correct.

You need to pass the stopPropagation to the DialogOverlay that lives in your components/ui/dialog.tsx

Here is what you need to do:

In your Dialog Component adjust it to stop Propagation for DialogOverlay element:


const DialogContent = React.forwardRef<

  React.ElementRef<typeof DialogPrimitive.Content>,

  React.ComponentPropsWithoutRef<typeof DialogPrimitive.Content>

>(({ className, children, ...props }, ref) => (

  <DialogPortal>

    <DialogOverlay onClick={(event) => event.stopPropagation()} /> // ADD ONCLICK HERE

    <DialogPrimitive.Content

      ref={ref}

      className={cn(

        'fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg',

        className

      )}

      {...props}

    >

      {children}

      <DialogPrimitive.Close className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground">

        <X className="h-4 w-4" />

        <span className="sr-only">Close</span>

      </DialogPrimitive.Close>

    </DialogPrimitive.Content>

  </DialogPortal>

))

DialogContent.displayName = DialogPrimitive.Content.displayName

I hope it works for you!

Thank you for your reply, I have tried your solution and it can indeed solve my problem, but you can see that on the mobile end, its mask cannot be closed by clicking

OmarAljoundi commented 2 weeks ago

Hi bao-io I found a solution for your issue, you almost solve it by passing the stopPropagation to the onClick, but the place is not correct. You need to pass the stopPropagation to the DialogOverlay that lives in your components/ui/dialog.tsx Here is what you need to do:

In your Dialog Component adjust it to stop Propagation for DialogOverlay element:


const DialogContent = React.forwardRef<

  React.ElementRef<typeof DialogPrimitive.Content>,

  React.ComponentPropsWithoutRef<typeof DialogPrimitive.Content>

>(({ className, children, ...props }, ref) => (

  <DialogPortal>

    <DialogOverlay onClick={(event) => event.stopPropagation()} /> // ADD ONCLICK HERE

    <DialogPrimitive.Content

      ref={ref}

      className={cn(

        'fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg',

        className

      )}

      {...props}

    >

      {children}

      <DialogPrimitive.Close className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground">

        <X className="h-4 w-4" />

        <span className="sr-only">Close</span>

      </DialogPrimitive.Close>

    </DialogPrimitive.Content>

  </DialogPortal>

))

DialogContent.displayName = DialogPrimitive.Content.displayName

I hope it works for you!

Thank you for your reply, I have tried your solution and it can indeed solve my problem, but you can see that on the mobile end, its mask cannot be closed by clicking

Indeed, the issue due to the additional events the mobile might use such as the "Touch Events".

I would suggest another approach which is to pass the "Close Function", e.g.

const DialogContent = React.forwardRef<
  React.ElementRef<typeof DialogPrimitive.Content>,
  React.ComponentPropsWithoutRef<typeof DialogPrimitive.Content> & {
    handleClose?: () => void
  }
>(({ className, children, handleClose, ...props }, ref) => (
  <DialogPortal>
    <DialogOverlay
      onClick={(event) => {
        if (handleClose) {
          event.stopPropagation()
          handleClose()
        }
      }}
    />
    <DialogPrimitive.Content
      ref={ref}
      className={cn(
        'fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg',
        className
      )}
      {...props}
    >
      {children}
      <DialogPrimitive.Close className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground">
        <X className="h-4 w-4" />
        <span className="sr-only">Close</span>
      </DialogPrimitive.Close>
    </DialogPrimitive.Content>
  </DialogPortal>
))

As you see, I've passed the close function to handle the close of the dialog if it was provided, and completely ignore the stopPropagation() in case it wasn't to keep the behavior as it is!

 <div
      onClick={(e) => {
        console.log(e.type, 'trigger')
      }}
      className="h-52 w-40 bg-gray-900"
    >
      <Dialog open={open} onOpenChange={setOpen} modal={false}>
        <DialogTrigger asChild>
          <Button size="sm" variant="outline">
            Click me
          </Button>
        </DialogTrigger>
        <DialogContent handleClose={() => setOpen(false)}>
          <DialogHeader>121321</DialogHeader>
        </DialogContent>
      </Dialog>
    </div>

Please note that you have to also handle the case when the user click the "X" button to close the dialog or when the user click inside the dialog as well.

Cheers!

bao-io commented 2 weeks ago

yeah, your idea is so wonderfull, but i think this

Hi bao-io I found a solution for your issue, you almost solve it by passing the stopPropagation to the onClick, but the place is not correct. You need to pass the stopPropagation to the DialogOverlay that lives in your components/ui/dialog.tsx Here is what you need to do:

In your Dialog Component adjust it to stop Propagation for DialogOverlay element:


const DialogContent = React.forwardRef<

  React.ElementRef<typeof DialogPrimitive.Content>,

  React.ComponentPropsWithoutRef<typeof DialogPrimitive.Content>

>(({ className, children, ...props }, ref) => (

  <DialogPortal>

    <DialogOverlay onClick={(event) => event.stopPropagation()} /> // ADD ONCLICK HERE

    <DialogPrimitive.Content

      ref={ref}

      className={cn(

        'fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg',

        className

      )}

      {...props}

    >

      {children}

      <DialogPrimitive.Close className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground">

        <X className="h-4 w-4" />

        <span className="sr-only">Close</span>

      </DialogPrimitive.Close>

    </DialogPrimitive.Content>

  </DialogPortal>

))

DialogContent.displayName = DialogPrimitive.Content.displayName

I hope it works for you!

Thank you for your reply, I have tried your solution and it can indeed solve my problem, but you can see that on the mobile end, its mask cannot be closed by clicking

Indeed, the issue due to the additional events the mobile might use such as the "Touch Events".

I would suggest another approach which is to pass the "Close Function", e.g.

const DialogContent = React.forwardRef<
  React.ElementRef<typeof DialogPrimitive.Content>,
  React.ComponentPropsWithoutRef<typeof DialogPrimitive.Content> & {
    handleClose?: () => void
  }
>(({ className, children, handleClose, ...props }, ref) => (
  <DialogPortal>
    <DialogOverlay
      onClick={(event) => {
        if (handleClose) {
          event.stopPropagation()
          handleClose()
        }
      }}
    />
    <DialogPrimitive.Content
      ref={ref}
      className={cn(
        'fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg',
        className
      )}
      {...props}
    >
      {children}
      <DialogPrimitive.Close className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground">
        <X className="h-4 w-4" />
        <span className="sr-only">Close</span>
      </DialogPrimitive.Close>
    </DialogPrimitive.Content>
  </DialogPortal>
))

As you see, I've passed the close function to handle the close of the dialog if it was provided, and completely ignore the stopPropagation() in case it wasn't to keep the behavior as it is!

 <div
      onClick={(e) => {
        console.log(e.type, 'trigger')
      }}
      className="h-52 w-40 bg-gray-900"
    >
      <Dialog open={open} onOpenChange={setOpen} modal={false}>
        <DialogTrigger asChild>
          <Button size="sm" variant="outline">
            Click me
          </Button>
        </DialogTrigger>
        <DialogContent handleClose={() => setOpen(false)}>
          <DialogHeader>121321</DialogHeader>
        </DialogContent>
      </Dialog>
    </div>

Please note that you have to also handle the case when the user click the "X" button to close the dialog or when the user click inside the dialog as well.

Cheers!

Thank you again for your reply，your idea is so wonderfull, but I don't think this idea is a long-term solution. I know that masks are closed through event bubbles, but under normal circumstances, this should not happen. This should be a bug in radixui

shadcn-ui / ui