From ef78d58a2e2aa1ef962f64ec6646968ce73eef2b Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Mon, 9 Sep 2024 21:13:45 +0200 Subject: [PATCH] Fix crash when using `DisclosureButton` inside of a `DisclosurePanel` when the `Disclosure` is open by default (#3465) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR fixes an issue where React hooks were called unconditionally> The `PopoverButton` and `DisclosureButton` act as a `CloseButton` when used inside of a panel. We conditionally handled the `ref` when it's inside a panel. To ensure that the callback is stable, the conditionally used function was wrapped in a `useEvent(…)` hook. This seemed to be ok (even though we break the rules of hooks) because a button can only be in a panel or not be in a panel, but it can't switch during the lifetime of the button. Aka, the rules of hooks are not broken because all code paths lead to the same hooks being called. ```ts Open Close ``` But... it can be called conditionally, because the way we know whether we are in a panel relies on a state value which comes from context and is populated by a `useEffect(…)` hook. The reason we didn't catch this in the `Disclosure` component, is because all the state is stable and known by the time the `DisclosurePanel` opens. But if you use the `defaultOpen` prop, the `DisclosurePanel` is already open and then the state is not ready yet (because we have to wait for the `useEffect(…)` hook). Long story short, moved the `isWithinPanel` check inside the `useEvent(…)` hook that holds the stable function which means that we don't call this hook unconditionally anymore. --- packages/@headlessui-react/CHANGELOG.md | 1 + .../components/disclosure/disclosure.test.tsx | 43 +++++++++++++++++++ .../src/components/disclosure/disclosure.tsx | 7 +-- .../src/components/popover/popover.tsx | 31 +++++++------ 4 files changed, 63 insertions(+), 19 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 1c6ba07..4988bc1 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/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 `ListboxOptions` being incorrectly marked as `inert` ([#3466](https://github.com/tailwindlabs/headlessui/pull/3466)) +- Fix crash when using `DisclosureButton` inside of a `DisclosurePanel` when the `Disclosure` is open by default ([#3465](https://github.com/tailwindlabs/headlessui/pull/3465)) ## [2.1.5] - 2024-09-04 diff --git a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx index d328449..6b66268 100644 --- a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx +++ b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx @@ -337,6 +337,49 @@ describe('Rendering', () => { }) ) + it('should behave as a close button when used inside of the Disclosure.Panel', async () => { + render( + + Open + + Close + + + ) + + assertDisclosureButton({ state: DisclosureState.InvisibleUnmounted }) + assertDisclosurePanel({ state: DisclosureState.InvisibleUnmounted }) + + await click(getByText('Open')) + + assertDisclosureButton({ state: DisclosureState.Visible }) + assertDisclosurePanel({ state: DisclosureState.Visible }) + + await click(getByText('Close')) + + assertDisclosureButton({ state: DisclosureState.InvisibleUnmounted }) + assertDisclosurePanel({ state: DisclosureState.InvisibleUnmounted }) + }) + + it('should behave as a close button when used inside of the Disclosure.Panel (defaultOpen)', async () => { + render( + + Open + + Close + + + ) + + assertDisclosureButton({ state: DisclosureState.Visible }) + assertDisclosurePanel({ state: DisclosureState.Visible }) + + await click(getByText('Close')) + + assertDisclosureButton({ state: DisclosureState.InvisibleUnmounted }) + assertDisclosurePanel({ state: DisclosureState.InvisibleUnmounted }) + }) + describe('`type` attribute', () => { it('should set the `type` to "button" by default', async () => { render( diff --git a/packages/@headlessui-react/src/components/disclosure/disclosure.tsx b/packages/@headlessui-react/src/components/disclosure/disclosure.tsx index 2f3e522..d7a5dd1 100644 --- a/packages/@headlessui-react/src/components/disclosure/disclosure.tsx +++ b/packages/@headlessui-react/src/components/disclosure/disclosure.tsx @@ -299,9 +299,10 @@ function ButtonFn( let buttonRef = useSyncRefs( internalButtonRef, ref, - !isWithinPanel - ? useEvent((element) => dispatch({ type: ActionTypes.SetButtonElement, element })) - : null + useEvent((element) => { + if (isWithinPanel) return + return dispatch({ type: ActionTypes.SetButtonElement, element }) + }) ) let mergeRefs = useMergeRefsFn() diff --git a/packages/@headlessui-react/src/components/popover/popover.tsx b/packages/@headlessui-react/src/components/popover/popover.tsx index b1efa7e..49a02f7 100644 --- a/packages/@headlessui-react/src/components/popover/popover.tsx +++ b/packages/@headlessui-react/src/components/popover/popover.tsx @@ -535,24 +535,23 @@ function ButtonFn( internalButtonRef, ref, useFloatingReference(), - isWithinPanel - ? null - : useEvent((button) => { - if (button) { - state.buttons.current.push(uniqueIdentifier) - } else { - let idx = state.buttons.current.indexOf(uniqueIdentifier) - if (idx !== -1) state.buttons.current.splice(idx, 1) - } + useEvent((button) => { + if (isWithinPanel) return + if (button) { + state.buttons.current.push(uniqueIdentifier) + } else { + let idx = state.buttons.current.indexOf(uniqueIdentifier) + if (idx !== -1) state.buttons.current.splice(idx, 1) + } - if (state.buttons.current.length > 1) { - console.warn( - 'You are already using a but only 1 is supported.' - ) - } + if (state.buttons.current.length > 1) { + console.warn( + 'You are already using a but only 1 is supported.' + ) + } - button && dispatch({ type: ActionTypes.SetButton, button }) - }) + button && dispatch({ type: ActionTypes.SetButton, button }) + }) ) let withinPanelButtonRef = useSyncRefs(internalButtonRef, ref) let ownerDocument = useOwnerDocument(internalButtonRef)