Skip to content

Commit

Permalink
Fix crash when using DisclosureButton inside of a DisclosurePanel
Browse files Browse the repository at this point in the history
… when the `Disclosure` is open by default (#3465)

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
<Disclosure defaultOpen>
  <DisclosureButton>Open</DisclosureButton>
  <DisclosurePanel>
    <DisclosureButton>Close</DisclosureButton>
  </DisclosurePanel>
<Disclosure>
```

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.
  • Loading branch information
RobinMalfait authored Sep 9, 2024
1 parent 07c9f1f commit ef78d58
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 19 deletions.
1 change: 1 addition & 0 deletions packages/@headlessui-react/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,49 @@ describe('Rendering', () => {
})
)

it('should behave as a close button when used inside of the Disclosure.Panel', async () => {
render(
<Disclosure>
<Disclosure.Button>Open</Disclosure.Button>
<Disclosure.Panel>
<Disclosure.Button>Close</Disclosure.Button>
</Disclosure.Panel>
</Disclosure>
)

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(
<Disclosure defaultOpen>
<Disclosure.Button>Open</Disclosure.Button>
<Disclosure.Panel>
<Disclosure.Button>Close</Disclosure.Button>
</Disclosure.Panel>
</Disclosure>
)

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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,10 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
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()

Expand Down
31 changes: 15 additions & 16 deletions packages/@headlessui-react/src/components/popover/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -535,24 +535,23 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
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 <Popover.Button /> but only 1 <Popover.Button /> is supported.'
)
}
if (state.buttons.current.length > 1) {
console.warn(
'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
)
}

button && dispatch({ type: ActionTypes.SetButton, button })
})
button && dispatch({ type: ActionTypes.SetButton, button })
})
)
let withinPanelButtonRef = useSyncRefs(internalButtonRef, ref)
let ownerDocument = useOwnerDocument(internalButtonRef)
Expand Down

0 comments on commit ef78d58

Please sign in to comment.