Popover does not close

[blasterbug]

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

v1.0.0

What browser are you using?

chrome, safari, firefox

Reproduction repository

https://github.com/blasterbug/headlessui-popover-does-not-close.git

As state in the doc "Popovers are perfect for floating panels with arbitrary content like navigation menus, mobile menus and flyout menus." so the expected behaviour is that the panel closes when an item inside is clicked. The use case is when a user opens such a menu to navigate and they click on a link within the panel then the panel disappears.

[rajohan]

I think it works as expected considering that the Popover is made to be able to contain any element. If it would close whenever any item inside is clicked you would have problems implementing a Popover containing a input as an example.

But I agree that it should be possible to make it close when specific items are clicked. I think the best solution may be to expose a render prop along with the "open" prop that makes you able to toggle the state.

Then you would be able to do something like this

const MyPopover = () => (
  <Popover>
    {({ open, setOpen }) => (
      <>
        <Popover.Button>
          <span className="sr-only">Open main menu</span>
          {open ? <XIcon /> : <MenuIcon />}
        </Popover.Button>
        <Popover.Panel>
          // clicking inside this wont make the popover close
          <input type="text" />
          // clicking any of these links will close the popover
          <a href="/link1" onClick={() => setOpen(false)}>
            Link 1
          </a>
          <a href="/link2" onClick={() => setOpen(false)}>
            Link 2
          </a>
          <a href="/link3" onClick={() => setOpen(false)}>
            Link 3
          </a>
          <a href="/link4" onClick={() => setOpen(false)}>
            Link 4
          </a>
        </Popover.Panel>
      </>
    )}
  </Popover>
);

[blasterbug]

@rajohan I guess another solution could be to have a prop on Popover to specify if the panel should close when an element inside is clicked.

[desaintflorent]

I agree this is really confusing, I would also expect the popover to close when I a link is clicked inside...

[nathanheffley]

@desaintflorent does your popover only have links inside of it, or is it contain a mix of elements where some should close the popover and some shouldn't?

[desaintflorent]

Only links ! this is for an header menu.
I'm using vue and can't find in the doc how to programmatically close the popover when the link is clicked ?

[nathanheffley]

Why not use the Menu component that is specifically built for being a dropdown navigation element, which closes when you select an option?

[rajohan]

Using the Menu component should probably be preferred when you only have links. But, its not possible if you need other elements as well. In my opinion the Popover component should have a way to programmatically close it.

There is also the consideration of accessibility as the Menu uses arrow keys to go up and down in the menu and the Popover component uses tab. For a nav menu I think tab to navigate the links is what's usually used.

[nathanheffley]

@rajohan tab may be the way that is most common, but that is not the most accessible. It is only most common because that is what browsers do by default and most developers don't fix it. If you refer to the W3's examples of best practices for a dropdown menu, you can see that arrow keys are the preferred method of navigating a dropdown nav menu: https://www.w3.org/TR/wai-aria-practices-1.1/examples/menu-button/menu-button-links.html

[rajohan]

@nathanheffley I agree that using the arrow keys might be easier. So i guess you have to choose between common vs easier to use.

You also have to consider that the navigation in many cases are a hamburger menu on small screens and on one row on larger screens. So by using the Menu component on small screens by default you would get 2 different ways of navigating the menu. Arrow keys on small screens, tab on larger screens. You could of course fix this with some JavaScript.

[blasterbug]

@nathanheffley FYI, on TailwindUI, as far as I know they only use Popover for navigation (ie panels that contains only links) for header menu...

[nathanheffley]

@blasterbug well if you use a links in the popover like Tailwind UI does then it wouldn't be an issue because navigating to a new page would "close" the popover.

I agree a programmatic way to close it would be nice probably, but this issue seems to be resolvable by using a Menu and submitting a new, clear feature request as this convo has gotten pretty wide in terms of subject matter.

[blasterbug]

@nathanheffley This is not accurate, for instance with react-router, the popover stays open. See the exemple I provided.

[nathanheffley]

@blasterbug I'm still confused as to your issue. Does the Menu element not work for your use case?

[blasterbug]

@nathanheffley sure Menu might work, as long as the content of the panel stay simple. But regarding TailwindUI I should use Popover. And reading the doc, that the components that seems more logical to use.

[RobinMalfait]

