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

[Dialog] DialogContent disables pointer events for the whole page which might conflict with 3rd party modals #2122

Closed
lesha1201 opened this issue May 1, 2023 · 20 comments

Comments

@lesha1201
Copy link
Contributor

lesha1201 commented May 1, 2023

Bug report

Current Behavior

When DialogContent is modal, it has disableOutsidePointerEvents={true} which sets pointer-events: none. And because of that modals from 3rd parties might not work properly when they're open from the dialog. For example, I need to implement a dialog for adding a payment method in Stripe. Stripe.js can open their dialog in some cases, for example, in case of 3D secure.

Desktop.2023.05.01.-.19.43.14.02.webm

Suggested solution

I think we should allow to specify disableOutsidePointerEvents for DialogContent. There are several reasons for that:

  • There is no sense for having it when you have DialogOverlay which covers the whole page.
  • To have more flexibility, when you have some special cases.

It should be easy to allow to specify that prop.

@ccxdev
Copy link

ccxdev commented May 2, 2023

I also faced the same problem when I integrated the recaptcha from google

My solution was:

onClick={() => {
  setIsDialogVisible(true)
  setTimeout(() => (document.body.style.pointerEvents = ""), 0)
}}

@akselinurmio
Copy link

We are experiencing this exact same issue too. Our app relies on multiple different external modals that can be opened while Radix UI dialog is open. Is there any reason for the pointer-events: none to be set on the body element when the Dialog is open? Isn’t the Overlay component supposed to stand on top of the content behind the dialog, and capture all clicks outside the Dialog?

@recs182
Copy link

recs182 commented May 4, 2023

I having this issue too! Also can confirm that version 1.0.2 does not have the problem, so people may consider downgrade until it is fixed.

@benoitgrelard
Copy link
Collaborator

benoitgrelard commented May 22, 2023

Hey all,

I think we should allow to specify disableOutsidePointerEvents for DialogContent. There are several reasons for that:

  • There is no sense for having it when you have DialogOverlay which covers the whole page.

Is there any reason for the pointer-events: none to be set on the body element when the Dialog is open? Isn’t the Overlay component supposed to stand on top of the content behind the dialog, and capture all clicks outside the Dialog?

The issue is you are thinking about modality only from the POV of Dialog. Although somewhat visually related sometimes, the concept of modality however has nothing to do with it. Dialog does happen to have an overlay, but there are plenty of other components that don't and still need to support modality (Popover, DropdownMenu, etc).

The easiest solution to the 3rd party component being open from one of our modal components would be to set pointer-events: auto on their root container.

We could try and find a solution from our side where you wrap the 3rd party lib with something, but I fear it will be difficult to generalise as other 3rd party might portal somewhere else, etc. So ultimately doing it yourself on a case by case (these are edge cases too) it probably the best solution.

@lesha1201
Copy link
Contributor Author

@benoitgrelard The issue with Stripe is that it doesn't allow to set a custom container or customize it. We don't have control over it.

I think allowing to set disableOutsidePointerEvents covers more use cases of Dialog and gives more flexibility. The default value will be the same.

The issue is you are thinking about modality only from the POV of Dialog. Although somewhat visually related sometimes, the concept of modality however has nothing to do with it. Dialog does happen to have an overlay, but there are plenty of other components that don't and still need to support modality (Popover, DropdownMenu, etc).

I don't get it how it's related to Popover, DropdownMenu, etc. because the proposal is to allow setting disableOutsidePointerEvents on DialogContent which isn't used in those components.

It's not a common case to see pointer-events: none on body in case of modal. It's much easier and more right, in my opinion, to handle that case from Radix side because Radix adds that behavior, not a 3rd party. Otherwise, the users of Radix will have to come up with workarounds because each case can be unique.

@benoitgrelard
Copy link
Collaborator

I don't get it how it's related to Popover, DropdownMenu, etc. because the proposal is to allow setting disableOutsidePointerEvents on DialogContent which isn't used in those components.

Because modality is a general concept which applies in more than dialogs. The stripe dialog could be opened from a Popover just as well as a Dialog. And that doesn't have an overlay.

It's not a common case to see pointer-events: none on body in case of modal.

I get that but many implementations of modality in the wild are very naïve and incomplete IMO.

The issue with Stripe is that it doesn't allow to set a custom container or customize it. We don't have control over it.

There must be a way to get to the node right? A CSS selector, some DOM traversal, etc. There's always a way really.
I understand we may need to find a way for an escape hatch but I am reluctant to do this via exposing the disableOutsidePointerEvents prop.

I will keep thinking on my side if there are other ways.

@lesha1201
Copy link
Contributor Author

Because modality is a general concept which applies in more than dialogs. The stripe dialog could be opened from a Popover just as well as a Dialog. And that doesn't have an overlay.

Thank you for clarifying.

There must be a way to get to the node right? A CSS selector, some DOM traversal, etc. There's always a way really.

In case of Stripe, they don't set id or any attribute which we can use to safely query for the container. They have name attribute on iframe which we can use for querying but the logic for attaching the container with iframe comes from a proprietary code so it's not very safe to rely on such solution:

document.querySelector('iframe[name^="__privateStripe"]').parentNode

image

I will keep thinking on my side if there are other ways.

👍

@benoitgrelard
Copy link
Collaborator

Wouldn't targeting the iframe itself be enough?

@lesha1201
Copy link
Contributor Author

