Ensure FocusTrap is only active when the given enabled value is `…

…true` (#2456)

* fix(tabs): wrong tab focus when Tab contains a Dialog

* refactor(focus-trap): rename variable and move logic

* test(tabs): improve test by asserting the active element

* ensure `FocusTrap` is not active when `enabled = false`

* fix: move the enabled check to unmounting

* refactor to `useOnUnmount` hook

This will allow us to make the code relatively similar between React and
Vue.

* update changelog

---------

Co-authored-by: Robin Malfait <malfait.robin@gmail.com>

packages/@headlessui-react/CHANGELOG.md

@@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - [internal] add demo mode to `Menu` and `Popover` components ([#2448](https://github.com/tailwindlabs/headlessui/pull/2448))
 
+### Fixed
+
+- Ensure `FocusTrap` is only active when the given `enabled` value is `true` ([#2456](https://github.com/tailwindlabs/headlessui/pull/2456))
+
 ## [1.7.14] - 2023-04-12
 
 ### Fixed

packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx

@@ -1,5 +1,4 @@
 import React, {
-  useEffect,
   useRef,
 
   // Types
@@ -25,6 +24,7 @@ import { microTask } from '../../utils/micro-task'
 import { useWatch } from '../../hooks/use-watch'
 import { useDisposables } from '../../hooks/use-disposables'
 import { onDocumentReady } from '../../utils/document-ready'
+import { useOnUnmount } from '../../hooks/use-on-unmount'
 
 type Containers =
   // Lazy resolved containers
@@ -93,6 +93,7 @@ function FocusTrapFn<TTag extends ElementType = typeof DEFAULT_FOCUS_TRAP_TAG>(
     { ownerDocument, container, initialFocus },
     Boolean(features & Features.InitialFocus)
   )
+
   useFocusLock(
     { ownerDocument, container, containers, previousActiveElement },
     Boolean(features & Features.FocusLock)
@@ -276,19 +277,11 @@ function useRestoreFocus({ ownerDocument }: { ownerDocument: Document | null },
   }, [enabled])
 
   // Restore the focus to the previous element when the component is unmounted
-  let trulyUnmounted = useRef(false)
-  useEffect(() => {
-    trulyUnmounted.current = false
-
-    return () => {
-      trulyUnmounted.current = true
-      microTask(() => {
-        if (!trulyUnmounted.current) return
+  useOnUnmount(() => {
+    if (!enabled) return
 
-        focusElement(getRestoreElement())
-      })
-    }
-  }, [])
+    focusElement(getRestoreElement())
+  })
 }
 
 function useInitialFocus(

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

@@ -2,6 +2,7 @@ import React, { createElement, useState } from 'react'
 import { render } from '@testing-library/react'
 
 import { Tab } from './tabs'
+import { Dialog } from '../dialog/dialog'
 import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs'
 import {
   assertTabs,
@@ -2881,6 +2882,69 @@ describe('Mouse interactions', () => {
   )
 })
 
+describe('Composition', () => {
+  it(
+    'should be possible to go to the next item containing a Dialog component',
+    suppressConsoleLogs(async () => {
+      render(
+        <>
+          <Tab.Group>
+            <Tab.List>
+              <Tab>Tab 1</Tab>
+              <Tab>Tab 2</Tab>
+              <Tab>Tab 3</Tab>
+            </Tab.List>
+
+            <Tab.Panels>
+              <Tab.Panel data-panel="0">Content 1</Tab.Panel>
+              <Tab.Panel data-panel="1">
+                <>
+                  <button>open</button>
+                  <Dialog open={false} onClose={console.log} />
+                </>
+              </Tab.Panel>
+              <Tab.Panel data-panel="2">Content 3</Tab.Panel>
+            </Tab.Panels>
+          </Tab.Group>
+        </>
+      )
+
+      assertActiveElement(document.body)
+
+      await press(Keys.Tab)
+      assertTabs({ active: 0 })
+
+      // Navigate to Dialog tab
+      await press(Keys.ArrowRight)
+      assertTabs({ active: 1 })
+
+      // Focus on to the Dialog panel
+      await press(Keys.Tab)
+      assertActiveElement(document.querySelector('[data-panel="1"]'))
+
+      // Focus on to the Dialog trigger button
+      await press(Keys.Tab)
+      assertActiveElement(getByText('open'))
+
+      // Focus back to the panel
+      await press(shift(Keys.Tab))
+      assertActiveElement(document.querySelector('[data-panel="1"]'))
+
+      // Focus back to tabs
+      await press(shift(Keys.Tab))
+      assertTabs({ active: 1 })
+
+      // Navigate to the next tab
+      await press(Keys.ArrowRight)
+      assertTabs({ active: 2 })
+
+      // Focus on to the content panel
+      await press(Keys.Tab)
+      assertActiveElement(document.querySelector('[data-panel="2"]'))
+    })
+  )
+})
+
 it(
   'should trigger the `onChange` when the tab changes',
   suppressConsoleLogs(async () => {

packages/@headlessui-react/src/hooks/use-on-unmount.ts

@@ -0,0 +1,18 @@
+import { useRef, useEffect } from 'react'
+import { microTask } from '../utils/micro-task'
+
+export function useOnUnmount(cb: () => void) {
+  let trulyUnmounted = useRef(false)
+  useEffect(() => {
+    trulyUnmounted.current = false
+
+    return () => {
+      trulyUnmounted.current = true
+      microTask(() => {
+        if (!trulyUnmounted.current) return
+
+        cb()
+      })
+    }
+  }, [])
+}

packages/@headlessui-vue/CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Fix memory leak in `Popover` component ([#2430](https://github.com/tailwindlabs/headlessui/pull/2430))
+- Ensure `FocusTrap` is only active when the given `enabled` value is `true` ([#2456](https://github.com/tailwindlabs/headlessui/pull/2456))
 
 ## [1.7.13] - 2023-04-12
 

packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts

@@ -284,6 +284,8 @@ function useRestoreFocus(
 
   // Restore the focus when we unmount the component
   onUnmounted(() => {
+    if (!enabled.value) return
+
     focusElement(getRestoreElement())
   })
 }

packages/@headlessui-vue/src/components/tabs/tabs.test.ts

@@ -1,6 +1,7 @@
 import { nextTick, ref } from 'vue'
 import { createRenderTemplate, render } from '../../test-utils/vue-testing-library'
 import { TabGroup, TabList, Tab, TabPanels, TabPanel } from './tabs'
+import { Dialog } from '../dialog/dialog'
 import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs'
 import {
   assertActiveElement,
@@ -20,7 +21,7 @@ beforeAll(() => {
 
 afterAll(() => jest.restoreAllMocks())
 
-const renderTemplate = createRenderTemplate({ TabGroup, TabList, Tab, TabPanels, TabPanel })
+const renderTemplate = createRenderTemplate({ TabGroup, TabList, Tab, TabPanels, TabPanel, Dialog })
 
 describe('safeguards', () => {
   it.each([
@@ -2716,3 +2717,69 @@ it('should trigger the `change` when the tab changes', async () => {
   expect(changes).toHaveBeenNthCalledWith(3, 1)
   expect(changes).toHaveBeenNthCalledWith(4, 0)
 })
+
+describe('Composition', () => {
+  it(
+    'should be possible to go to the next item containing a Dialog component',
+    suppressConsoleLogs(async () => {
+      renderTemplate({
+        template: `
+          <TabGroup>
+            <TabList>
+              <Tab>Tab 1</Tab>
+              <Tab>Tab 2</Tab>
+              <Tab>Tab 3</Tab>
+            </TabList>
+
+            <TabPanels>
+              <TabPanel data-panel="0">Content 1</TabPanel>
+              <TabPanel data-panel="1">
+                <button>open</button>
+                <Dialog :open="false" @close="noop" />
+              </TabPanel>
+              <TabPanel data-panel="2">Content 3</TabPanel>
+            </TabPanels>
+          </TabGroup>
+        `,
+        setup: () => ({
+          noop: console.log,
+        }),
+      })
+
+      await new Promise<void>(nextTick)
+
+      assertActiveElement(document.body)
+
+      await press(Keys.Tab)
+      assertTabs({ active: 0 })
+
+      // Navigate to Dialog tab
+      await press(Keys.ArrowRight)
+      assertTabs({ active: 1 })
+
+      // Focus on to the Dialog panel
+      await press(Keys.Tab)
+      assertActiveElement(document.querySelector('[data-panel="1"]'))
+
+      // Focus on to the Dialog trigger button
+      await press(Keys.Tab)
+      assertActiveElement(getByText('open'))
+
+      // Focus back to the panel
+      await press(shift(Keys.Tab))
+      assertActiveElement(document.querySelector('[data-panel="1"]'))
+
+      // Focus back to tabs
+      await press(shift(Keys.Tab))
+      assertTabs({ active: 1 })
+
+      // Navigate to the next tab
+      await press(Keys.ArrowRight)
+      assertTabs({ active: 2 })
+
+      // Focus on to the content panel
+      await press(Keys.Tab)
+      assertActiveElement(document.querySelector('[data-panel="2"]'))
+    })
+  )
+})