Scrolling bug when opening subsequent dialogs

[elilambnz]

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

v1.5.0

What browser are you using?

Chrome

Reproduction URL

CodeSandbox minimal reproduction

Describe your issue

Related to #1000, opening a new Dialog from within another causes a bug with scrolling, as shown in the gif on the original issue.

I'm opening a new issue as there was no minimal reproduction in the original. One workaround described here https://github.com/tailwindlabs/headlessui/issues/1000#issuecomment-1001841999 which involves adding a 500ms delay before opening the next Dialog seems to fix the problem.

[phuhl]

I have the same issue, but wanted to point out, that also for no apparent reason I get the There are no focusable elements inside the <FocusTrap /> warning. I am pretty confident, that my dialogs always have focusable elements inside, this only happens, when I open the second dialog while closing the first one. (Before, this even caused a crash, but thankfully, that was resolved in https://github.com/tailwindlabs/headlessui/issues/407).

[JLambertazzo]

Can only reproduce the main issue on a project but not on the development playground..

I can reproduce the "no focusable elements inside <FocusTrap />" warning mentioned by @phuhl and maybe fixing that also fixes this issue? The warning is sent because of a check in utils/focus-management.ts at line 152, specifically the condition offset > total. When opening the nested dialog the offset value goes up to 1 which is equal to the total value (number of focusable elements, in my case 1 button in the nested dialog) which breaks the condition.

Changing line 152 with

- if (offset >= total || ...
+ if (offset > total || ...

fixes the incorrect warning message, and the if condition still does not allow infinite loops which is what it seems to be written for.

This change fixes the warning being sent incorrectly for me, but I'm also wondering:

• Is the difference of 1 that makes it work with > and not >= a solution in general or is it related to the amount of nesting?
  • Meaning if we have 3 levels of nesting will we need to check offset >= total + 3 rather than offset > total
• Could the main issue and the warning have to do with timing?
  • line 166 in focus-management.ts says focus might not work if element is "hidden" to user
  • could this apply to a dialog that is about to open?
• Does fixing the <FocusTrap /> issue fix the main issue here?

[liho00]

Same issue here +1 Temporary solution

global style

html {
  overflow: auto !important;
}

I sacrificed user experience with this fix...else unable to scroll to webpage even worst user experience...

[ryan-mahoney]

@liho00 your solution is more elegant than mine, I wrote this to reject mutations to HTML style attribute:

// this is a hack, but essentially it make it so headless ui can remove scrolling
const Observe = (sel, opt, cb) => {
  const Obs = new MutationObserver((m) => [...m].forEach(cb));
  document.querySelectorAll(sel).forEach((el) => Obs.observe(el, opt));
};

Observe("html", { attributesList: ["style"], attributeOldValue: true }, (m) => {
  if (((m["oldValue"] || "").match(/hidden/g) || []).length > 0) {
    document.getElementsByTagName("html")[0].setAttribute("style", "");
  }
});

[michaelm244]

@RobinMalfait Thoughts on making it configurable via a prop whether the Dialog should do scroll locking?

[pfcodes]

@RobinMalfait Thoughts on making it configurable via a prop whether the Dialog should do scroll locking?

+1. Scroll locking can manually be set/unset in a useEffect which will provide a less hacky way to solve this problem.

[OzkanAbdullahoglu]

conditional render for both modals (dialogs) worked for me with a little performance sacrifice maybe...

    {confirmationModalVisible && (
        <ConfirmationModal
          isOpen={confirmationModalVisible}
          closeModal={closeConfirmationModal}
          onConfirmModal={onConfirmModal}
        />
      )}
      {errorModalVisible && (
        <ErrorModal
          isOpen={errorModalVisible}
          closeModal={closeErrorModal}
          errorText={errorText}
        />
      )}

https://user-images.githubusercontent.com/34690158/175791486-7ed5e56e-792d-434c-9c07-8ad42f8bb1a6.mov

[RobinMalfait]

Hey! Thank you for your bug report! Much appreciated! 🙏

Right now we don't support opening multiple dialogs at the same time (unless they are nested). Note: this means actually nested, not triggered one Dialog from another because that would still result in incorrect behaviour if they are actually rendered as siblings instead of parent/child.

A common problem is that the Dialog is using a Transition component to fase in / out when showing / hiding the Dialog. This causes a problem where both Dialogs will be open at the same time.

I wrote more about this here: https://github.com/tailwindlabs/headlessui/issues/1744#issuecomment-1208079384

Once we implemented multiple Dialogs at the same time behaviour, then these issues should go away, but in the mean time one of the workarounds should be used.

Hope you understand and sorry for the inconvenience.

[sumanchapai]

Probably a better solution, if you're okay with closing other dialog, could be to do so perhaps by dispatching an ESCAPE keypress

element.dispatchEvent(new KeyboardEvent('keydown', {'key': 'ESCAPE'}));
setMyDialog(true)

[kluplau]

I've ran into this issue too, possibly after adding a second language. My first thought was that it was due to same IDs so the cleanup didn't work properly, but I can't tell if this is the case.