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 incorrect closing while interacting with third party libraries in Dialog component #1268

Merged
merged 3 commits into from
Mar 24, 2022

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Mar 24, 2022

This PR fixes an issue where the outside click behaviour was triggered if you clicked on 3rd party provided components that were portalled to the body.
From a DOM perspective this bug made sense because the 3rd party components are rendered outside of the current Dialog and therefore it closes.

This PR should fix that behaviour and it should be possible to interact with 3rd party libraries now.

You can play with it here:

Fixes: #432

@vercel
Copy link

vercel bot commented Mar 24, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

headlessui-react – ./packages/playground-react

🔍 Inspect: https://vercel.com/tailwindlabs/headlessui-react/5SpRor6rFyi1pkfAhHC3iJdG1SJj
✅ Preview: https://headlessui-react-git-fix-third-party-interactions-tailwindlabs.vercel.app

headlessui-vue – ./packages/playground-vue

🔍 Inspect: https://vercel.com/tailwindlabs/headlessui-vue/GAa5Ety2SfXoWxe14ghv8gWJ4bM8
✅ Preview: https://headlessui-vue-git-fix-third-party-interactions-tailwindlabs.vercel.app

<TabSentinel />
</div>
</Dialog>
<ThirdPartyLibrary />

Choose a reason for hiding this comment

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

My issue in #432 was with third-party components inside a Dialog, it seems like this test doesn't do that? Maybe I'm misunderstanding. I would have expected something like:

          <div>
            <span>Main app</span>
            <Dialog :open="isOpen" @close="setIsOpen">
              <div>
                <ThirdPartyLibrary />
                <TabSentinel />
              </div>
            </Dialog>
          </div>

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yep I should probably add a test for that case as well. But the way it is implemented it doesn't really matter where the 3rd party gets initiated from. I'll explain the process in the other question you asked 👍

