Removing of overflow: hidden on html happens before transition finished when closing Dialog

[xsjcTony]

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

For example: v1.7.15

What browser are you using?

For example: Chrome

Reproduction URL

https://stackblitz.com/edit/vitejs-vite-kgljjr?file=src%2FApp.tsx

Describe your issue

As described in the title, and please refer to my reproduction

This will cause a small flick when closing Dialog when there's a transition applied since the scrollbar appears before the transition ends.
It's working properly when opening Dialog since the scrollbar is disabled before the transition starts.

This is especially obvious when working with consequence modals, where the flick is really not ideal.

I'm aware that in #365 , you have added a padding-right to the html to let the content stop shifting, but it's not applied to the modal.

---

One possible workaround is to add another right-[calc(100%-100vw)] to the modal content container like this

<div className="fixed inset-0 right-[calc(100%-100vw)]">
  {content}
</div>

But it's not that ideal... even with this workaround, although the modal stops shifting, but the scrollbar will still appear for the leaving transition period... it's acceptable for single modal, but definitely not for consequence modals (a blink).
The ultimate solution should be only to remove overflow: hidden on html after the transition is completed.

[callmejm]

I found a workaround

import { Dialog } from '@headlessui/react';

const MyDialog = ({ isOpen, onClose }) => {
  const htmlElement = document.documentElement;

  // Add or remove a class to the HTML element when the dialog opens or closes
  if (isOpen) {
    htmlElement.classList.add('dialog-open');
  } else {
    htmlElement.classList.remove('dialog-open');
  }

  return (
    <Dialog open={isOpen} onClose={onClose}>
      {/* Dialog content */}
      <Dialog.Overlay />
      <Dialog.Title>Title</Dialog.Title>
      <Dialog.Description>Description</Dialog.Description>
      <button onClick={onClose}>Close Dialog</button>
    </Dialog>
  );
};

export default MyDialog;

html.dialog-open body {
  overflow: hidden;
}

[xsjcTony]

@callmejm Is that going to work even there's a transition? The key factor here is the transition.

Also I think this should be a built-in behaviour, since it's pretty common.

[xambassador]

You can fix it by updating the style of your main content wrapper.

{/* Update this element styles from "fixed inset-0 overflow-y-auto" */}
 <div className="fixed inset-y-0 left-0 z-10 w-screen overflow-y-auto">
  <div className="flex min-h-full items-center justify-center p-4 text-center">
    <Transition.Child
        as={Fragment}
        enter="ease-out duration-[1s]"
        enterFrom="opacity-0 scale-95"
        enterTo="opacity-100 scale-100"
        leave="ease-in duration-[1s]"
        leaveFrom="opacity-100 scale-100"
        leaveTo="opacity-0 scale-95"
      >

You can check this pull request: link

[MichaelAllenWarner]

Might be worth mentioning that if you're on Mac, you have to enable the "Show scroll bars: Always" option in system settings to see this behavior. (All front-end devs on Mac should enable this setting! See: https://www.matuzo.at/blog/css-pro-tip-for-mac-users-always-show-scroll-bars-in-macos/ )

[a1mersnow]

Why we need this unit test?

without this test, we can remove this line:

let scrollLockEnabled = computed(() => {
  // if (isClosing.value) return false
  if (dialogState.value !== DialogStates.Open) return false
  if (hasParentDialog) return false
  return true
})

Then everything will be ok!

If this is ok, I'll be happy to submit a pr.

[RobinMalfait]

Hey, thank you for this bug report! 🙏

inset-0 adjusts based on wether a scrollbar is there or not which causes that to shift to happen. To prevent this and fix it, you can apply the following diff:

- <div className="fixed inset-0 overflow-y-auto">
+ <div className="fixed inset-0 w-screen overflow-y-auto">

The w-screen will ensure that it takes up the full space including the scrollbar, therefore the jump won't happen again.

I've updated the StackBlitz example to include this change: https://stackblitz.com/edit/vitejs-vite-btbesg?file=src%2FApp.tsx