@benoitgrelard Stripe creates several iframes already and also a new one for 3D secure.

I'm going to just changing pointerEvents on body when that iframe opens:

document.body.style.pointerEvents = ""

I think that's the easiest and quete safe solution for now.

@isaackoz
Copy link

isaackoz commented Aug 6, 2023

Had this same issue with Stripe modals today. @lesha1201's solution above was the simplest.

useEffect(() => {
		if (modalOpen) {
			// Pushing the change to the end of the call stack
			const timer = setTimeout(() => {
				document.body.style.pointerEvents = "";
			}, 0);

			return () => clearTimeout(timer);
		} else {
			document.body.style.pointerEvents = "auto";
		}
	}, [modalOpen]);

This assumes you set open={modalOpen} and onOpenChange={()=>setModalOpen(!modalOpen)} or onOpenChange={()=>{}} if you handle closing it yourself. I would recommend the latter so users don't accidentally close the modal when inputting their Stripe info.

@softmarshmallow
Copy link

softmarshmallow commented Mar 13, 2024

This happens when the Dialog is opened under Popover / Dropdown. this issue is still present, couldn't find a clean solution in 2024.

After closing the dialog, it will block the entire ui (when used under popover / dropdown)

Can you kindly look into this case? @benoitgrelard should I open a new issue? seems related and unresolved.

Screen.Recording.2024-03-13.at.9.43.17.AM.mov

@zeroliu
Copy link

zeroliu commented Mar 24, 2024

The easiest solution to the 3rd party component being open from one of our modal components would be to set pointer-events: auto on their root container.

I am trying to use react-select with Radix dialog and I had to make the select menu rendered on a portal in my use case. Because of disableOutsidePointerEvents, the menu is not interactable. I added pointer-events: auto to the menuPortal and it solves the problem. Thanks for sharing the solution @benoitgrelard !

@ansh-les
Copy link

ansh-les commented May 22, 2024

The easiest solution to the 3rd party component being open from one of our modal components would be to set pointer-events: auto on their root container.

I am trying to use react-select with Radix dialog and I had to make the select menu rendered on a portal in my use case. Because of disableOutsidePointerEvents, the menu is not interactable. I added pointer-events: auto to the menuPortal and it solves the problem. Thanks for sharing the solution @benoitgrelard !

I'm facing the same issue. Where exactly did you portal the menu? I'm able to select the items in the menu but I'm unable to scroll the menu using the mouse scroll wheel.

@jamesthesken
Copy link

Had this same issue with Stripe modals today. @lesha1201's solution above was the simplest.

useEffect(() => {
		if (modalOpen) {
			// Pushing the change to the end of the call stack
			const timer = setTimeout(() => {
				document.body.style.pointerEvents = "";
			}, 0);

			return () => clearTimeout(timer);
		} else {
			document.body.style.pointerEvents = "auto";
		}
	}, [modalOpen]);

This assumes you set open={modalOpen} and onOpenChange={()=>setModalOpen(!modalOpen)} or onOpenChange={()=>{}} if you handle closing it yourself. I would recommend the latter so users don't accidentally close the modal when inputting their Stripe info.

Thanks! This was a quick fix for us when using Yet Another React Lightbox

@faizananwerali
Copy link

Had this same issue with Stripe modals today. @lesha1201's solution above was the simplest.

useEffect(() => {
		if (modalOpen) {
			// Pushing the change to the end of the call stack
			const timer = setTimeout(() => {
				document.body.style.pointerEvents = "";
			}, 0);

			return () => clearTimeout(timer);
		} else {
			document.body.style.pointerEvents = "auto";
		}
	}, [modalOpen]);

This assumes you set open={modalOpen} and onOpenChange={()=>setModalOpen(!modalOpen)} or onOpenChange={()=>{}} if you handle closing it yourself. I would recommend the latter so users don't accidentally close the modal when inputting their Stripe info.

Thank you so much. It fixes my issue using shadcdn.

const [modalOpen, setModelOpen] = useState(false);

useEffect(() => {
    if (modalOpen) {
      // Pushing the change to the end of the call stack
      const timer = setTimeout(() => {
        document.body.style.pointerEvents = '';
      }, 0);

      return () => clearTimeout(timer);
    } else {
      document.body.style.pointerEvents = 'auto';
    }
  }, [modalOpen]);
<Dialog modal={true} onOpenChange={(open) => setModelOpen(open)}>

@mochetts
Copy link

mochetts commented Jul 9, 2024

Due to this issue, it is borderline impossible to integrate google recaptcha within a modal dialog.

@sahanatvessel
Copy link

Why was this issue closed?

@Notmeomg
Copy link

Notmeomg commented Aug 18, 2024

This issue was resolved for me by aligning package versions. I did this after reading #1088.

@levanton
Copy link

I have fixed the issue by using the modal prop in the Dialog component.

interface DialogProps { children?: React.ReactNode; open?: boolean; defaultOpen?: boolean; onOpenChange?(open: boolean): void; modal?: boolean; }

By setting modal to false, everything works fine.

@jpereiramp
Copy link

I was having issues with setting modal={false} on shadcn-ui's Sheet and Dialog components, both of which were instantly closing upon being opened. While I did find a hacky way to overcome this, I had lost some styling on these components when disabling the modal option.

So another solution that worked for my case was to include the following on my CSS:

@layer base {
    * {
        @apply pointer-events-auto
    }
}

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

No branches or pull requests