diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 89fb6bc26c..a8af42d98d 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -7,7 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -- Nothing yet! +### Fixed + +- Fix components not closing properly when using the `transition` prop ([#3448](https://github.com/tailwindlabs/headlessui/pull/3448)) ## [2.1.3] - 2024-08-23 diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx index 00fb4953ee..2eb66609a0 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx @@ -1,4 +1,4 @@ -import { render } from '@testing-library/react' +import { render, waitFor } from '@testing-library/react' import React, { Fragment, createElement, useEffect, useState } from 'react' import { ComboboxMode, @@ -42,7 +42,13 @@ import { } from '../../test-utils/interactions' import { mockingConsoleLogs, suppressConsoleLogs } from '../../test-utils/suppress-console-logs' import { Transition } from '../transition/transition' -import { Combobox } from './combobox' +import { + Combobox, + ComboboxButton, + ComboboxInput, + ComboboxOption, + ComboboxOptions, +} from './combobox' let NOOP = () => {} @@ -6060,3 +6066,39 @@ describe('Form compatibility', () => { ]) }) }) + +describe('transitions', () => { + it( + 'should be possible to close the Combobox when using the `transition` prop', + suppressConsoleLogs(async () => { + render( + + Toggle + + + Alice + Bob + Charlie + + + ) + + // Open the combobox + await click(getComboboxButton()) + + // Ensure the combobox is visible + assertCombobox({ state: ComboboxState.Visible }) + + // Close the combobox + await click(getComboboxButton()) + + // Wait for the transition to finish, and the combobox to close + await waitFor(() => { + assertComboboxList({ state: ComboboxState.InvisibleUnmounted }) + }) + + // Ensure the input got the restored focus + assertActiveElement(getComboboxInput()) + }) + ) +}) diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index 131284aa31..8e582a0349 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx @@ -1672,14 +1672,26 @@ function OptionsFn( } let [floatingRef, style] = useFloatingPanel(anchor) + + // To improve the correctness of transitions (timing related race conditions), + // we track the element locally to this component, instead of relying on the + // context value. This way, the component can re-render independently of the + // parent component when the `useTransition(…)` hook performs a state change. + let [localOptionsElement, setLocalOptionsElement] = useState(null) + let getFloatingPanelProps = useFloatingPanelProps() - let optionsRef = useSyncRefs(ref, anchor ? floatingRef : null, actions.setOptionsElement) + let optionsRef = useSyncRefs( + ref, + anchor ? floatingRef : null, + actions.setOptionsElement, + setLocalOptionsElement + ) let ownerDocument = useOwnerDocument(data.optionsElement) let usesOpenClosedState = useOpenClosed() let [visible, transitionData] = useTransition( transition, - data.optionsElement, + localOptionsElement, usesOpenClosedState !== null ? (usesOpenClosedState & State.Open) === State.Open : data.comboboxState === ComboboxState.Open diff --git a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx index 2c7aeb29df..d32844943c 100644 --- a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx +++ b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx @@ -1,18 +1,18 @@ -import { render } from '@testing-library/react' -import React, { createElement, Suspense, useEffect, useRef } from 'react' +import { render, waitFor } from '@testing-library/react' +import React, { Suspense, createElement, useEffect, useRef } from 'react' import { + DisclosureState, assertActiveElement, assertDisclosureButton, assertDisclosurePanel, - DisclosureState, getByText, getDisclosureButton, getDisclosurePanel, } from '../../test-utils/accessibility-assertions' -import { click, focus, Keys, MouseButton, press } from '../../test-utils/interactions' +import { Keys, MouseButton, click, focus, press } from '../../test-utils/interactions' import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs' import { Transition } from '../transition/transition' -import { Disclosure } from './disclosure' +import { Disclosure, DisclosureButton, DisclosurePanel } from './disclosure' jest.mock('../../hooks/use-id') @@ -985,3 +985,40 @@ describe('Mouse interactions', () => { }) ) }) + +describe('transitions', () => { + it( + 'should be possible to close the Disclosure when using the `transition` prop', + suppressConsoleLogs(async () => { + render( + + Toggle + Contents + + ) + + // Focus the button + await focus(getDisclosureButton()) + + // Ensure the button is focused + assertActiveElement(getDisclosureButton()) + + // Open the disclosure + await click(getDisclosureButton()) + + // Ensure the disclosure is visible + assertDisclosurePanel({ state: DisclosureState.Visible }) + + // Close the disclosure + await click(getDisclosureButton()) + + // Wait for the transition to finish, and the disclosure to close + await waitFor(() => { + assertDisclosurePanel({ state: DisclosureState.InvisibleUnmounted }) + }) + + // Ensure the button got the restored focus + assertActiveElement(getDisclosureButton()) + }) + ) +}) diff --git a/packages/@headlessui-react/src/components/disclosure/disclosure.tsx b/packages/@headlessui-react/src/components/disclosure/disclosure.tsx index 4a20bb2e5e..2f3e522627 100644 --- a/packages/@headlessui-react/src/components/disclosure/disclosure.tsx +++ b/packages/@headlessui-react/src/components/disclosure/disclosure.tsx @@ -11,6 +11,7 @@ import React, { useMemo, useReducer, useRef, + useState, type ContextType, type Dispatch, type ElementType, @@ -451,11 +452,18 @@ function PanelFn( let { close } = useDisclosureAPIContext('Disclosure.Panel') let mergeRefs = useMergeRefsFn() + // To improve the correctness of transitions (timing related race conditions), + // we track the element locally to this component, instead of relying on the + // context value. This way, the component can re-render independently of the + // parent component when the `useTransition(…)` hook performs a state change. + let [localPanelElement, setLocalPanelElement] = useState(null) + let panelRef = useSyncRefs( ref, useEvent((element) => { startTransition(() => dispatch({ type: ActionTypes.SetPanelElement, element })) - }) + }), + setLocalPanelElement ) useEffect(() => { @@ -468,7 +476,7 @@ function PanelFn( let usesOpenClosedState = useOpenClosed() let [visible, transitionData] = useTransition( transition, - state.panelElement, + localPanelElement, usesOpenClosedState !== null ? (usesOpenClosedState & State.Open) === State.Open : state.disclosureState === DisclosureStates.Open diff --git a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx index f603344b52..d75b01f5e3 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx @@ -1,4 +1,4 @@ -import { render } from '@testing-library/react' +import { render, waitFor } from '@testing-library/react' import React, { createElement, useEffect, useState } from 'react' import { ListboxMode, @@ -35,7 +35,7 @@ import { } from '../../test-utils/interactions' import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs' import { Transition } from '../transition/transition' -import { Listbox } from './listbox' +import { Listbox, ListboxButton, ListboxOption, ListboxOptions } from './listbox' jest.mock('../../hooks/use-id') @@ -4811,3 +4811,44 @@ describe('Form compatibility', () => { ]) }) }) + +describe('transitions', () => { + it( + 'should be possible to close the Listbox when using the `transition` prop', + suppressConsoleLogs(async () => { + render( + + Toggle + + Alice + Bob + Charlie + + + ) + + // Focus the button + await focus(getListboxButton()) + + // Ensure the button is focused + assertActiveElement(getListboxButton()) + + // Open the listbox + await click(getListboxButton()) + + // Ensure the listbox is visible + assertListbox({ state: ListboxState.Visible }) + + // Close the listbox + await click(getListboxButton()) + + // Wait for the transition to finish, and the listbox to close + await waitFor(() => { + assertListbox({ state: ListboxState.InvisibleUnmounted }) + }) + + // Ensure the button got the restored focus + assertActiveElement(getListboxButton()) + }) + ) +}) diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index e8c40e4365..e82cad3475 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.tsx @@ -12,6 +12,7 @@ import React, { useMemo, useReducer, useRef, + useState, type CSSProperties, type ElementType, type MutableRefObject, @@ -932,6 +933,12 @@ function OptionsFn( } = props let anchor = useResolvedAnchor(rawAnchor) + // To improve the correctness of transitions (timing related race conditions), + // we track the element locally to this component, instead of relying on the + // context value. This way, the component can re-render independently of the + // parent component when the `useTransition(…)` hook performs a state change. + let [localOptionsElement, setLocalOptionsElement] = useState(null) + // Always enable `portal` functionality, when `anchor` is enabled if (anchor) { portal = true @@ -945,7 +952,7 @@ function OptionsFn( let usesOpenClosedState = useOpenClosed() let [visible, transitionData] = useTransition( transition, - data.optionsElement, + localOptionsElement, usesOpenClosedState !== null ? (usesOpenClosedState & State.Open) === State.Open : data.listboxState === ListboxStates.Open @@ -1023,7 +1030,12 @@ function OptionsFn( let [floatingRef, style] = useFloatingPanel(anchorOptions) let getFloatingPanelProps = useFloatingPanelProps() - let optionsRef = useSyncRefs(ref, anchor ? floatingRef : null, actions.setOptionsElement) + let optionsRef = useSyncRefs( + ref, + anchor ? floatingRef : null, + actions.setOptionsElement, + setLocalOptionsElement + ) let searchDisposables = useDisposables() diff --git a/packages/@headlessui-react/src/components/menu/menu.test.tsx b/packages/@headlessui-react/src/components/menu/menu.test.tsx index 0285959de9..a57e748f88 100644 --- a/packages/@headlessui-react/src/components/menu/menu.test.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.test.tsx @@ -1,4 +1,4 @@ -import { render } from '@testing-library/react' +import { render, waitFor } from '@testing-library/react' import React, { createElement, useEffect } from 'react' import { MenuState, @@ -31,7 +31,7 @@ import { } from '../../test-utils/interactions' import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs' import { Transition } from '../transition/transition' -import { Menu } from './menu' +import { Menu, MenuButton, MenuItem, MenuItems } from './menu' jest.mock('../../hooks/use-id') @@ -3531,3 +3531,44 @@ describe('Mouse interactions', () => { }) ) }) + +describe('transitions', () => { + it( + 'should be possible to close the Menu when using the `transition` prop', + suppressConsoleLogs(async () => { + render( + + Toggle + + Alice + Bob + Charlie + + + ) + + // Focus the button + await focus(getMenuButton()) + + // Ensure the button is focused + assertActiveElement(getMenuButton()) + + // Open the menu + await click(getMenuButton()) + + // Ensure the menu is visible + assertMenu({ state: MenuState.Visible }) + + // Close the menu + await click(getMenuButton()) + + // Wait for the transition to finish, and the menu to close + await waitFor(() => { + assertMenu({ state: MenuState.InvisibleUnmounted }) + }) + + // Ensure the button got the restored focus + assertActiveElement(getMenuButton()) + }) + ) +}) diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx index 8da007529a..88b0074740 100644 --- a/packages/@headlessui-react/src/components/menu/menu.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.tsx @@ -12,6 +12,7 @@ import React, { useMemo, useReducer, useRef, + useState, type CSSProperties, type Dispatch, type ElementType, @@ -620,10 +621,18 @@ function ItemsFn( let [state, dispatch] = useMenuContext('Menu.Items') let [floatingRef, style] = useFloatingPanel(anchor) let getFloatingPanelProps = useFloatingPanelProps() + + // To improve the correctness of transitions (timing related race conditions), + // we track the element locally to this component, instead of relying on the + // context value. This way, the component can re-render independently of the + // parent component when the `useTransition(…)` hook performs a state change. + let [localItemsElement, setLocalItemsElement] = useState(null) + let itemsRef = useSyncRefs( ref, anchor ? floatingRef : null, - useEvent((element) => dispatch({ type: ActionTypes.SetItemsElement, element })) + useEvent((element) => dispatch({ type: ActionTypes.SetItemsElement, element })), + setLocalItemsElement ) let ownerDocument = useOwnerDocument(state.itemsElement) @@ -635,7 +644,7 @@ function ItemsFn( let usesOpenClosedState = useOpenClosed() let [visible, transitionData] = useTransition( transition, - state.itemsElement, + localItemsElement, usesOpenClosedState !== null ? (usesOpenClosedState & State.Open) === State.Open : state.menuState === MenuStates.Open diff --git a/packages/@headlessui-react/src/components/popover/popover.test.tsx b/packages/@headlessui-react/src/components/popover/popover.test.tsx index 85d00d0f71..86d9d44841 100644 --- a/packages/@headlessui-react/src/components/popover/popover.test.tsx +++ b/packages/@headlessui-react/src/components/popover/popover.test.tsx @@ -1,4 +1,4 @@ -import { render } from '@testing-library/react' +import { render, waitFor } from '@testing-library/react' import React, { Fragment, act, createElement, useEffect, useRef, useState } from 'react' import ReactDOM from 'react-dom' import { @@ -2844,3 +2844,40 @@ describe('Nested popovers', () => { }) ) }) + +describe('transitions', () => { + it( + 'should be possible to close the Popover when using the `transition` prop', + suppressConsoleLogs(async () => { + render( + + Toggle + Contents + + ) + + // Focus the button + await focus(getPopoverButton()) + + // Ensure the button is focused + assertActiveElement(getPopoverButton()) + + // Open the popover + await click(getPopoverButton()) + + // Ensure the popover is visible + assertPopoverPanel({ state: PopoverState.Visible }) + + // Close the popover + await click(getPopoverButton()) + + // Wait for the transition to finish, and the popover to close + await waitFor(() => { + assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }) + }) + + // Ensure the button got the restored focus + assertActiveElement(getPopoverButton()) + }) + ) +}) diff --git a/packages/@headlessui-react/src/components/popover/popover.tsx b/packages/@headlessui-react/src/components/popover/popover.tsx index 018368d167..b1efa7eaf6 100644 --- a/packages/@headlessui-react/src/components/popover/popover.tsx +++ b/packages/@headlessui-react/src/components/popover/popover.tsx @@ -763,13 +763,19 @@ function BackdropFn( ...theirProps } = props let [{ popoverState }, dispatch] = usePopoverContext('Popover.Backdrop') - let [backdropElement, setBackdropElement] = useState(null) - let backdropRef = useSyncRefs(ref, setBackdropElement) + + // To improve the correctness of transitions (timing related race conditions), + // we track the element locally to this component, instead of relying on the + // context value. This way, the component can re-render independently of the + // parent component when the `useTransition(…)` hook performs a state change. + let [localBackdropElement, setLocalBackdropElement] = useState(null) + + let backdropRef = useSyncRefs(ref, setLocalBackdropElement) let usesOpenClosedState = useOpenClosed() let [visible, transitionData] = useTransition( transition, - backdropElement, + localBackdropElement, usesOpenClosedState !== null ? (usesOpenClosedState & State.Open) === State.Open : popoverState === PopoverStates.Open @@ -865,11 +871,18 @@ function PanelFn( portal = true } + // To improve the correctness of transitions (timing related race conditions), + // we track the element locally to this component, instead of relying on the + // context value. This way, the component can re-render independently of the + // parent component when the `useTransition(…)` hook performs a state change. + let [localPanelElement, setLocalPanelElement] = useState(null) + let panelRef = useSyncRefs( internalPanelRef, ref, anchor ? floatingRef : null, - useEvent((panel) => dispatch({ type: ActionTypes.SetPanel, panel })) + useEvent((panel) => dispatch({ type: ActionTypes.SetPanel, panel })), + setLocalPanelElement ) let ownerDocument = useOwnerDocument(internalPanelRef) let mergeRefs = useMergeRefsFn() @@ -884,7 +897,7 @@ function PanelFn( let usesOpenClosedState = useOpenClosed() let [visible, transitionData] = useTransition( transition, - state.panel, + localPanelElement, usesOpenClosedState !== null ? (usesOpenClosedState & State.Open) === State.Open : state.popoverState === PopoverStates.Open @@ -1028,7 +1041,10 @@ function PanelFn( // Ignore sentinel buttons and items inside the panel for (let element of combined.slice()) { - if (element.dataset.headlessuiFocusGuard === 'true' || state.panel?.contains(element)) { + if ( + element.dataset.headlessuiFocusGuard === 'true' || + localPanelElement?.contains(element) + ) { let idx = combined.indexOf(element) if (idx !== -1) combined.splice(idx, 1) } diff --git a/packages/@headlessui-react/src/components/transition/transition.tsx b/packages/@headlessui-react/src/components/transition/transition.tsx index 31aaf07a4c..9f0922f0a3 100644 --- a/packages/@headlessui-react/src/components/transition/transition.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.tsx @@ -319,12 +319,12 @@ function TransitionChildFn(null) + let [localContainerElement, setLocalContainerElement] = useState(null) let container = useRef(null) let requiresRef = shouldForwardRef(props) let transitionRef = useSyncRefs( - ...(requiresRef ? [container, ref, setContainerElement] : ref === null ? [] : [ref]) + ...(requiresRef ? [container, ref, setLocalContainerElement] : ref === null ? [] : [ref]) ) let strategy = theirProps.unmount ?? true ? RenderStrategy.Unmount : RenderStrategy.Hidden @@ -438,7 +438,7 @@ function TransitionChildFn` is done, but there is still a // child `` busy, then `visible` would be `false`, while // `state` would still be `TreeStates.Visible`. - let [, transitionData] = useTransition(enabled, containerElement, show, { start, end }) + let [, transitionData] = useTransition(enabled, localContainerElement, show, { start, end }) let ourProps = compact({ ref: transitionRef,