I will update the Headless UI docs and the Tailwind UI components with this change as well.

Hope this helps!

[RobinMalfait]

@a1mersnow That change was deliberate. Here is some context:

We had timing issues when using a Dialog + Transition + Next.js that does scroll restoration / re-positioning when you move to a different page.

When you have a Dialog component with a Transition component, the "closing" of the Dialog is delayed until the Transition is done. However, if you close this because you are navigating away to another page then the following scenario happens:

1. User clicks link in Dialog to navigate away
2. Next.js navigates to new page
3. Next.js restores/changes scroll position
4. In the meantime, the Dialog is still transitioning out
5. When the Transition is complete
   1. The Dialog closes
   2. The scroll locking is restored + scroll position is restored. (Which causes issues because it was already restored by Next.js)

Instead, we restore the scroll lock / scroll positions when the Transition is starting. This results in the following scenario:

1. User clicks link in Dialog to navigate away
2. Transition starts, therefore scroll lock / scroll position is restored
3. Next.js navigates to new page
4. Next.js restores/changes scroll position
5. In the meantime, the Dialog is still transitioning out
6. When the Transition is complete
   1. The Dialog closes
   2. The scroll locking was already handled earlier. No more issues from a scroll restoration perspective.

This solved the issue where the scroll positions was all wrong in certain scenarios.
Now with the additional solution here: #2525 (comment) it will also get rid of the jumping issue you are seeing.

Hope that clears things up a bit!

[0xPT]

@RobinMalfait When I utilize a NextJS in a Dialog, the page transition doesn't scroll to the top of the new page, which is the expected behavior and works whenever I utilize Links outside of Dialogs.

If I manually override the html and body styles that are added by Dialog, it works as expected.

So basically my navigation on mobile doesn't scroll to top if you're utilizing the navigation in the

Here's an example of how I'm getting my expected behavior (scroll to top on page change).. Am I doing something wrong?

  useEffect(() => {
    document.documentElement.style.overflow = "auto";
    document.body.style.marginTop = "0px";
  }, [mobileMenuOpen]);

[meylis-mobdev]

html:has(dialog[open]) {
/* remove the main scrollbar when dialog is open */
overflow: hidden;
}

this is best solution

[ESOGenics]

Why was this closed? It is still an issue. There should not be a workaround. Good development would have this built into the functionality of the product.

[alexanderflink]

We had an issue in Next.js, where the Transition component would keep overflow: hidden on the html element too long, causing the Next router to not be able to scroll to the top of the next page when routing. We solved this by removing the overflow: hidden manually in the beforeLeave method of the Transition component:

<Transition
  beforeLeave={() => {
    document.documentElement.style.overflow = ''
  }}
  ...

[maeertin]

I do like the approach of beforeLeave as that would still keep the body scroll locked while the dialog is open, and remove scroll locking only before exiting the dialog.

However in @headlessui/react version 2, the <Transition /> component is baked into the <Dialog /> component and I couldn't find a way of passing wanted props to it, in this case a beforeLeave as your suggestion @alexanderflink. This doesn't solve the root of the issue, but I do think that forwarding props to internal components is a good pattern and so I opened a PR for this #3552. I haven't contributed here before so let's see what maintainers say.

Suggested interface being:

<Dialog
  TransitionProps={{
    beforeLeave: () => {
      document.documentElement.style.overflow = ''
    },
    // ...props
  }}
>
  ...

[maeertin]

For those interested, I did get it working with the following setup:

<Transition
  as={Dialog}
  beforeLeave={() => {
    // Remove document scroll lock before dialog exit for `next/link` scroll to top to work.
    document.documentElement.style.overflow = '';
  }}
  onClose={handleClose}
  show={isOpen}
  static // opts-out of the internal `<Transition />` component
>
  ...

UPDATE: Removing overflow on beforeLeave seems to break scroll lock on mobile so couldn't go with this suggestion anyway.