From b6708960b86b18f86821e19d4a35375786d72103 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Thu, 12 Sep 2024 12:35:03 +0200 Subject: [PATCH] Fix crash when using `as={Fragment}` on `MenuButton`, `ListboxButton`, `DisclosureButton` or `Button` components (#3478) This PR fixes an issue where a maximum update depth exceeded error was thrown when using `as={Fragment}` on button related components. The issue here is that the `ref` on a element would re-fire every render _if_ the a function was used _and_ the function is a new function (aka not a stable function). This resulted in the `ref` being called with the DOM element, then `null`, then the DOM element, then `null`, and so on. To solve this, we have to make sure that the `ref` is always a stable reference. Fixes: #3476 Fixes: #3439 --- packages/@headlessui-react/CHANGELOG.md | 4 ++- .../src/components/button/button.test.tsx | 12 +++++++- .../src/components/button/button.tsx | 3 ++ .../src/components/combobox/combobox.test.tsx | 25 +++++++++++++++++ .../src/components/combobox/combobox.tsx | 3 ++ .../components/disclosure/disclosure.test.tsx | 24 +++++++++++++++- .../src/components/listbox/listbox.test.tsx | 28 ++++++++++++++++++- .../src/components/listbox/listbox.tsx | 3 ++ .../src/components/menu/menu.test.tsx | 28 ++++++++++++++++++- .../src/components/menu/menu.tsx | 3 ++ 10 files changed, 128 insertions(+), 5 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 05e75b808e..dc7c8224cf 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 crash when using `as={Fragment}` on `MenuButton`, `ListboxButton`, `DisclosureButton` or `Button` components ([#3478](https://github.com/tailwindlabs/headlessui/pull/3478)) ## [2.1.7] - 2024-09-11 diff --git a/packages/@headlessui-react/src/components/button/button.test.tsx b/packages/@headlessui-react/src/components/button/button.test.tsx index c7d6c9676c..c421fb14dd 100644 --- a/packages/@headlessui-react/src/components/button/button.test.tsx +++ b/packages/@headlessui-react/src/components/button/button.test.tsx @@ -1,5 +1,5 @@ import { render, screen } from '@testing-library/react' -import React from 'react' +import React, { Fragment } from 'react' import { Button } from './button' describe('Rendering', () => { @@ -35,5 +35,15 @@ describe('Rendering', () => { expect(screen.getByRole('button')).toHaveAttribute('data-autofocus') }) + + it('should be possible to render a Button using as={Fragment}', async () => { + render( + + + ) + + expect(screen.getByRole('button')).toHaveAttribute('type') + }) }) }) diff --git a/packages/@headlessui-react/src/components/button/button.tsx b/packages/@headlessui-react/src/components/button/button.tsx index ba5cdb9837..96310a8ff0 100644 --- a/packages/@headlessui-react/src/components/button/button.tsx +++ b/packages/@headlessui-react/src/components/button/button.tsx @@ -10,6 +10,7 @@ import { forwardRefWithAs, mergeProps, render, + useMergeRefsFn, type HasDisplayName, type RefProp, } from '../../utils/render' @@ -41,6 +42,7 @@ function ButtonFn( ref: Ref ) { let providedDisabled = useDisabled() + let mergeRefs = useMergeRefsFn() let { disabled = providedDisabled || false, autoFocus = false, ...theirProps } = props let { isFocusVisible: focus, focusProps } = useFocusRing({ autoFocus }) @@ -64,6 +66,7 @@ function ButtonFn( }, [disabled, hover, focus, active, autoFocus]) return render({ + mergeRefs, ourProps, theirProps, slot, diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx index 2eb66609a0..30ee990374 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx @@ -1211,6 +1211,31 @@ describe('Rendering', () => { expect(getComboboxButton()).not.toHaveAttribute('type') }) }) + + it( + 'should be possible to render a ComboboxButton using as={Fragment}', + suppressConsoleLogs(async () => { + render( + + + + + + + Option A + Option B + Option C + + + ) + + assertComboboxButton({ state: ComboboxState.InvisibleUnmounted }) + + await click(getComboboxButton()) + + assertComboboxButton({ state: ComboboxState.Visible }) + }) + ) }) describe('Combobox.Options', () => { diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index 8e582a0349..6161293008 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx @@ -70,6 +70,7 @@ import { forwardRefWithAs, mergeProps, render, + useMergeRefsFn, type HasDisplayName, type PropsForFeatures, type RefProp, @@ -1495,6 +1496,7 @@ function ButtonFn( let data = useData('Combobox.Button') let actions = useActions('Combobox.Button') let buttonRef = useSyncRefs(ref, actions.setButtonElement) + let mergeRefs = useMergeRefsFn() let internalId = useId() let { @@ -1616,6 +1618,7 @@ function ButtonFn( ) return render({ + mergeRefs, ourProps, theirProps, slot, diff --git a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx index 6b6626878d..635c87dea5 100644 --- a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx +++ b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx @@ -1,5 +1,5 @@ import { render, waitFor } from '@testing-library/react' -import React, { Suspense, createElement, useEffect, useRef } from 'react' +import React, { Fragment, Suspense, createElement, useEffect, useRef } from 'react' import { DisclosureState, assertActiveElement, @@ -439,6 +439,28 @@ describe('Rendering', () => { expect(getDisclosureButton()).not.toHaveAttribute('type') }) }) + + it( + 'should be possible to render a DisclosureButton using as={Fragment}', + suppressConsoleLogs(async () => { + render( + + + + + Contents + + ) + + assertDisclosureButton({ state: DisclosureState.InvisibleUnmounted }) + assertDisclosurePanel({ state: DisclosureState.InvisibleUnmounted }) + + await click(getDisclosureButton()) + + assertDisclosureButton({ state: DisclosureState.Visible }) + assertDisclosurePanel({ state: DisclosureState.Visible }) + }) + ) }) describe('Disclosure.Panel', () => { diff --git a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx index d75b01f5e3..f606fc9aa8 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx @@ -1,5 +1,5 @@ import { render, waitFor } from '@testing-library/react' -import React, { createElement, useEffect, useState } from 'react' +import React, { Fragment, createElement, useEffect, useState } from 'react' import { ListboxMode, ListboxState, @@ -760,6 +760,32 @@ describe('Rendering', () => { expect(getListboxButton()).not.toHaveAttribute('type') }) }) + + it( + 'should be possible to render a ListboxButton using as={Fragment}', + suppressConsoleLogs(async () => { + render( + + + + + + Option A + Option B + Option C + + + ) + + assertListboxButton({ state: ListboxState.InvisibleUnmounted }) + assertListbox({ state: ListboxState.InvisibleUnmounted }) + + await click(getListboxButton()) + + assertListboxButton({ state: ListboxState.Visible }) + assertListbox({ state: ListboxState.Visible }) + }) + ) }) describe('Listbox.Options', () => { diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index b4b9a724ee..e9a8150a57 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.tsx @@ -75,6 +75,7 @@ import { forwardRefWithAs, mergeProps, render, + useMergeRefsFn, type HasDisplayName, type PropsForFeatures, type RefProp, @@ -785,6 +786,7 @@ function ButtonFn( autoFocus = false, ...theirProps } = props + let mergeRefs = useMergeRefsFn() let buttonRef = useSyncRefs(ref, useFloatingReference(), actions.setButtonElement) let getFloatingReferenceProps = useFloatingReferenceProps() @@ -880,6 +882,7 @@ function ButtonFn( ) return render({ + mergeRefs, ourProps, theirProps, slot, diff --git a/packages/@headlessui-react/src/components/menu/menu.test.tsx b/packages/@headlessui-react/src/components/menu/menu.test.tsx index a57e748f88..a3ed4d20f5 100644 --- a/packages/@headlessui-react/src/components/menu/menu.test.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.test.tsx @@ -1,5 +1,5 @@ import { render, waitFor } from '@testing-library/react' -import React, { createElement, useEffect } from 'react' +import React, { Fragment, createElement, useEffect } from 'react' import { MenuState, assertActiveElement, @@ -306,6 +306,32 @@ describe('Rendering', () => { expect(getMenuButton()).not.toHaveAttribute('type') }) }) + + it( + 'should be possible to render a MenuButton using as={Fragment}', + suppressConsoleLogs(async () => { + render( + + + + + + Item A + Item B + Item C + + + ) + + assertMenuButton({ state: MenuState.InvisibleUnmounted }) + assertMenu({ state: MenuState.InvisibleUnmounted }) + + await click(getMenuButton()) + + assertMenuButton({ state: MenuState.Visible }) + assertMenu({ state: MenuState.Visible }) + }) + ) }) describe('Menu.Items', () => { diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx index 88b0074740..57a823eb09 100644 --- a/packages/@headlessui-react/src/components/menu/menu.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.tsx @@ -68,6 +68,7 @@ import { forwardRefWithAs, mergeProps, render, + useMergeRefsFn, type HasDisplayName, type RefProp, } from '../../utils/render' @@ -483,6 +484,7 @@ function ButtonFn( } = props let [state, dispatch] = useMenuContext('Menu.Button') let getFloatingReferenceProps = useFloatingReferenceProps() + let mergeRefs = useMergeRefsFn() let buttonRef = useSyncRefs( ref, useFloatingReference(), @@ -570,6 +572,7 @@ function ButtonFn( ) return render({ + mergeRefs, ourProps, theirProps, slot,