Skip to content

Fix Transition component from incorrectly exposing the Closing state #3696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 16, 2025

Conversation

RobinMalfait
Copy link
Member

This PR fixes an issue where the scroll locking logic was incorrectly re-enabled in Dialogs if you were using a Transition component or a transition prop and you had nested components with the transition prop (or a nested TransitionChild component) and the parent transition finishes before any of its children.

To visualize this, it would happen in this situation:

<Dialog transition> /* No transition classes */
  <DialogBackdrop transition className="duration-500" />
  <DialogPanel transition className="duration-200" />
  </DialogPanel>
</Dialog>

With the transition prop, internally these components would render a wrapper Transition component.
The Dialog will look at the open/closed state provided by the Transition component to know whether to unmount its children or not.

The Dialog component also has some internal hooks to make it behave as a dialog. One of those hooks is the useScrollLock hook. This hook will be enabled if the Dialog is open and disabled when it's closed.

If you are using the Transition component or the transition prop, then we have to make sure that the useScrollLock gets disabled immediate, and not wait until the transition completes. This is done by looking at the Closing state. The reason for this is that disabling the useScrollLock also means that we restore the scroll position. But if you in the meantime navigate to a different page which also changes the scroll position, then we would restore the scroll position on a totally different page.

We already had this logic setup, but the problem is that the Closing state was incorrectly derived from the transition state. That state was only looking at the current component (in the example above, the Dialog component) but not at any of the child components.

Since the Dialog didn't have any transitions itself, the Closing state was only briefly there.

If there is no Closing state, then the useScrollLock is looking at the open state of the Dialog. Because other child components were still transitioning, the Dialog was still in an open state. This actually re-enabled the useScrollLock hook. Because from the Dialogs perspective no transitions were happening anymore.

Eventually the transitions of all the children completed causing the Transition and thus the Dialog to unmount. This in turn caused the useScrollLock hook to also clean up and restore the scroll position.

But as you might have guessed, now this second time, it's restoring after the transition is done.

Luckily, the fix is simple. Make sure that the Closing state also keeps the full hierarchy into account and not only the state of the current element.

The `opening` and `closing` state was based on the transition data of
the *current* element. This means that if the parent stops transitioning
before children that the `Opening` or `Closing` state is also gone.

But should should be based on the full hierarchy such that the whole
`Dialog` is still closing if the `Dialog` or any of its children is
transitioning out.
Copy link

vercel bot commented Apr 16, 2025

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 Apr 16, 2025 6:24pm
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 16, 2025 6:24pm

@RobinMalfait RobinMalfait changed the title Fix Transition component from incorrectly re-enabling scroll locking Fix Transition component from incorrectly exposing the Closing state Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant