Fix components not properly closing when using the transition prop
#3448
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This now relies on local state for transitions so that when the local state changes, the current component can re-render on its own already. Relying on state from the parent via context, requires the full component to re-render first which is not efficient enough for this case. We still need that more global state for cases where we want to reference an element in another component (e.g.: reference the panel id in a button's aria-controls attribute)
RobinMalfait
force-pushed
the
fix/issue-3437
branch
from
4a10e56
to
2fbdc96
Compare
thecrypticace
requested changes
Sep 3, 2024
reinink
reviewed
Sep 3, 2024
Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
RobinMalfait
added a commit
that referenced
this pull request
Sep 4, 2024
…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
This was referenced Sep 25, 2024
This was referenced Sep 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug where the components don't always properly close when using the
transitionprop 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 theuseTransition(…). This is why we store the DOM element in state instead of relying on auseRef(…).Let's say you have a
Popovercomponent, then the structure looks like this:We store a DOM reference to the button and the panel in state, and the state lives in the
Popovercomponent. 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 somearia-*attributes for example: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
Popovercomponent) eventually updates and then theuseTransition(…)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 theuseTransition(…)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