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 outside click in nested portalled Popover components #3362

Merged
merged 9 commits into from
Jul 5, 2024

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Jul 4, 2024

This PR fixes an issue where nested Popover components that are using the anchor prop (and thus are portalled) are not correctly closed when clicking outside.

The issue stems from incorrect DOM node references to know whether we are allowed to click on a certain DOM element or not.

As a general recap, when an outside click happens, we need to react to it and typically use the useOutsideClick hook for this.

We also require the context of "allowed root containers", this means that clicking on a 3rd party toast when a dialog is open, that we allow this even though we are technically clicking outside of the dialog. This is simply because we don't have control over these elements.

We also need a reference to know what the "main tree" container is, because this is the container where your application lives and we know that we are not allowed to click on anything in this container. The complex part is getting a reference to this element.

<html>
  <head>
    <title></title>
  </head>
  <body>
    <div id="app"> <-- main root container -->
      <div></div>
      <div>
        <Popover></Popover> <!-- Current component -->
      </div>
      <div></div>
    </div>

    <!-- Allowed container #1 -->
    <third-party-toast-container></third-party-toast-container>
  </body>

  <!-- Allowed container #2 -->
  <grammarly-extension></grammarly-extension>
</html>

Some examples:

  • In case of a Dialog, the Dialog is rendered in a Portal which means that a DOM ref to the Dialog or anything inside will not point to the "main tree" node.
  • In case of a Popover we can use the PopoverButton as an element that lives in the main tree. However, if you work with nested Popover components, and the outer PopoverPanel uses the anchor or portal props, then the inner PortalButton will not be in the main tree either because it will live in the portalled PopoverPanel of the parent.

This is where the MainTreeProvider comes in handy. This component will use the passed in node as the main tree node reference and pass this via the context through the React tree. This means that a nested Popover will still use a reference from the parent Popover.

In case of the Dialog, we wrap the Dialog itself with this provider which means that the provider will be in the main tree and can be used inside the portalled Dialog.

Another part of the MainTreeProvider is that if no node exists in the parent (reading from context), and no node is provided via props, then we will briefly render a hidden element, find the root node of the main tree (aka, the parent element that is a direct child of the body, body > *). Once we found it, we remove the hidden element again. This way we don't keep unnecessary DOM nodes around.

This also fixes another issue #3319, because we will now briefly render the hidden element to determine the main tree node.

Fixes: #3332
Fixes: #3319

This improves the HTML DOM tree if this happens to be used in let's say
a `p` tag where `div` elements are not allowed. The `Hidden` element is
hidden so it doesn't really matter what the underlying element is.

Fixes: #3319
Copy link

vercel bot commented Jul 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 4, 2024 11:25pm
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 4, 2024 11:25pm

As a general recap, when an outside click happens, we need to react to
it and typically use the `useOutsideClick` hook.

We also require the context of "allowed root containers", this means
that clicking on a 3rd party toast when a dialog is open, that we allow
this even though we are technically clicking outside of the dialog. This
is simply because we don't have control over these elements.

We also need a reference to know what the "main tree" container is,
because this is the container where your application lives and we _know_
that we are not allowed to click on anything in this container. The
complex part is getting a reference to this element.

```html
<html>
<head>
   <title></title>
</head>
<body>
   <div id="app"> <-- main root container -->
      <div></div>
      <div>
         <Popover></Popover> <!-- Current component -->
      </div>
      <div></div>
   </div>

   <!-- Allowed container #1 -->
   <3rd-party-toast-container></3rd-party-toast-container>
</body>

<!-- Allowed container #2 -->
<grammarly-extension></grammarly-extension>
</html>
```

Some examples:

- In case of a `Dialog`, the `Dialog` is rendered in a `Portal` which
  means that a DOM ref to the `Dialog` or anything inside will not point
  to the "main tree" node.

- In case of a `Popover` we can use the `PopoverButton` as an element
  that lives in the main tree. However, if you work with nested
  `Popover` components, and the outer `PopoverPanel` uses the `anchor`
  or `portal` props, then the inner `PortalButton` will not be in the
  main tree either because it will live in the portalled `PopoverPanel`
  of the parent.

This is where the `MainTreeProvider` comes in handy. This component will
use the passed in `node` as the main tree node reference and pass this
via the context through the React tree. This means that a nested
`Popover` will still use a reference from the parent `Popover`.

In case of the `Dialog`, we wrap the `Dialog` itself with this provider
which means that the provider will be in the main tree and can be used
inside the portalled `Dialog`.

Another part of the `MainTreeProvider` is that if no node exists in the
parent (reading from context), and no node is provided via props, then
we will briefly render a hidden element, find the root node of the main
tree (aka, the parent element that is a direct child of the body, `body
> *`). Once we found it, we remove the hidden element again. This way we
don't keep unnecessary DOM nodes around.
Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

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

Looks generally good just some grammar stuff and one code thing

RobinMalfait and others added 5 commits July 5, 2024 01:22
Co-authored-by: Jordan Pittman <jordan@cryptica.me>
Co-authored-by: Jordan Pittman <jordan@cryptica.me>
Co-authored-by: Jordan Pittman <jordan@cryptica.me>
Co-authored-by: Jordan Pittman <jordan@cryptica.me>
Co-authored-by: Jordan Pittman <jordan@cryptica.me>
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.

Nested Popovers not closing on click outside React Popover v2: warnings when used inside p-element
2 participants