Bug on headlessui Popover #1625
Comments
|
Hey @M4RC0Sx, thanks for bringing this to our attention. I'm going to close this issue in favor of tailwindlabs/headlessui#3438 (I see you commented there too). Hopefully we can get this one fixed up quickly, but until then I recommend staying on v2.1.2 👍 |
…3448) This PR fixes a bug where the components don't always properly close when using the `transition` prop on those components. The issue here is that the internal `useTransition(…)` hook relies on a DOM node. Whenever the DOM node changes, we need to re-run the `useTransition(…)`. This is why we store the DOM element in state instead of relying on a `useRef(…)`. Let's say you have a `Popover` component, then the structure looks like this: ```ts <Popover> <PopoverButton>Show</PopoverButton> <PopoverPanel>Contents</PopoverPanel> </Popover> ``` We store a DOM reference to the button and the panel in state, and the state lives in the `Popover` component. The reason we do that is so that the button can reference the panel and the panel can reference the button. This is needed for some `aria-*` attributes for example: ```ts <PopoverButton aria-controls={panelElement.id}> ``` For the transitions, we set some state to make sure that the panel is visible or hidden, then we wait for transitions to finish by listening to transition related events on the DOM node directly. If you now say, "hey panel, please re-render because you have to become visible/hidden" then the component re-renders, the panel DOM node (stored in the `Popover` component) eventually updates and then the `useTransition(…)` hooks receives the new value (either the DOM node or null when the leave transition is complete). The problem here is the round trip that it first has to go to the root `<Popover/>` component, re-render everything and provide the new DOM node to the `useTransition(…)` hook. The solution? Local state so that the panel can re-render on its own and doesn't require the round trip via the parent. Fixes: #3438 Fixes: #3437 Fixes: tailwindlabs/tailwindui-issues#1625 --------- Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
|
Hey sorry about the regression! This should be fixed, and is available in the next release of Headless UI (2.1.4). |
Hi @RobinMalfait ! This is not fixed in 2.1.4, when you spam-click the button it stops working. Could you reopen the issue? |
|
Hmm, can you verify a few things for me?
That said, this is the behavior I see:
Caution Due to the spamming of the button, there will be flashing content in the video below. This video may potentially trigger seizures for people with photosensitive epilepsy. Screen.Recording.2024-09-04.at.10.32.44.mov |
|
Hi @RobinMalfait . Just tried it with all the steps you said and still getting same issue. |
|
@M4RC0Sx can you share a (minimal) reproduction GitHub repo by any chance so that I can take a look? |
|
Just invited you to my repo. Latest changes with your steps are on develop branch. @RobinMalfait |
|
I see the same correct behavior on your repo: Screen.Recording.2024-09-04.at.10.59.35.movAssuming you did all the steps I mentioned:
Can you share which OS you are using, which browser and what version of that browser? |
|
Yes, all the steps done @RobinMalfait . I am using Windows 11 PRO 23H2 with WSL Ubuntu 20.04.6. |
…on` in rapid succession (#3452) We recently landed a fix for `Popover`s 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: tailwindlabs/tailwindui-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 <kbd>space</kbd> 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: tailwindlabs/tailwindui-issues#1625
|
Hey @M4RC0Sx I was able to reproduce this issue on Firefox. I wrote more about it here: #3452 Before publishing a new version, could you try with this insiders version: Let me know if this behaves the way you expect it to behave 👍 |
|
Hi @RobinMalfait ! Thank you very much! Works like a charm with the insiders version! |
|
Beautiful! I published this as a new version 2.1.5 |
What component (if applicable)
Describe the bug
Using the exact component you have on the web on a NextJS 14.2.7 with React 18 app, if you spam click the toggle mobile navbar button which open and closes de Popover, it stops working. If I do it on your web it is not happening. Using @headlessui/react@2.1.2 becasue 2.1.3 totally breaks the popover open-close behavior.
To Reproduce
Expected behavior
You can spam click the button and the popover menu just opens and closes normally.
Screenshots
Not applicable
Browser/Device (if applicable)
The text was updated successfully, but these errors were encountered: