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

Popover component won't always close when clicking outside of it #2752

Closed
FabioDainese opened this issue Sep 15, 2023 Discussed in #2731 · 3 comments
Closed

Popover component won't always close when clicking outside of it #2752

FabioDainese opened this issue Sep 15, 2023 Discussed in #2731 · 3 comments
Assignees

Comments

@FabioDainese
Copy link

Discussed in #2731

Originally posted by FabioDainese September 5, 2023
What package within Headless UI are you using?
@headlessui/react (more specifically the popover component)

What version of that package are you using?
v1.7.17

What browser are you using?
Chrome (v116.0.5845.96), Firefox (v116.0.3), Safari (v16.4.1)

Reproduction URL
https://codesandbox.io/p/sandbox/flamboyant-marco-57cyzh

Describe your issue
The popover component seems to not close every time you click outside of it. To be more specific - you can try on the provided sandbox - only when you click on one of the siblings of the component that uses the popover, it won't work. So, for example, in the provided project if you click on:

  • the green div: the popover will still be open (i.e. the <main> tag is sibling to the <nav> one, which uses the popover component)
  • the white div: the popover will close (i.e. the <body> tag is parent of the <nav>, so all okay)
  • the "'body' tag - white background" text: the popover will still be open (i.e. the <p> tag that contains the text is sibling to the <nav> one)

Now, the temporary "workaround" that kinda fix this issue would be to use a transparent overlay (i.e. <Popover.Overlay className="fixed inset-0" />), so basically wherever you click, you would click on the overlay that consequently will close the popover. Now, this is far from optimal though, since if a user would like to click on an hypothetical visible button (placed outside the popover), it would need to click 2 times (one to close the popover by clicking the invisible overlay and the second one to actually trigger the button).

So, I'm not sure if this is the intended behavior for the popover component - or if I coded something wrong here - but I would like to gently ask you a feedback on it, also to understand which would be the best approach to tackle this issue (if it's not the intended behavior).

P.S. the popover used on the provided project is just a simpler copy-paste version of the first example that you can find on the headlessui website


As mentioned by @mattmcegg and @ijrzhong this behavior can be seen on v1.7.15, v1.7.16 and v1.7.17 of the @headlessui/react package (on the v1.7.14 all works normal)

@RobinMalfait RobinMalfait self-assigned this Sep 15, 2023
@RobinMalfait
Copy link
Collaborator

RobinMalfait commented Sep 18, 2023

Hey!

You didn't do anything wrong, but in Headless UI we have a few assumptions that worked fine up until now, but one of the assumptions don't work when you use the app router in Next.js. I will dig more into this exact issue soon, but a quick workaround for now to unblock you is that you apply the following diff in the layout.tsx file:

  import Navbar from '@components/navbar/navbar'
  import './globals.css'

  export default function RootLayout({
    children,
  }: {
    children: React.ReactNode
  }) {
    return (
      <html lang="en">
        <body className="p-5">
+         <div>
            <p className='text-center mb-5 bg-slate-100 rounded-lg'>&apos;body&apos; tag - white background</p>
            <Navbar />
            {children}
+         </div>
        </body>
      </html>
    )
  }

@RobinMalfait
Copy link
Collaborator

Hey!

Alright so I think for now the way to solve it is to apply the diff I mentioned above.


But I also want to give some background information about why this is even necessary in the first place and why "solving" it will very likely break existing code. I am also going to close this issue because I think this workaround is not too bad, but if more people are running into this, and solving it with an additional wrapper is too painful for one reason or another, then I'm more than happen to look into this again.

Some context:

A few components within Headless UI use different mechanisms for "closing" a component. For example the Menu, Listbox, Combobox, Dialog and the Popover component will be closed when you click outside of the component.

The click outside behaviour is trickier than you initially expect if you want to use this in real world applications. A typical click outside function listens for a click event, and receives a DOM element (e.g.: a DOM reference to the Popover component). If the click happens on an element outside of that Popover then we close it. Easy.

However, let's imagine that you have a little notification that pops up in the corner of your application because of some action that happened, dismissing that notification will close the popover. This typically happens because:

  1. The notifications is not rendered inside the popover
  2. It might be rendered inside the popover from a React tree perspective, but uses a portal such that it is rendered somewhere else in the DOM
  3. It's coming from a 3rd party library and injects some HTML into the document, which is also not rendered inside the popover component

One solution to this problem could be a Notification component that we expose from Headless UI. But this will only solve the Notification problem. There are countless of other "things" that could be rendered and that you want to interact with without closing the popover.

Another solution is that we provide a very generic <SafeArea /> component and interacting with that will be considered safe and won't close any of our components. However, we can't expect every 3rd party library on planet earth to include our SafeArea component.

Another very common problem is browser extensions. Imagine you have a Dialog with a textarea. If you used Grammerly to check the textarea contents, clicking on the Grammerly icon will... close the dialog.

This happens because Grammerly injects some elements in either the html or the body of your application, "outside" of the dialog. Therefore clicking on it will close the dialog.

To solve all of this we made some assumptions. The idea looks like this:

  1. Your main application lives in an element inside the body
    This is also where the additional wrapping div comes in. In the past, Next.js (and many other frameworks) used some element where the React or Vue application gets rendered to. <div id="app">, <div id="__next">, ...
  2. Portals, 3rd party libraries, browser extensions typically get injected into the body or the html

Next, when an outside click happens we check a few things:

  1. Is the click happening in the "main" application (point 1 from above)
    1. Is the click happening inside the Popover (or whatever component we are rendering)?
      1. Yes? Good, keep open
      2. No? Time to close the component
  2. Is the click happening in the other containers (point 2 from above)
    1. If so, we consider this allowed and therefore won't close. This of course won't be bullet proof, but for portals, notifications, browser extensions this is all fine right now. We had this feature for quite some time and not a single bug report. This has worked very well as an assumption so far.

We could find other solutions assumptions that would work without the wrapping div, but that's risky for existing applications. One that comes to mind is to store the elements that exist before opening a component, then any new element that is rendered after that is considered "safe". (You can imagine a notification or grammerly being injected after a dialog is open).

Properly fixing this without this assumption is going to be hard because we don't have control over 3rd party components or browser extensions and therefore we can't really tell if the current element should be closed or not when clicked on a specific DOM node.

We could introduce a component like Root, but this would literally just render a div right now so there is not a lot of benefit to that either.

This is all the context I wanted to provide. I hope this makes sense. Going to close the issue right now, but if more people run into this, then I'm more than happy to take another look at how we can solve this.

Thanks!

@philicious
Copy link

philicious commented Nov 26, 2023

@RobinMalfait I have the same issue and using a div wrapper as suggested yields a hydration error. If I move the div to only wrap my <Header /> its working in dev but not in production build.
I get that its related to SSR and some of my components use client, but I am not sure how to address the issue

Any advice appreciated!

    <html
      lang="de"
      className={clsx(
        'h-full scroll-smooth bg-sand antialiased',
        inter.variable,
        lexend.variable,
      )}
    >
      <body className="flex h-full flex-col">{children}</body>
    </html>
export default function Home() {
  return (
    <>
      <GTM />
      <div>
        <Header /> // use client
      </div>
      <Consent /> // use client
      <main>
        <Hero /> // use client
        <MoreComponents />
      </main>
      <Footer />
    </>
  )
}

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

3 participants