Hey! Thank you for your bug report!
Much appreciated! 🙏

We can't automatically close the Popover because we don't know what kind of elements people will use in their applications. Some people will add some content where they expect that the Popover stays open, some people will have links that redirect somewhere and then they expect the Popover to close. Automatically closing is not an option here. We could add a prop that controls the auto close effect (as mentioned by @blasterbug in #358 (comment)), but even then you could have multiple pieces of content where part A requires the Popover to close and part B requires the Popover to stay open.

There is another issue, you are using a router library, but Headless UI is not aware of this router, so there is no way to know when the route changed. In a non-spa world, the browser would refresh and the new content would be there and the Popover would be closed, but now that's not the case.

What are some of the possible solutions?

• A first solution would be to expose a close function so that you can imperatively close the Popover yourself (as mentioned by @rajohan in Popover does not close #358 (comment)). However this has an issue, if you don't move the focus to the new content, then by default the body will get focus, which is very confusing for screenreader users. We could move the focus to the Popover.Button, which is fine, but then screenreader users won't be aware of the new content either.
• Let's talk about the accessibility issues. The Popover will close when the focus is moved to an element outside of the Popover / Popover.Group. So what you should do is move the focus when new content appears.

When I install and run your reproduction application, this is what screenreader users will hear:

Screen.Recording.2021-04-19.at.16.50.12.mov

A few things to notice here:

• You don't see a visual change (the Popover doesn't close)
• I am using tab to move through the items. When I press enter to "click" the link, nothing happens.
  • This is very important. If we would expose a close function, then sighted people will see the popover close. But screenreader users will still be unaware of new content.

---

In the next video I applied a fix (checkout the pr) so that whenever the content changes, the focus gets moved to the new content.

Screen.Recording.2021-04-19.at.16.53.43.mov

A few things to notice here:

• The Popover component closes as expected, this is good for sighted people!
• The new content gets announced to screenreader users, also good!
• I didn't need to call a close function manually. Even if we exposed this function, for accessibility reasons you would still have to move the focus manually and at that point the close function will be unnecessary.

If you use a router like Reach Router then they manage the focus for you so that you get the expected result.

---

Long story short, sometimes there is a bit of friction, but the friction is good for everyone.

---

I also see some confusion about using a Popover vs a Menu. We talk about that in the docs here: https://headlessui.dev/react/menu#when-to-use-a-menu and https://headlessui.dev/react/popover#when-to-use-a-popover

A Menu is good for things like profile dropdowns, or an actions dropdown at the end of a table row. A Popover for navigation is preferred because you can use the normal tab order to move through items. It's also possible to put multiple Popover's in a Popover group, which means that if you are at the end of Popover A, press tab and go to Popover B, then Popover A is still open. This is useful for when you press shift+tab so that you end up at the end of the Popover A, without going through the full list again.

A good use case for that is on pages like webshops:

Imagine I used tab to go to the next category because I accidentally was too quick. If you would close the currently open Popover then keyboard users have to go through the full list again, just to be at the very last item.

---

Does this make sense, and does this answer your question?
Checkout the PR I provided: blasterbug/headlessui-popover-does-not-close#1

[blasterbug]

Hi @RobinMalfait
And thank you for the detailed reply, very good as well as for the PR.
However,

If you use a router like Reach Router then they manage the focus for you so that you get the expected result.

This is actually always the case. Or what do you mean they manage the focus for you?

[RobinMalfait]

Reach Router will move the focus when there is new content: https://github.com/reach/router/blob/d2c9ad06715c9d48c2d16f55f7cd889b626d2521/src/index.js#L291

However as far as I know, React Router doesn't do this, hence why I did it manually in the PR.

[amantiwari1]

so where is the solution?

[devrsi0n]

Found a trick to make it work, not perfect though.

const buttonRef = useRef();
const handleClickPanel = () => buttonRef.current?.click();

<Popover>
  <Popover.Button ref={buttonRef}>Click me</Popover.Button>
  <Popover.Panel onClick={handleClickPanel} onKeyDown={handleClickPanel} role="button" tabIndex={0}>
   {content}
  </Popover.Panel>
</Popover>

[tvarwig]

Found a trick to make it work, not perfect though.

const buttonRef = useRef();
const handleClickPanel = () => buttonRef.current?.click();

<Popover>
  <Popover.Button ref={buttonRef}>Click me</Popover.Button>
  <Popover.Panel onClick={handleClickPanel} onKeyDown={handleClickPanel} role="button" tabIndex={0}>
   {content}
  </Popover.Panel>
</Popover>

This example does not work in typescript. The error is pointing to ref={buttonRef}

Type error: Type 'MutableRefObject<undefined>' is not assignable to type 'LegacyRef<HTMLButtonElement> | undefined'.
Type 'MutableRefObject<undefined>' is not assignable to type 'RefObject<HTMLButtonElement>'.
Types of property 'current' are incompatible.
Type 'undefined' is not assignable to type 'HTMLButtonElement | null'.

[desaintflorent]

I have to tell, what a nightmare to simply close a popover when we click on a link.
I read again all the comments one by one, and what I understand is that we should find a way to "focus out" when route is changing, instead of just closing the popover manually.

I'm using Intertiajs with Vue 3, and there is a route change event but the focus is not changing automatically.
So what I did, is adding a div to wrap my app and execute :

import { Inertia } from "@inertiajs/inertia";

Inertia.on("success", (event) => {
    document.getElementById("wrap").focus();
});

To be able to focus on a div we also have to add tabindex to the wrap div ( tabindex="-1" )
Look dirty to me, but didn't find any other solution online. I will ask inertia if there is another solution.

[nibtime]

Hi,

I am currently working on a new project with Tailwind and Tailwind UI and I was also struggling with closing popovers on navigation.

I am using Next.js and found that the best solution, in this case, is a hook useShiftFocusOnNavigation I include in _app.tsx that decides how to shift focus on internal navigation events from next/router.

Here is the code as-is from my current project. I wanted to share it in case somebody with Next.js can save some time on this and also to get feedback .With such a11y-impacting things it's somehow very easy to think to have a good solution, while in reality it really isn't :).

