Skip to content
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

Start cleanup phase of the Dialog component when going into the Closing state #2264

Merged
merged 10 commits into from
Feb 15, 2023

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Feb 9, 2023

This PR improves when certain cleanup parts of the Dialog component happen.

The Dialog is a complex component. Once the Dialog is active it does a number of things:

  • Ensure all other elements on the page are marked as inert
  • Setup outside click behaviour
  • Setup Escape to close
  • Focuses the first focusable element in the Dialog
  • Traps focus in the Dialog
  • Ensures you can't scroll the background / interact with it

When you wrap the Dialog in a Transition, then the open state is controlled by the Transition component. This means that all of the steps above will only be "reverted" the moment the Transition component is fully done transitioning out.

This can cause some problems in SPA environments where clicking on a link goes to a new page, but the Transition/Dialog is persistent (For example when it is used as a sidebar). A lot of routers in those environments will do scroll restoration. But this means that the following will happen:

  1. Click link
  2. Application goes to new page
  3. In the meantime the sidebar Dialog wrapped in a Transition starts transitioning out
  4. Application arrives on the new page and does scroll restoration
  5. Transition is done transitioning out
  6. Dialog receives the information from the Transition component that it is done transitioning
  7. Dialog does scroll restoration

The last step is an important step for the Dialog itself because we have to apply (admittedly a bunch of hacks, especially on iOS) to make it work properly.

The big problem here is that the scroll restoration doesn't make any sense anymore because we are on a new page (potentially) already. This could now result in the fact that you are scrolled down on the new page, because you happened to be scrolled down on the previous page.

Headless UI itself is generic in the sense that we don't know anything about the applications / frameworks we are used in. So we can't listen to router events from Next.js if our components are used in a Remix application.

Instead what we can do is split up the functionality and cleanup parts in "stages". The moment our Transition component starts transitioning out, it will add the Closing state to the OpenClosed state. (Internally we have an OpenClosed context provider that allows us to have a thin layer for communication between components. This way you don't have to manually link the state from a Transition component with the state from a Menu, Listbox, Dialog, ... components).

Using this new state we can immediately start cleanup for some of the functionality in the Dialog component. For example, we can immediately do the scroll restoration while the Transition component is still transitioning.

@vercel
Copy link

vercel bot commented Feb 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 15, 2023 at 11:11AM (UTC)
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 15, 2023 at 11:11AM (UTC)

Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is handleIOSLocking the right place for this? We still have the scroll restoration bug even if it's not on iOS. Right?

I can repro this in desktop Safari. I can't repro in Chrome though weirdly enough.

Now I can't so uhh… have at it. lol

@RobinMalfait
Copy link
Member Author

RobinMalfait commented Feb 9, 2023

This is still causing issues because while we can skip the scroll restoration, the "undo" of the marginTop still needs to happen which fights against previously scroll restorations of let's say Next.js or Inertia.

Have to think harder about this one.


Update: Implemented a different approach — updated the description and title of the PR

@RobinMalfait RobinMalfait changed the title Prevent restoring the scroll position if the history changed Start cleanup of Dialog component, when in the Closing state Feb 14, 2023
@RobinMalfait RobinMalfait changed the title Start cleanup of Dialog component, when in the Closing state Start cleanup of the Dialog component when in the Closing state Feb 14, 2023
Also represent them as bits so that we can easily combine them while we
are transitioning from one state to the other.
Instead of checking whether it is in one state or an other, we can check
if the current state contains some potential sub-state.

This allows us to still check if we are in the `Open` state, while also
`Closing` because the state will be `S.Open | S.Closing`.
@RobinMalfait RobinMalfait changed the title Start cleanup of the Dialog component when in the Closing state Start cleanup phase of the Dialog component when going into the Closing state Feb 15, 2023
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.

2 participants