() => {
// Third party roots
let rootContainers = Array.from(
ownerDocument.value?.querySelectorAll('body > *') ?? []

Choose a reason for hiding this comment

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

I don't completely understand what's happening here but I'm curious whether this fix works with elements that are not direct children of the body tag? What about clicks on an element nested inside a few others?

Copy link
Member Author

@RobinMalfait RobinMalfait Mar 24, 2022

Choose a reason for hiding this comment

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

Alright so the idea with this code is to allow for 3rd party plugins.

We don't allow to interact with elements behind the Dialog component for accessibility reasons and what not. We still don't allow that, but there are a few exceptions that technically violate some of the rules but you can think of it as progressive enhancement.

We already allowed our Portal component to be used inside the Dialog (and nested Dialogs). The HTML looks something like this:

<body>
  <div id="app"><!-- Your main application --></div>
  <div id="headlessui-portal-root">
    <div><!-- The main Dialog --></div>
    <div><!-- ... other Portal components --></div>
  </div>
</body>

What you will notice is that the other portal components live outside the main Dialog, so technically we should close the Dialog if you click on any of those items because they are "outside" the Dialog component.

The reason this is already possible with our provided Portal is because we control that component and we can have a reference to the elements it renders in the DOM.

The issue this PR fixes is with 3rd party plugins. Often 3rd party plugins will render the popup elements in a portal as well. The problem is that we don't know when this happens and we also can't get a DOM reference easily to those elements. We also can't ask every library on planet earth to expose some of the information we need.

So this fix is definitely not perfect, but I think it will solve a lot of the issues people experience today.

How does it work?

Let's imagine you have this structure again:

<body>
  <div id="app">
    <div>
      <!--
        This is an example button in the App that is **not** inside the Dialog.
        Trying to click this button will close the Dialog because this is
        "outside" of the Dialog which is not allowed.
      -->
      <button>in app</button>
    </div>
  </div>
  <div id="headlessui-portal-root">
    <div>
      <!-- This is a button in the Dialog, interacting with this is allowed. -->
      <button>in dialog</button>
    </div>
  </div>
  <div id="third-party-library-portal">
    <!--
      Interacting with this button is technically not allowed since it lives
      outside of the Dialog. However we make an exception that you _can_
      interact with this one because it lives in another "parent" than the main
      application. This means that we are currently making an assumption that
      interacteable elements that live in a parent outside of the main app are allowed.

      This trade-off is necessary since we don't know when 3rd party libraries
      will render certain elements in the DOM and we don't get a stable
      reference to those elements.
    -->
    <button>Deeply nested button inside the 3rd party library</button>
  </div>
</body>

How it works is that we collect all the direct "root" containers, that's what the body > * is for. In this case we will get 3 elements:

  • <div id="app">
  • <div id="headlessui-portal-root">
  • <div id="third-party-library-portal">

  • Then we filter out the roots we know about, in this case we don't allow interactions in the <div id="app"> because that would violate our initial rule of "don't interact with elements outside of the Dialog".
  • Next, we still run the outside click logic, this function gives us a target which is the target element that we are clicking.
  • Last but not least we check if one of those root containers contains the target. The contains function checks whether one element is contained within another (also does check deeply nested).
  • This means that:
    • If the target is contained inside the <div id="app"> then we should run the outside click callback which in this case closes the Dialog
    • But if the target is contained inside the <div id="third-party-library-portal"> then that's fine because we assume that this is the 3rd party library.

Does that answer your question?

Choose a reason for hiding this comment

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

Wow yeah that does answer my question, thanks! I really appreciate the detailed explanation. At one point I had thought it would be easier to just listen for clicks directly on the backdrop, but now I'm realizing there doesn't even necessarily have to be a backdrop... not to mention all the other possibilities. This is way more complicated than I thought and your solution looks great. Thanks for breaking it down 👍🏻

@agcty
Copy link

agcty commented Mar 29, 2022

@RobinMalfait this PR is great, however, I have some questions revolving around two use-cases.

Notifications
In our app, we have a notification component, sometimes a notification gets triggered by an action inside the dialog. We are looking for a way to dismiss that notification, without also closing the Dialog. For context, the library we use is react-hot-toast and it allows you to put your Toaster component pretty much anywhere inside the app, it doesn't however portal the Toasts to a sibling of the app root. Just to clarify, would this mean we just have to portal the toasts/notifications ourselves and it should work?

Navbars
We have a navigation bar at the bottom of the screen with X buttons, each button triggers a Dialog, users should be able to click another button inside that navbar, while the Dialog is open. In this case, the Dialog should close, and another should open. We tried giving the navbar a higher z-index than the dialog and even setting the Dialogs height as screen - navbar height but the issue is, that clicking the navbar counts as an outside click and therefore closes the Dialog.

There is basically no way to stop this behavior, not even with this PR from what I can tell. I think this could be solved with a flag like "closesOnOutsideClick" as described in #911. What do you think about this?

We'd really like to avoid having to roll our own solution because of the Dialog's handling of focus states, scroll behavior, and portalling.

@RobinMalfait
Copy link
Member Author

@agcty thanks for your questions:

Notifications

... and it allows you to put your Toaster component pretty much anywhere inside the app, it doesn't however portal the Toasts to a sibling of the app root
How do you guarantee that the notification/toast is always on top? Even with z-index: 999999; you can't guarantee that. I would recommend to render the toast itself in a portal. If you don't want to write a component for this, we expose a Portal component (undocumented). Usage looks like this:

import { Portal } from '@headlessui/react'

function SomwhereDeeplyNested() {
  return <Portal>This will get portalled</Portal>
}

Navbars
Would something like this work for you?

@agcty
Copy link

agcty commented Mar 29, 2022

@RobinMalfait thanks for the explanation, was successfully able to use the Portal component to be able to click on notifications!

However, in more complex scenarios, usage of dialog + portal seems to break

Consider this sandbox with 3 examples:
https://codesandbox.io/s/vigorous-dewdney-o3lmcl?file=/src/App.jsx

I added a mute/play music button to the footer that should be clickable and not close the dialog, and a notification that when clicked should also not close the dialog.

I wasn't able to come up with a working solution to this problem, example2 especially even further confused me.

@Powell-v2
Copy link

Powell-v2 commented May 23, 2022

Hi there, I believe there's a similar issue with a Popover component. My use case involves a third part date picker which portals the calendar over to the body, and any click on its contents causes the popover to close automatically. I'm not quite sure how to get around this problem - any help is highly appreciated!


UPD: Nevermind, I found a solution! The component is the one provided by Mantine (https://mantine.dev/dates/date-picker) and it portals the calendar over to the body by default. All one has to do is to simply set a prop called withinPortal to false 😄

Thank you @RobinMalfait for a detailed explanation of the problem! 👍

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

Successfully merging this pull request may close these issues.

[Bug]: Interacting with third-party components that use portals inside a Dialog closes the Dialog
4 participants