utils/browser.ts

export const selectFocusable = (
  el: Element | null | undefined
): HTMLElement | null | undefined => {
  return el?.querySelector(
    `button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])`
  );
};
export const isElementVisible = (el: Element | null | undefined) => {
  if (!el) return false;
  const rect = el.getBoundingClientRect();
  if (!rect.height && !rect.width) {
    return false;
  }
  if (getComputedStyle(el).visibility === 'hidden') {
    return false;
  }
  return true;
};

export const selectAllVisible = (
  selector: string,
  querySelectorAll?: (selector: string) => NodeListOf<Element>
) => {
  const elements =
    querySelectorAll?.(selector) || document.querySelectorAll(selector);
  return [...elements].filter(isElementVisible);
};

hooks/useShiftFocusOnNavigation.tsx

import { useRouter } from 'next/router';
import { useEffect } from 'react';
import { selectFocusable, selectAllVisible } from 'utils/browser';

const hashChangedHandler = (url: string) => {
  const [, fragment] = url.split('#');
  selectFocusable(document.getElementById(fragment))?.focus();
};
const routeChangedHandler = (url: string) => {
  const [, fragment] = url.split('#');
  const focusable =
    // shift focus to the first focusable element in the fragment section
    (fragment && selectFocusable(document.getElementById(fragment))) ||
    // or shift focus to the first call-to-action in main
    selectFocusable(document.querySelector('main')?.querySelector('.cta')) ||
    // or shift focus to the first focusable within h1 of main (titles with self-link)
    selectFocusable(document.querySelector('main')?.querySelector('h1')) ||
    // or shift focus to the first visible navigation link
    selectFocusable(selectAllVisible('.nav-focus-container')[0]);
  focusable?.focus();
};

const useShiftFocusOnNavigation = () => {
  const { events } = useRouter();
  useEffect(() => {
    events.on('hashChangeComplete', hashChangedHandler);
    events.on('routeChangeComplete', routeChangedHandler);
    return () => {
      events.off('hashChangeComplete', hashChangedHandler);
      events.off('routeChangeComplete', routeChangedHandler);
    };
  }, []);
};

export default useShiftFocusOnNavigation;

For this, to work you also need to provide components/elements that can be sensibly focused (e.g. slugify section titles and auto-link the section title to its own section fragment), and if the HTML of pages is sound and consistent this will be helpful as well.

Then you get the popover closing for free :)

[CR1AT0RS]

For those looking answer for this. Answer is here:
#427 (comment)