Fix Dialog cycling (#553)

* add tests to verify that tabbing around when using `initialFocus` works * add nesting example to `playground-vue` * fix nested dialog and initialFocus cycling * make React dialog consistent - Disable FocusLock on leaf Dialog's * update changelog
tailwindlabs · Mar 7, 2022 · e0cc62c · e0cc62c
1 parent 27dece1
commit e0cc62c
Show file tree

Hide file tree

Showing 11 changed files with 403 additions and 176 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Reset Combobox Input when the value gets reset ([#1181](https://github.com/tailwindlabs/headlessui/pull/1181))
 - Adjust active {item,option} index ([#1184](https://github.com/tailwindlabs/headlessui/pull/1184))
 - Fix re-focusing element after close ([#1186](https://github.com/tailwindlabs/headlessui/pull/1186))
+- Fix `Dialog` cycling ([#553](https://github.com/tailwindlabs/headlessui/pull/553))
 
 ## [@headlessui/react@v1.5.0] - 2022-02-17
 

diff --git a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx
@@ -1,4 +1,4 @@
-import React, { createElement, useState } from 'react'
+import React, { createElement, useRef, useState } from 'react'
 import { render } from '@testing-library/react'
 
 import { Dialog } from './dialog'
@@ -515,6 +515,57 @@ describe('Keyboard interactions', () => {
       })
     )
   })
+
+  describe('`Tab` key', () => {
+    it(
+      'should be possible to tab around when using the initialFocus ref',
+      suppressConsoleLogs(async () => {
+        function Example() {
+          let [isOpen, setIsOpen] = useState(false)
+          let initialFocusRef = useRef(null)
+          return (
+            <>
+              <button id="trigger" onClick={() => setIsOpen((v) => !v)}>
+                Trigger
+              </button>
+              <Dialog open={isOpen} onClose={setIsOpen} initialFocus={initialFocusRef}>
+                Contents
+                <TabSentinel id="a" />
+                <input type="text" id="b" ref={initialFocusRef} />
+              </Dialog>
+            </>
+          )
+        }
+        render(<Example />)
+
+        assertDialog({ state: DialogState.InvisibleUnmounted })
+
+        // Open dialog
+        await click(document.getElementById('trigger'))
+
+        // Verify it is open
+        assertDialog({
+          state: DialogState.Visible,
+          attributes: { id: 'headlessui-dialog-1' },
+        })
+
+        // Verify that the input field is focused
+        assertActiveElement(document.getElementById('b'))
+
+        // Verify that we can tab around
+        await press(Keys.Tab)
+        assertActiveElement(document.getElementById('a'))
+
+        // Verify that we can tab around
+        await press(Keys.Tab)
+        assertActiveElement(document.getElementById('b'))
+
+        // Verify that we can tab around
+        await press(Keys.Tab)
+        assertActiveElement(document.getElementById('a'))
+      })
+    )
+  })
 })
 
 describe('Mouse interactions', () => {
@@ -762,19 +813,17 @@ describe('Nesting', () => {
     let [showChild, setShowChild] = useState(false)
 
     return (
-      <>
-        <Dialog open={true} onClose={onClose}>
-          <Dialog.Overlay />
-
-          <div>
-            <p>Level: {level}</p>
-            <button onClick={() => setShowChild(true)}>Open {level + 1} a</button>
-            <button onClick={() => setShowChild(true)}>Open {level + 1} b</button>
-            <button onClick={() => setShowChild(true)}>Open {level + 1} c</button>
-          </div>
-          {showChild && <Nested onClose={setShowChild} level={level + 1} />}
-        </Dialog>
-      </>
+      <Dialog open={true} onClose={onClose}>
+        <Dialog.Overlay />
+
+        <div>
+          <p>Level: {level}</p>
+          <button onClick={() => setShowChild(true)}>Open {level + 1} a</button>
+          <button onClick={() => setShowChild(true)}>Open {level + 1} b</button>
+          <button onClick={() => setShowChild(true)}>Open {level + 1} c</button>
+        </div>
+        {showChild && <Nested onClose={setShowChild} level={level + 1} />}
+      </Dialog>
     )
   }
 

diff --git a/packages/@headlessui-react/src/components/dialog/dialog.tsx b/packages/@headlessui-react/src/components/dialog/dialog.tsx
@@ -165,6 +165,7 @@ let DialogRoot = forwardRefWithAs(function Dialog<
       `You provided an \`onClose\` prop to the \`Dialog\`, but the value is not a function. Received: ${onClose}`
     )
   }
+
   let dialogState = open ? DialogStates.Open : DialogStates.Closed
   let visible = (() => {
     if (usesOpenClosedState !== null) {
@@ -200,7 +201,7 @@ let DialogRoot = forwardRefWithAs(function Dialog<
     enabled
       ? match(position, {
           parent: FocusTrapFeatures.RestoreFocus,
-          leaf: FocusTrapFeatures.All,
+          leaf: FocusTrapFeatures.All & ~FocusTrapFeatures.FocusLock,
         })
       : FocusTrapFeatures.None,
     { initialFocus, containers }

diff --git a/packages/@headlessui-react/src/hooks/use-focus-trap.ts b/packages/@headlessui-react/src/hooks/use-focus-trap.ts
@@ -41,9 +41,7 @@ export function useFocusTrap(
     containers?: MutableRefObject<Set<MutableRefObject<HTMLElement | null>>>
   } = {}
 ) {
-  let restoreElement = useRef<HTMLElement | null>(
-    typeof window !== 'undefined' ? (document.activeElement as HTMLElement) : null
-  )
+  let restoreElement = useRef<HTMLElement | null>(null)
   let previousActiveElement = useRef<HTMLElement | null>(null)
   let mounted = useIsMounted()
 
@@ -54,7 +52,9 @@ export function useFocusTrap(
   useEffect(() => {
     if (!featuresRestoreFocus) return
 
-    restoreElement.current = document.activeElement as HTMLElement
+    if (!restoreElement.current) {
+      restoreElement.current = document.activeElement as HTMLElement
+    }
   }, [featuresRestoreFocus])
 
   // Restore the focus when we unmount the component.
@@ -70,7 +70,8 @@ export function useFocusTrap(
   // Handle initial focus
   useEffect(() => {
     if (!featuresInitialFocus) return
-    if (!container.current) return
+    let containerElement = container.current
+    if (!containerElement) return
 
     let activeElement = document.activeElement as HTMLElement
 
@@ -79,7 +80,7 @@ export function useFocusTrap(
         previousActiveElement.current = activeElement
         return // Initial focus ref is already the active element
       }
-    } else if (container.current.contains(activeElement)) {
+    } else if (containerElement.contains(activeElement)) {
       previousActiveElement.current = activeElement
       return // Already focused within Dialog
     }
@@ -88,7 +89,7 @@ export function useFocusTrap(
     if (initialFocus?.current) {
       focusElement(initialFocus.current)
     } else {
-      if (focusIn(container.current, Focus.First) === FocusResult.Error) {
+      if (focusIn(containerElement, Focus.First) === FocusResult.Error) {
         console.warn('There are no focusable elements inside the <FocusTrap />')
       }
     }

diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts
@@ -637,6 +637,68 @@ describe('Keyboard interactions', () => {
       })
     )
   })
+
+  describe('`Tab` key', () => {
+    it(
+      'should be possible to tab around when using the initialFocus ref',
+      suppressConsoleLogs(async () => {
+        renderTemplate({
+          template: `
+            <div>
+              <button id="trigger" @click="toggleOpen">
+                Trigger
+              </button>
+              <Dialog :open="isOpen" @close="setIsOpen" :initialFocus="initialFocusRef">
+                Contents
+                <TabSentinel id="a" />
+                <input type="text" id="b" ref="initialFocusRef" />
+              </Dialog>
+            </div>
+          `,
+          setup() {
+            let isOpen = ref(false)
+            let initialFocusRef = ref(null)
+            return {
+              isOpen,
+              initialFocusRef,
+              setIsOpen(value: boolean) {
+                isOpen.value = value
+              },
+              toggleOpen() {
+                isOpen.value = !isOpen.value
+              },
+            }
+          },
+        })
+
+        assertDialog({ state: DialogState.InvisibleUnmounted })
+
+        // Open dialog
+        await click(document.getElementById('trigger'))
+
+        // Verify it is open
+        assertDialog({
+          state: DialogState.Visible,
+          attributes: { id: 'headlessui-dialog-1' },
+        })
+
+        // Verify that the input field is focused
+        assertActiveElement(document.getElementById('b'))
+
+        // Verify that we can tab around
+        await press(Keys.Tab)
+        assertActiveElement(document.getElementById('a'))
+
+        // Verify that we can tab around
+        await press(Keys.Tab)
+        assertActiveElement(document.getElementById('b'))
+
+        // Verify that we can tab around
+        await press(Keys.Tab)
+        assertActiveElement(document.getElementById('a'))
+      })
+    )
+  })
 })
 
 describe('Mouse interactions', () => {
@@ -950,31 +1012,13 @@ describe('Nesting', () => {
 
       return () => {
         let level = props.level ?? 1
-        return h(Dialog, { open: true, onClose: onClose }, () => [
+        return h(Dialog, { open: true, onClose }, () => [
           h(DialogOverlay),
           h('div', [
             h('p', `Level: ${level}`),
-            h(
-              'button',
-              {
-                onClick: () => (showChild.value = true),
-              },
-              `Open ${level + 1} a`
-            ),
-            h(
-              'button',
-              {
-                onClick: () => (showChild.value = true),
-              },
-              `Open ${level + 1} b`
-            ),
-            h(
-              'button',
-              {
-                onClick: () => (showChild.value = true),
-              },
-              `Open ${level + 1} c`
-            ),
+            h('button', { onClick: () => (showChild.value = true) }, `Open ${level + 1} a`),
+            h('button', { onClick: () => (showChild.value = true) }, `Open ${level + 1} b`),
+            h('button', { onClick: () => (showChild.value = true) }, `Open ${level + 1} c`),
           ]),
           showChild.value &&
             h(Nested, {