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 transition bug on Firefox, triggered by clicking the `PopoverButton` in rapid succession #3452

Closed RobinMalfait closed 3 weeks ago

RobinMalfait commented 3 weeks ago

We recently landed a fix for Popovers not closing correctly when using the transition prop (#3448). Once this fix was published, some users still ran into issues using Firefox on Windows (see: https://github.com/tailwindlabs/tailwindui-issues/issues/1625).

One fun thing I discovered is that transitions somehow behave differently based on where they are triggered from (?). What I mean by this is that holding down the space key on the button does properly open/close the Popover. But if you rapidly click the button, the Popover will eventually get stuck.

Note: when testing this, I made sure that the handling of the space key (in a keydown handler) and the clicking of the mouse (handled in a click handler) called the exact same code. It still happened.

The debugging continues…

One thing I noticed is that when the Popover gets stuck, it meant that a transition didn't properly complete.

The current implementation of the internal useTransition(…) hook has to wait for all the transitions to finish. This is done using a waitForTransition(…) helper. This helper sets up some event listeners (transitionstart, transitionend, …) and waits for them to fire.

This seems to be unreliable on Firefox for some unknown reason.

I knew the code for waiting for transitions wasn't ideal, so I wanted to see if using the native node.getAnimations() simplifies this and makes it work in general.

Lo and behold, it did! 🎉

This now has multiple benefits:

  1. It works as expected on Firefox
  2. The code is much much simpler
  3. Uses native features

The getAnimations(…) function is supported in all modern browsers (since 2020). At the time it was too early to rely on it, but right now it should be safe to use.

Fixes: https://github.com/tailwindlabs/tailwindui-issues/issues/1625

vercel[bot] commented 3 weeks 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 Sep 4, 2024 0:11am
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 0:11am
razzeee commented 3 weeks ago

We now have jest tests failing with


    TypeError: node.getAnimations is not a function

      at waitForTransition (node_modules/@headlessui/react/dist/headlessui.dev.cjs:3752:26)
      at node_modules/@headlessui/react/dist/headlessui.dev.cjs:3739:13
      at invokeTheCallbackFunction (node_modules/jsdom/lib/jsdom/living/generated/Function.js:19:26)
      at runAnimationFrameCallbacks (node_modules/jsdom/lib/jsdom/browser/Window.js:603:13)
      at Timeout.<anonymous> (node_modules/jsdom/lib/jsdom/browser/Window.js:581:11)
corneliusroemer commented 3 weeks ago

Indeed, also having a regression, see #3469