tailwindlabs / headlessui

Completely unstyled, fully accessible UI components, designed to integrate beautifully with Tailwind CSS.
https://headlessui.com
MIT License
25.81k stars 1.07k forks source link

Fix closing components using the `transition` prop, and after scrolling the page #3407

Closed RobinMalfait closed 1 month ago

RobinMalfait commented 2 months ago

This PR fixes an issue where components such as the MenuItems and the ListboxOptions won't close correctly if these are used in combination with the transition prop and after you scrolled the page a little bit.

For some background information: the Menu and Listbox components can be used with the transition prop so that the MenuItems and ListboxOptions can be transitioned in and out based on the open/closed state.

To make the user experience better, we have some logic to check whether the MenuButton or ListboxButton moved from its initial position. If it did move when it's closing, then we immediately stop all transitions by unmounting the MenuItems or ListboxOptions immediately.

A situation this happens in:

  1. Open the Menu (with the transition prop)
  2. Press tab to focus the next element on the page
  3. The browser scrolls the page to make the next element visible
  4. The MenuItems will fade out while moving around (while the page scrolls)
    1. Bonus points, if you also use the anchor prop, then the MenuItems will reposition itself which makes the UX even worse.

While debugging this issue, I noticed a timing related issue because we are using the DOM element in a useTransition hook. Typically you store those DOM elements in a useRef. This is good practice because the DOM element will be filled in once you hit the useEffect hook, and you don't introduce unnecessary re-renders.

However, if the DOM element changes, then the useRef value will update, but the useEffect hook doesn't run again (because no state change happened).

There are other places in Headless UI where we also need to re-run effects when the DOM node changes. There are also places where we need access to the DOM element during render.

One of React's rules is to not read / write a ref during render.

So, this PR simplifies a few components and hooks by actually storing the DOM elements in state directly instead of in a ref. This way we can use the DOM element in dependency arrays of useEffect hooks, and we can use the DOM element during render.

Maybe a bit of an unorthodox solution, but it works quite well and it simplifies the code quite a bit.


It might be easier to review commit by commit. The reason why is that in some hooks I added temporary support for HTMLElement | null in places where we wanted MutableRefObject<HTMLElement | null>. These now temporary look like MutableRefObject<HTMLElement | null> | HTMLElement | null.

This way we can use both versions while refactoring. By the end of the commits those are simplified again such that we only handle HTMLElement | null instead of MutableRefObject<HTMLElement | null>.

Fixes: #3398 Fixes: #3403

vercel[bot] commented 2 months ago

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 Aug 2, 2024 9:00am
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 2, 2024 9:00am
thecrypticace commented 2 months ago

I like how much cleaner this makes some of the code 💯