Don't break overflow when multiple dialogs are open at the same time

jest/create-jest-config.cjs

@@ -8,6 +8,9 @@ module.exports = function createJestConfig(root, options) {
         '^.+\\.(t|j)sx?$': '@swc/jest',
         ...transform,
       },
+      globals: {
+        __DEV__: true,
+      },
     },
     rest
   )

packages/@headlessui-react/CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Fix SSR tab hydration when using Strict Mode in development ([#2231](https://github.com/tailwindlabs/headlessui/pull/2231))
+- Don't break overflow when multiple dialogs are open at the same time ([#2215](https://github.com/tailwindlabs/headlessui/pull/2215))
 
 ## [1.7.8] - 2023-01-27
 

packages/@headlessui-react/src/components/dialog/dialog.test.tsx

@@ -1,4 +1,4 @@
-import React, { createElement, useRef, useState, Fragment, useEffect } from 'react'
+import React, { createElement, useRef, useState, Fragment, useEffect, useCallback } from 'react'
 import { render } from '@testing-library/react'
 
 import { Dialog } from './dialog'
@@ -37,13 +37,13 @@ global.IntersectionObserver = class FakeIntersectionObserver {
 afterAll(() => jest.restoreAllMocks())
 
 function nextFrame() {
-  return new Promise<void>((resolve) => {
-    requestAnimationFrame(() => {
-      requestAnimationFrame(() => {
-        resolve()
-      })
-    })
-  })
+  return frames(1)
+}
+
+async function frames(count: number) {
+  for (let n = 0; n <= count; n++) {
+    await new Promise<void>((resolve) => requestAnimationFrame(() => resolve()))
+  }
 }
 
 function TabSentinel(props: PropsOf<'button'>) {
@@ -323,6 +323,94 @@ describe('Rendering', () => {
         expect(document.documentElement.style.overflow).toBe('hidden')
       })
     )
+
+    it(
+      'scroll locking should work when transitioning between dialogs',
+      suppressConsoleLogs(async () => {
+        // While we don't support multiple dialogs
+        // We at least want to work towards supporting it at some point
+        // The first step is just making sure that scroll locking works
+        // when there are multiple dialogs open at the same time
+
+        function Example() {
+          let [dialogs, setDialogs] = useState<string[]>([])
+          let toggle = useCallback(
+            (id: string, state: 'open' | 'close') => {
+              if (state === 'open' && !dialogs.includes(id)) {
+                setDialogs([id])
+              } else if (state === 'close' && dialogs.includes(id)) {
+                setDialogs(dialogs.filter((x) => x !== id))
+              }
+            },
+            [dialogs]
+          )
+
+          return (
+            <>
+              <DialogWrapper id="d1" dialogs={dialogs} toggle={toggle} />
+              <DialogWrapper id="d2" dialogs={dialogs} toggle={toggle} />
+              <DialogWrapper id="d3" dialogs={dialogs} toggle={toggle} />
+            </>
+          )
+        }
+
+        function DialogWrapper({
+          id,
+          dialogs,
+          toggle,
+        }: {
+          id: string
+          dialogs: string[]
+          toggle: (id: string, state: 'open' | 'close') => void
+        }) {
+          return (
+            <>
+              <button id={`open_${id}`} onClick={() => toggle(id, 'open')}>
+                Open {id}
+              </button>
+              <Transition as={Fragment} show={dialogs.includes(id)}>
+                <Dialog onClose={() => toggle(id, 'close')}>
+                  <button id={`close_${id}`} onClick={() => toggle(id, 'close')}>
+                    Close {id}
+                  </button>
+                </Dialog>
+              </Transition>
+            </>
+          )
+        }
+
+        render(<Example />)
+
+        // No overflow yet
+        expect(document.documentElement.style.overflow).toBe('')
+
+        let open1 = () => document.getElementById('open_d1')
+        let open2 = () => document.getElementById('open_d2')
+        let open3 = () => document.getElementById('open_d3')
+        let close3 = () => document.getElementById('close_d3')
+
+        // Open the dialog & expect overflow
+        await click(open1())
+        await frames(2)
+        expect(document.documentElement.style.overflow).toBe('hidden')
+
+        // Open the dialog & expect overflow
+        await click(open2())
+        await frames(2)
+        expect(document.documentElement.style.overflow).toBe('hidden')
+
+        // Open the dialog & expect overflow
+        await click(open3())
+        await frames(2)
+        expect(document.documentElement.style.overflow).toBe('hidden')
+
+        // At this point only the last dialog should be open
+        // Close the dialog & dont expect overflow
+        await click(close3())
+        await frames(2)
+        expect(document.documentElement.style.overflow).toBe('')
+      })
+    )
   })
 
   describe('Dialog.Overlay', () => {

packages/@headlessui-react/src/components/dialog/dialog.tsx

@@ -37,8 +37,7 @@ import { useOwnerDocument } from '../../hooks/use-owner'
 import { useEventListener } from '../../hooks/use-event-listener'
 import { Hidden, Features as HiddenFeatures } from '../../internal/hidden'
 import { useEvent } from '../../hooks/use-event'
-import { disposables } from '../../utils/disposables'
-import { isIOS } from '../../utils/platform'
+import { useDocumentOverflowLockedEffect } from '../../hooks/document-overflow/use-document-overflow'
 
 enum DialogStates {
   Open,
@@ -96,110 +95,9 @@ function useScrollLock(
   enabled: boolean,
   resolveAllowedContainers: () => HTMLElement[] = () => [document.body]
 ) {
-  useEffect(() => {
-    if (!enabled) return
-    if (!ownerDocument) return
-
-    let d = disposables()
-    let scrollPosition = window.pageYOffset
-
-    function style(node: HTMLElement, property: string, value: string) {
-      let previous = node.style.getPropertyValue(property)
-      Object.assign(node.style, { [property]: value })
-      return d.add(() => {
-        Object.assign(node.style, { [property]: previous })
-      })
-    }
-
-    let documentElement = ownerDocument.documentElement
-    let ownerWindow = ownerDocument.defaultView ?? window
-
-    let scrollbarWidthBefore = ownerWindow.innerWidth - documentElement.clientWidth
-    style(documentElement, 'overflow', 'hidden')
-
-    if (scrollbarWidthBefore > 0) {
-      let scrollbarWidthAfter = documentElement.clientWidth - documentElement.offsetWidth
-      let scrollbarWidth = scrollbarWidthBefore - scrollbarWidthAfter
-      style(documentElement, 'paddingRight', `${scrollbarWidth}px`)
-    }
-
-    if (isIOS()) {
-      style(ownerDocument.body, 'marginTop', `-${scrollPosition}px`)
-      window.scrollTo(0, 0)
-
-      // Relatively hacky, but if you click a link like `<a href="#foo">` in the Dialog, and there
-      // exists an element on the page (outside of the Dialog) with that id, then the browser will
-      // scroll to that position. However, this is not the case if the element we want to scroll to
-      // is higher and the browser needs to scroll up, but it doesn't do that.
-      //
-      // Let's try and capture that element and store it, so that we can later scroll to it once the
-      // Dialog closes.
-      let scrollToElement: HTMLElement | null = null
-      d.addEventListener(
-        ownerDocument,
-        'click',
-        (e) => {
-          if (e.target instanceof HTMLElement) {
-            try {
-              let anchor = e.target.closest('a')
-              if (!anchor) return
-              let { hash } = new URL(anchor.href)
-              let el = ownerDocument.querySelector(hash)
-              if (el && !resolveAllowedContainers().some((container) => container.contains(el))) {
-                scrollToElement = el as HTMLElement
-              }
-            } catch (err) {}
-          }
-        },
-        true
-      )
-
-      d.addEventListener(
-        ownerDocument,
-        'touchmove',
-        (e) => {
-          // Check if we are scrolling inside any of the allowed containers, if not let's cancel the event!
-          if (
-            e.target instanceof HTMLElement &&
-            !resolveAllowedContainers().some((container) =>
-              container.contains(e.target as HTMLElement)
-            )
-          ) {
-            e.preventDefault()
-          }
-        },
-        { passive: false }
-      )
-
-      // Restore scroll position
-      d.add(() => {
-        // Before opening the Dialog, we capture the current pageYOffset, and offset the page with
-        // this value so that we can also scroll to `(0, 0)`.
-        //
-        // If we want to restore a few things can happen:
-        //
-        // 1. The window.pageYOffset is still at 0, this means nothing happened, and we can safely
-        // restore to the captured value earlier.
-        // 2. The window.pageYOffset is **not** at 0. This means that something happened (e.g.: a
-        // link was scrolled into view in the background). Ideally we want to restore to this _new_
-        // position. To do this, we can take the new value into account with the captured value from
-        // before.
-        //
-        // (Since the value of window.pageYOffset is 0 in the first case, we should be able to
-        // always sum these values)
-        window.scrollTo(0, window.pageYOffset + scrollPosition)
-
-        // If we captured an element that should be scrolled to, then we can try to do that if the
-        // element is still connected (aka, still in the DOM).
-        if (scrollToElement && scrollToElement.isConnected) {
-          scrollToElement.scrollIntoView({ block: 'nearest' })
-          scrollToElement = null
-        }
-      })
-    }
-
-    return d.dispose
-  }, [ownerDocument, enabled])
+  useDocumentOverflowLockedEffect(ownerDocument, enabled, (meta) => ({
+    containers: [...(meta.containers ?? []), resolveAllowedContainers],
+  }))
 }
 
 function stateReducer(state: StateDefinition, action: Actions) {

packages/@headlessui-react/src/hooks/document-overflow/adjust-scrollbar-padding.ts

@@ -0,0 +1,25 @@
+import { ScrollLockStep } from './overflow-store'
+
+export function adjustScrollbarPadding(): ScrollLockStep {
+  let scrollbarWidthBefore: number
+
+  return {
+    before({ doc }) {
+      let documentElement = doc.documentElement
+      let ownerWindow = doc.defaultView ?? window
+
+      scrollbarWidthBefore = ownerWindow.innerWidth - documentElement.clientWidth
+    },
+
+    after({ doc, d }) {
+      let documentElement = doc.documentElement
+
+      // Account for the change in scrollbar width
+      // NOTE: This is a bit of a hack, but it's the only way to do this
+      let scrollbarWidthAfter = documentElement.clientWidth - documentElement.offsetWidth
+      let scrollbarWidth = scrollbarWidthBefore - scrollbarWidthAfter
+
+      d.style(documentElement, 'paddingRight', `${scrollbarWidth}px`)
+    },
+  }
+}

packages/@headlessui-react/src/hooks/document-overflow/handle-ios-locking.ts

@@ -0,0 +1,98 @@
+import { isIOS } from '../../utils/platform'
+import { ScrollLockStep } from './overflow-store'
+
+interface ContainerMetadata {
+  containers: (() => HTMLElement[])[]
+}
+
+export function handleIOSLocking(): ScrollLockStep<ContainerMetadata> {
+  if (!isIOS()) {
+    return {}
+  }
+
+  let scrollPosition: number
+
+  return {
+    before() {
+      scrollPosition = window.pageYOffset
+    },
+
+    after({ doc, d, meta }) {
+      function inAllowedContainer(el: HTMLElement) {
+        return meta.containers
+          .flatMap((resolve) => resolve())
+          .some((container) => container.contains(el))
+      }
+
+      d.style(doc.body, 'marginTop', `-${scrollPosition}px`)
+      window.scrollTo(0, 0)
+
+      // Relatively hacky, but if you click a link like `<a href="#foo">` in the Dialog, and there
+      // exists an element on the page (outside of the Dialog) with that id, then the browser will
+      // scroll to that position. However, this is not the case if the element we want to scroll to
+      // is higher and the browser needs to scroll up, but it doesn't do that.
+      //
+      // Let's try and capture that element and store it, so that we can later scroll to it once the
+      // Dialog closes.
+      let scrollToElement: HTMLElement | null = null
+      d.addEventListener(
+        doc,
+        'click',
+        (e) => {
+          if (!(e.target instanceof HTMLElement)) {
+            return
+          }
+
+          try {
+            let anchor = e.target.closest('a')
+            if (!anchor) return
+            let { hash } = new URL(anchor.href)
+            let el = doc.querySelector(hash)
+            if (el && !inAllowedContainer(el as HTMLElement)) {
+              scrollToElement = el as HTMLElement
+            }
+          } catch (err) {}
+        },
+        true
+      )
+
+      d.addEventListener(
+        doc,
+        'touchmove',
+        (e) => {
+          // Check if we are scrolling inside any of the allowed containers, if not let's cancel the event!
+          if (e.target instanceof HTMLElement && !inAllowedContainer(e.target as HTMLElement)) {
+            e.preventDefault()
+          }
+        },
+        { passive: false }
+      )
+
+      // Restore scroll position
+      d.add(() => {
+        // Before opening the Dialog, we capture the current pageYOffset, and offset the page with
+        // this value so that we can also scroll to `(0, 0)`.
+        //
+        // If we want to restore a few things can happen:
+        //
+        // 1. The window.pageYOffset is still at 0, this means nothing happened, and we can safely
+        // restore to the captured value earlier.
+        // 2. The window.pageYOffset is **not** at 0. This means that something happened (e.g.: a
+        // link was scrolled into view in the background). Ideally we want to restore to this _new_
+        // position. To do this, we can take the new value into account with the captured value from
+        // before.
+        //
+        // (Since the value of window.pageYOffset is 0 in the first case, we should be able to
+        // always sum these values)
+        window.scrollTo(0, window.pageYOffset + scrollPosition)
+
+        // If we captured an element that should be scrolled to, then we can try to do that if the
+        // element is still connected (aka, still in the DOM).
+        if (scrollToElement && scrollToElement.isConnected) {
+          scrollToElement.scrollIntoView({ block: 'nearest' })
+          scrollToElement = null
+        }
+      })
+    },
+  }
+}