diff --git a/packages/react/src/ActionList/ActionList.test.tsx b/packages/react/src/ActionList/ActionList.test.tsx index e520c6ad03f..9c9af6074a4 100644 --- a/packages/react/src/ActionList/ActionList.test.tsx +++ b/packages/react/src/ActionList/ActionList.test.tsx @@ -2,6 +2,7 @@ import {describe, it, expect, vi} from 'vitest' import {render as HTMLRender} from '@testing-library/react' import userEvent from '@testing-library/user-event' import {ActionList} from '.' +import {createRef} from 'react' import {ActionListContainerContext} from './ActionListContainerContext' import {implementsClassName} from '../utils/testing' import classes from './ActionList.module.css' @@ -606,3 +607,33 @@ describe('ActionList with role="tree"', () => { expect(container.querySelector('[data-component="ActionList"]')).not.toHaveAttribute('data-item-gap') }) }) + +describe('ActionList forwarded ref (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => { + it('forwards a ref object to the element', () => { + const ref = createRef() + HTMLRender( + + + Item + + , + ) + expect(ref.current).toBeInstanceOf(HTMLElement) + }) + + it('calls a callback ref with the element', () => { + const refCallback = vi.fn() + HTMLRender( + + + Item + + , + ) + expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLElement)).toBe(true) + }) + }) + } +}) diff --git a/packages/react/src/ActionList/List.tsx b/packages/react/src/ActionList/List.tsx index 1ab0a116f0b..d00ba6a95be 100644 --- a/packages/react/src/ActionList/List.tsx +++ b/packages/react/src/ActionList/List.tsx @@ -1,11 +1,11 @@ -import React, {type JSX} from 'react' +import React, {useRef, type JSX} from 'react' import {fixedForwardRef} from '../utils/modern-polymorphic' import {ActionListContainerContext} from './ActionListContainerContext' import {useSlots} from '../hooks/useSlots' import {Heading} from './Heading' import {useId} from '../hooks/useId' import {ListContext, type ActionListProps} from './shared' -import {useProvidedRefOrCreate} from '../hooks' +import {useMergedRefs, useProvidedRefOrCreate} from '../hooks' import {FocusKeys, useFocusZone} from '../hooks/useFocusZone' import {clsx} from 'clsx' import classes from './ActionList.module.css' @@ -43,7 +43,14 @@ const UnwrappedList = ( const ariaLabelledBy = slots.heading ? (slots.heading.props.id ?? headingId) : listLabelledBy const listRole = role || listRoleFromContainer - const listRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + const listRef = useRef(null) + const mergedRef = useMergedRefs(listRef, forwardedRef) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readRef` with `listRef` and `appliedRef` with `mergedRef`. + const providedOrCreatedRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const readRef = mergedRefEnabled ? listRef : providedOrCreatedRef + const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef const itemGapEnabled = useFeatureFlag('primer_react_action_list_item_gap') && container === 'NavList' let enableFocusZone = false @@ -52,7 +59,7 @@ const UnwrappedList = ( useFocusZone({ disabled: !enableFocusZone, - containerRef: listRef, + containerRef: readRef, bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd | FocusKeys.PageUpDown, focusOutBehavior: listRole === 'menu' || container === 'SelectPanel' || container === 'FilteredActionList' ? 'wrap' : undefined, @@ -84,7 +91,7 @@ const UnwrappedList = ( // Two querySelector calls after render is trivially cheap compared to what the browser // was doing on every DOM mutation with `:has()`. useIsomorphicLayoutEffect(() => { - const list = listRef.current + const list = readRef.current if (!list) return const hasMixed = list.querySelector('[data-has-description="true"]') !== null && @@ -105,7 +112,7 @@ const UnwrappedList = ( className={clsx(classes.ActionList, className)} role={listRole} aria-labelledby={ariaLabelledBy} - ref={listRef} + ref={appliedRef} data-component="ActionList" data-dividers={showDividers} data-variant={variant} diff --git a/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx b/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx index d5e6b4179bf..557cc896ee6 100644 --- a/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx +++ b/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx @@ -765,3 +765,49 @@ describe('AnchoredOverlay anchor element replacement', () => { expect(newAnchor.style.getPropertyValue('anchor-name')).toBe(anchorName) }) }) + +describe('AnchoredOverlay overlay forwarded ref (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => { + it('forwards a ref object to the overlay', () => { + const ref = createRef() + render( + + + {}} + onOpen={() => {}} + renderAnchor={props => } + overlayProps={{ref}} + > + + + + , + ) + expect(ref.current).toBeInstanceOf(HTMLDivElement) + }) + + it('calls a callback ref with the overlay', () => { + const refCallback = vi.fn() + render( + + + {}} + onOpen={() => {}} + renderAnchor={props => } + overlayProps={{ref: refCallback}} + > + + + + , + ) + expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLDivElement)).toBe(true) + }) + }) + } +}) diff --git a/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx b/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx index a0c9d6a1da8..bc373025e56 100644 --- a/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx +++ b/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx @@ -6,7 +6,7 @@ import type {FocusTrapHookSettings} from '../hooks/useFocusTrap' import {useFocusTrap} from '../hooks/useFocusTrap' import type {FocusZoneHookSettings} from '../hooks/useFocusZone' import {useFocusZone} from '../hooks/useFocusZone' -import {useAnchoredPosition, useProvidedRefOrCreate, useRenderForcingRef} from '../hooks' +import {useAnchoredPosition, useMergedRefs, useProvidedRefOrCreate, useRenderForcingRef} from '../hooks' import {useId} from '../hooks/useId' import type {AnchorPosition, PositionSettings} from '@primer/behaviors' import {type ResponsiveValue} from '../hooks/useResponsiveValue' @@ -207,7 +207,9 @@ export const AnchoredOverlay: React.FC() + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') const [overlayElement, setOverlayElement] = useState(null) + const mergedOverlayRef = useMergedRefs(updateOverlayRef, useMergedRefs(overlayProps?.ref, setOverlayElement)) const anchorId = useId(externalAnchorId) const onClickOutside = useCallback(() => onClose?.('click-outside'), [onClose]) @@ -436,13 +438,17 @@ export const AnchoredOverlay: React.FC { - if (overlayProps?.ref) { - assignRef(overlayProps.ref, node) - } - updateOverlayRef(node) - setOverlayElement(node) - }} + ref={ + mergedRefEnabled + ? mergedOverlayRef + : node => { + setOverlayElement(node) + if (overlayProps?.ref) { + assignRef(overlayProps.ref, node) + } + updateOverlayRef(node) + } + } data-anchor-position={cssAnchorPositioning} data-side={cssAnchorPositioning ? side : position?.anchorSide} > diff --git a/packages/react/src/Autocomplete/AutocompleteInput.tsx b/packages/react/src/Autocomplete/AutocompleteInput.tsx index f7622d7013a..d512a53e3cf 100644 --- a/packages/react/src/Autocomplete/AutocompleteInput.tsx +++ b/packages/react/src/Autocomplete/AutocompleteInput.tsx @@ -56,13 +56,19 @@ const AutocompleteInput = React.forwardRef( const handleInputBlur: FocusEventHandler = useCallback( event => { - onBlur && onBlur(event) + onBlur?.(event) - // HACK: wait a tick and check the focused element before hiding the autocomplete menu - // this prevents the menu from hiding when the user is clicking an option in the Autoselect.Menu, - // but still hides the menu when the user blurs the input by tabbing out or clicking somewhere else on the page + // HACK: wait a tick before hiding the menu so click interactions can complete. + // Use the blur event's relatedTarget to determine whether focus is moving into the + // autocomplete menu; if not, hide the menu when focus leaves the input. safeSetTimeout(() => { - if (document.activeElement !== inputRef.current) { + const nextFocusedElement = event.relatedTarget as Node | null + const menuElement = document.getElementById(`${id}-listbox`) + + if ( + !nextFocusedElement || + (nextFocusedElement !== menuElement && !menuElement?.contains(nextFocusedElement)) + ) { setShowMenu(false) // Reset the input's value to the text the user actually typed rather than leaving the @@ -75,11 +81,11 @@ const AutocompleteInput = React.forwardRef( } }, 0) }, - [onBlur, setShowMenu, inputRef, safeSetTimeout, autocompleteSuggestion, inputValue], + [onBlur, setShowMenu, inputRef, safeSetTimeout, autocompleteSuggestion, inputValue, id], ) const handleInputChange: ChangeEventHandler = event => { - onChange && onChange(event) + onChange?.(event) setInputValue(event.currentTarget.value) if (!showMenu) { setShowMenu(true) @@ -88,7 +94,7 @@ const AutocompleteInput = React.forwardRef( const handleInputKeyDown: KeyboardEventHandler = useCallback( event => { - onKeyDown && onKeyDown(event) + onKeyDown?.(event) if (event.key === 'Backspace') { setHighlightRemainingText(false) @@ -107,7 +113,7 @@ const AutocompleteInput = React.forwardRef( const handleInputKeyUp: KeyboardEventHandler = useCallback( event => { - onKeyUp && onKeyUp(event) + onKeyUp?.(event) if (event.key === 'Backspace') { setHighlightRemainingText(true) @@ -118,12 +124,11 @@ const AutocompleteInput = React.forwardRef( const onInputKeyPress: KeyboardEventHandler = useCallback( event => { - onKeyPress && onKeyPress(event) + onKeyPress?.(event) if (showMenu && event.key === 'Enter' && activeDescendantRef.current) { event.preventDefault() event.nativeEvent.stopImmediatePropagation() - // Forward Enter key press to active descendant so that item gets activated const activeDescendantEvent = new KeyboardEvent(event.type, event.nativeEvent) activeDescendantRef.current.dispatchEvent(activeDescendantEvent) } @@ -136,17 +141,10 @@ const AutocompleteInput = React.forwardRef( return } - // resets input value to being empty after a selection has been made if (!autocompleteSuggestion) { inputRef.current.value = inputValue } - // TODO: fix bug where this function prevents `onChange` from being triggered if the highlighted item text - // is the same as what I'm typing - // e.g.: typing 'tw' highlights 'two', but when I 'two', the text input change does not get triggered - // Only apply the inline autocomplete suggestion while the input is focused. Without this guard, - // the suggestion can be re-applied to the DOM after the input is blurred, which would restore - // the full suggestion the user was editing away from. See https://github.com/primer/react/issues/4275 const isInputFocused = document.activeElement === inputRef.current if ( @@ -163,7 +161,6 @@ const AutocompleteInput = React.forwardRef( } } - // calling this useEffect when `highlightRemainingText` changes breaks backspace functionality // eslint-disable-next-line react-hooks/exhaustive-deps }, [autocompleteSuggestion, inputValue, inputRef, isMenuDirectlyActivated]) diff --git a/packages/react/src/ButtonGroup/ButtonGroup.test.tsx b/packages/react/src/ButtonGroup/ButtonGroup.test.tsx index fb8fc7e0039..64d935ed1bd 100644 --- a/packages/react/src/ButtonGroup/ButtonGroup.test.tsx +++ b/packages/react/src/ButtonGroup/ButtonGroup.test.tsx @@ -1,6 +1,8 @@ import {render, screen} from '@testing-library/react' import ButtonGroup from './ButtonGroup' -import {describe, expect, it} from 'vitest' +import {createRef} from 'react' +import {FeatureFlags} from '../FeatureFlags' +import {describe, expect, it, vi} from 'vitest' import {implementsClassName} from '../utils/testing' import classes from './ButtonGroup.module.css' @@ -22,3 +24,33 @@ describe('ButtonGroup', () => { expect(screen.getByRole('toolbar')).toBeInTheDocument() }) }) + +describe('ButtonGroup forwarded ref (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => { + it('forwards a ref object to the element', () => { + const ref = createRef() + render( + + + + + , + ) + expect(ref.current).toBeInstanceOf(HTMLDivElement) + }) + + it('calls a callback ref with the element', () => { + const refCallback = vi.fn() + render( + + + + + , + ) + expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLDivElement)).toBe(true) + }) + }) + } +}) diff --git a/packages/react/src/ButtonGroup/ButtonGroup.tsx b/packages/react/src/ButtonGroup/ButtonGroup.tsx index 7fa12fad872..ecba2d33604 100644 --- a/packages/react/src/ButtonGroup/ButtonGroup.tsx +++ b/packages/react/src/ButtonGroup/ButtonGroup.tsx @@ -1,8 +1,9 @@ -import React, {type PropsWithChildren} from 'react' +import React, {useRef, type PropsWithChildren} from 'react' import classes from './ButtonGroup.module.css' import {clsx} from 'clsx' import {FocusKeys, useFocusZone} from '../hooks/useFocusZone' -import {useProvidedRefOrCreate} from '../hooks' +import {useMergedRefs, useProvidedRefOrCreate} from '../hooks' +import {useFeatureFlag} from '../FeatureFlags' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' export type ButtonGroupProps = PropsWithChildren<{ @@ -16,15 +17,22 @@ const ButtonGroup = React.forwardRef(function ButtonGroup( {as: BaseComponent = 'div', children, className, role, ...rest}, forwardRef, ) { + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + const buttonRef = useRef(null) + const mergedRef = useMergedRefs(buttonRef, forwardRef) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readRef` with `buttonRef` and `appliedRef` with `mergedRef`. + const providedOrCreatedRef = useProvidedRefOrCreate(forwardRef as React.RefObject) + const readRef = mergedRefEnabled ? buttonRef : providedOrCreatedRef + const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef const buttons = React.Children.map(children, (child, index) => (
{child}
)) - const buttonRef = useProvidedRefOrCreate(forwardRef as React.RefObject) useFocusZone({ - containerRef: buttonRef, + containerRef: readRef, disabled: role !== 'toolbar', bindKeys: FocusKeys.ArrowHorizontal, focusOutBehavior: 'wrap', @@ -32,8 +40,8 @@ const ButtonGroup = React.forwardRef(function ButtonGroup( return ( { expect(checkbox).toHaveAttribute('aria-required', 'true') }) }) + +describe('Checkbox forwarded ref (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => { + it('forwards a ref object to the element', () => { + const ref = createRef() + render( + + + , + ) + expect(ref.current).toBeInstanceOf(HTMLInputElement) + }) + + it('calls a callback ref with the element', () => { + const refCallback = vi.fn() + render( + + + , + ) + expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLInputElement)).toBe(true) + }) + }) + } +}) diff --git a/packages/react/src/Checkbox/Checkbox.tsx b/packages/react/src/Checkbox/Checkbox.tsx index 6e54d07cfdd..047a0660c02 100644 --- a/packages/react/src/Checkbox/Checkbox.tsx +++ b/packages/react/src/Checkbox/Checkbox.tsx @@ -1,6 +1,14 @@ import {clsx} from 'clsx' -import {useProvidedRefOrCreate} from '../hooks' -import React, {useContext, useEffect, type ChangeEventHandler, type InputHTMLAttributes, type ReactElement} from 'react' +import {useMergedRefs, useProvidedRefOrCreate} from '../hooks' +import {useFeatureFlag} from '../FeatureFlags' +import React, { + useContext, + useEffect, + useRef, + type ChangeEventHandler, + type InputHTMLAttributes, + type ReactElement, +} from 'react' import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' import type {FormValidationStatus} from '../utils/types/FormValidationStatus' import {CheckboxGroupContext} from '../CheckboxGroup/CheckboxGroupContext' @@ -58,21 +66,28 @@ const Checkbox = React.forwardRef( ref, // eslint-disable-next-line @typescript-eslint/no-explicit-any ): ReactElement => { - const checkboxRef = useProvidedRefOrCreate(ref as React.RefObject) + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + const checkboxRef = useRef(null) + const mergedRef = useMergedRefs(checkboxRef, ref) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readRef` with `checkboxRef` and `appliedRef` with `mergedRef`. + const providedOrCreatedRef = useProvidedRefOrCreate(ref as React.RefObject) + const readRef = mergedRefEnabled ? checkboxRef : providedOrCreatedRef + const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef const checkboxGroupContext = useContext(CheckboxGroupContext) const handleOnChange: ChangeEventHandler = e => { checkboxGroupContext.onChange && checkboxGroupContext.onChange(e) onChange && onChange(e) - if (indeterminate && checkboxRef.current) { - checkboxRef.current.indeterminate = true - checkboxRef.current.setAttribute('aria-checked', 'mixed') + if (indeterminate && readRef.current) { + readRef.current.indeterminate = true + readRef.current.setAttribute('aria-checked', 'mixed') } } const inputProps = { type: 'checkbox', disabled, - ref: checkboxRef, + ref: appliedRef, checked: indeterminate ? false : checked, defaultChecked, required, @@ -85,13 +100,13 @@ const Checkbox = React.forwardRef( } useLayoutEffect(() => { - if (checkboxRef.current) { - checkboxRef.current.indeterminate = indeterminate || false + if (readRef.current) { + readRef.current.indeterminate = indeterminate || false } - }, [indeterminate, checked, checkboxRef]) + }, [indeterminate, checked, readRef]) useEffect(() => { - const {current: checkbox} = checkboxRef + const {current: checkbox} = readRef if (!checkbox) { return } diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx index 0cdbac53645..21f1fc2447c 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/packages/react/src/Dialog/Dialog.test.tsx @@ -3,6 +3,7 @@ import {render, fireEvent, waitFor} from '@testing-library/react' import {describe, expect, it, vi} from 'vitest' import userEvent from '@testing-library/user-event' import {Dialog} from './Dialog' +import {FeatureFlags} from '../FeatureFlags' import {Button} from '../Button' import {implementsClassName} from '../utils/testing' import classes from './Dialog.module.css' @@ -566,3 +567,21 @@ describe('Footer button loading states', () => { }) }) }) + +describe('Dialog auto-focus button forwarded ref (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + it(`focuses the auto-focus footer button via the gated ref with the flag ${enabled ? 'enabled' : 'disabled'}`, async () => { + const {getByRole} = render( + + {}} + footerButtons={[{buttonType: 'primary', content: 'Footer button', autoFocus: true}]} + > + Body + + , + ) + await waitFor(() => expect(getByRole('button', {name: 'Footer button'})).toHaveFocus()) + }) + } +}) diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index de9cc4f8794..909bad31424 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -2,6 +2,7 @@ import React, {useCallback, useEffect, useRef, useState, type CSSProperties, typ import type {ButtonProps} from '../Button' import {Button, IconButton} from '../Button' import {useMergedRefs, useOnEscapePress, useProvidedRefOrCreate} from '../hooks' +import {useFeatureFlag} from '../FeatureFlags' import {useFocusTrap} from '../hooks/useFocusTrap' import {XIcon} from '@primer/octicons-react' import {useFocusZone} from '../hooks/useFocusZone' @@ -484,18 +485,26 @@ const Footer = React.forwardRef(function Foot Footer.displayName = 'Dialog.Footer' const Buttons: React.FC> = ({buttons}) => { - const autoFocusRef = useProvidedRefOrCreate(buttons.find(button => button.autoFocus)?.ref) + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + const providedButtonRef = buttons.find(button => button.autoFocus)?.ref + const autoFocusRef = useRef(null) + const mergedRef = useMergedRefs(autoFocusRef, providedButtonRef) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readRef` with `autoFocusRef` and `appliedRef` with `mergedRef`. + const providedOrCreatedRef = useProvidedRefOrCreate(providedButtonRef) + const readRef = mergedRefEnabled ? autoFocusRef : providedOrCreatedRef + const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef let autoFocusCount = 0 const [hasRendered, setHasRendered] = useState(0) useEffect(() => { // hack to work around dialogs originating from other focus traps. if (hasRendered === 1) { - autoFocusRef.current?.focus() + readRef.current?.focus() } else { // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state setHasRendered(hasRendered + 1) } - }, [autoFocusRef, hasRendered]) + }, [readRef, hasRendered]) return ( <> @@ -509,7 +518,7 @@ const Buttons: React.FC> // 'normal' value is equivalent to 'default', this is used for backwards compatibility variant={buttonType === 'normal' ? 'default' : buttonType} // @ts-expect-error it needs a non nullable ref - ref={autoFocus && autoFocusCount === 0 ? (autoFocusCount++, autoFocusRef) : null} + ref={autoFocus && autoFocusCount === 0 ? (autoFocusCount++, appliedRef) : null} > {content} diff --git a/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts b/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts index 763a96df96a..fcf8dcca3ab 100644 --- a/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts +++ b/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts @@ -8,4 +8,5 @@ export const DefaultFeatureFlags = FeatureFlagScope.create({ primer_react_action_list_group_heading_trailing_action: false, primer_react_action_list_item_gap: false, primer_react_timeline_list_semantics: false, + primer_react_merged_forwarded_refs: false, }) diff --git a/packages/react/src/FilteredActionList/FilteredActionList.test.tsx b/packages/react/src/FilteredActionList/FilteredActionList.test.tsx index a2554222900..f90ba4fd845 100644 --- a/packages/react/src/FilteredActionList/FilteredActionList.test.tsx +++ b/packages/react/src/FilteredActionList/FilteredActionList.test.tsx @@ -2,6 +2,7 @@ import {render} from '@testing-library/react' import {describe, expect, it, vi} from 'vitest' import React from 'react' import {FilteredActionList} from '../FilteredActionList' +import {FeatureFlags} from '../FeatureFlags' import {FilteredActionListBodyLoader, FilteredActionListLoadingTypes} from './FilteredActionListLoaders' import {implementsClassName} from '../utils/testing' import classes from './FilteredActionList.module.css' @@ -120,3 +121,39 @@ describe('FilteredActionListBodyLoader', () => { }) }) }) + +describe('FilteredActionList forwarded refs (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => { + it('forwards the input ref object', () => { + const ref = React.createRef() + render( + + + , + ) + expect(ref.current).toBeInstanceOf(HTMLInputElement) + }) + + it('forwards the scroll container ref object', () => { + const ref = React.createRef() + render( + + + , + ) + expect(ref.current).toBeInstanceOf(HTMLDivElement) + }) + + it('calls a scroll container callback ref', () => { + const refCallback = vi.fn() + render( + + + , + ) + expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLDivElement)).toBe(true) + }) + }) + } +}) diff --git a/packages/react/src/FilteredActionList/FilteredActionList.tsx b/packages/react/src/FilteredActionList/FilteredActionList.tsx index 29c6328f9cb..e004536ca55 100644 --- a/packages/react/src/FilteredActionList/FilteredActionList.tsx +++ b/packages/react/src/FilteredActionList/FilteredActionList.tsx @@ -8,7 +8,6 @@ import {ActionList, type ActionListProps} from '../ActionList' import type {GroupedListProps, ListPropsBase, ItemInput, RenderItemFn} from './' import {useFocusZone} from '../hooks/useFocusZone' import {useId} from '../hooks/useId' -import {useProvidedRefOrCreate} from '../hooks/useProvidedRefOrCreate' import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate' import useScrollFlash from '../hooks/useScrollFlash' import {VisuallyHidden} from '../VisuallyHidden' @@ -21,6 +20,8 @@ import {isValidElementType} from 'react-is' import {useAnnouncements} from './useAnnouncements' import {clsx} from 'clsx' import {useVirtualizer} from '@tanstack/react-virtual' +import {useMergedRefs, useProvidedRefOrCreate} from '../hooks' +import {useFeatureFlag} from '../FeatureFlags' import {FilteredActionListInput} from './FilteredActionListInput' const menuScrollMargins: ScrollIntoViewOptions = {startMargin: 0, endMargin: 8} @@ -189,10 +190,25 @@ export function FilteredActionList({ const inputAndListContainerRef = useRef(null) const listRef = useRef(null) - const scrollContainerRef = useProvidedRefOrCreate( + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + + const scrollContainerRef = useRef(null) + const mergedScrollContainerRef = useMergedRefs(scrollContainerRef, providedScrollContainerRef) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readScrollContainerRef` with `scrollContainerRef` and `appliedScrollContainerRef` with `mergedScrollContainerRef`. + const providedOrCreatedScrollContainerRef = useProvidedRefOrCreate( providedScrollContainerRef as React.RefObject, ) - const inputRef = useProvidedRefOrCreate(providedInputRef) + const readScrollContainerRef = mergedRefEnabled ? scrollContainerRef : providedOrCreatedScrollContainerRef + const appliedScrollContainerRef = mergedRefEnabled ? mergedScrollContainerRef : providedOrCreatedScrollContainerRef + + const inputRef = useRef(null) + const mergedInputRef = useMergedRefs(inputRef, providedInputRef) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readInputRef` with `inputRef` and `appliedInputRef` with `mergedInputRef`. + const providedOrCreatedInputRef = useProvidedRefOrCreate(providedInputRef) + const readInputRef = mergedRefEnabled ? inputRef : providedOrCreatedInputRef + const appliedInputRef = mergedRefEnabled ? mergedInputRef : providedOrCreatedInputRef const usingRovingTabindex = _PrivateFocusManagement === 'roving-tabindex' const [listContainerElement, setListContainerElement] = useState(null) @@ -282,8 +298,8 @@ export function FilteredActionList({ ) useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-pass-data-to-parent - onInputRefChanged?.(inputRef) - }, [inputRef, onInputRefChanged]) + onInputRefChanged?.(readInputRef) + }, [readInputRef, onInputRefChanged]) // Matches the most common ActionList.Item height (single-line text + description). // Items are measured dynamically via `measureElement`, so this only affects the @@ -293,7 +309,7 @@ export function FilteredActionList({ // eslint-disable-next-line react-hooks/incompatible-library const virtualizer = useVirtualizer({ count: items.length, - getScrollElement: () => scrollContainerRef.current, + getScrollElement: () => readScrollContainerRef.current, estimateSize: () => DEFAULT_VIRTUAL_ITEM_HEIGHT, overscan: 10, enabled: isVirtualized, @@ -331,7 +347,7 @@ export function FilteredActionList({ focusableElementFilter: element => { return !(element instanceof HTMLInputElement) }, - activeDescendantFocus: inputRef, + activeDescendantFocus: readInputRef, onActiveDescendantChanged: (current, previous, directlyActivated) => { activeDescendantRef.current = current @@ -343,8 +359,8 @@ export function FilteredActionList({ } } - if (current && scrollContainerRef.current && (directlyActivated || focusPrependedElements)) { - scrollIntoView(current, scrollContainerRef.current, { + if (current && readScrollContainerRef.current && (directlyActivated || focusPrependedElements)) { + scrollIntoView(current, readScrollContainerRef.current, { ...menuScrollMargins, behavior: scrollBehavior, }) @@ -361,13 +377,13 @@ export function FilteredActionList({ ) useEffect(() => { - if (activeDescendantRef.current && scrollContainerRef.current) { - scrollIntoView(activeDescendantRef.current, scrollContainerRef.current, { + if (activeDescendantRef.current && readScrollContainerRef.current) { + scrollIntoView(activeDescendantRef.current, readScrollContainerRef.current, { ...menuScrollMargins, behavior: scrollBehavior, }) } - }, [items, inputRef, scrollContainerRef, scrollBehavior]) + }, [items, readInputRef, readScrollContainerRef, scrollBehavior]) useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler @@ -379,8 +395,8 @@ export function FilteredActionList({ // Listen for focus changes within the container const handleFocusIn = (event: FocusEvent) => { - if (event.target === inputRef.current || list.contains(event.target as Node)) { - setIsInputFocused(inputRef.current && inputRef.current === document.activeElement ? true : false) + if (event.target === readInputRef.current || list.contains(event.target as Node)) { + setIsInputFocused(readInputRef.current && readInputRef.current === document.activeElement ? true : false) } } @@ -390,26 +406,26 @@ export function FilteredActionList({ inputAndListContainerElement.removeEventListener('focusin', handleFocusIn) } } - }, [items, inputRef, listContainerElement, usingRovingTabindex]) // Re-run when items change to update active indicators + }, [items, readInputRef, listContainerElement, usingRovingTabindex]) // Re-run when items change to update active indicators useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler if (usingRovingTabindex && !loading) { // eslint-disable-next-line react-you-might-not-need-an-effect/no-adjust-state-on-prop-change - setIsInputFocused(inputRef.current && inputRef.current === document.activeElement ? true : false) + setIsInputFocused(readInputRef.current && readInputRef.current === document.activeElement ? true : false) } - }, [loading, inputRef, usingRovingTabindex]) + }, [loading, readInputRef, usingRovingTabindex]) useAnnouncements( items, usingRovingTabindex ? listRef : {current: listContainerElement}, - inputRef, + readInputRef, announcementsEnabled, loading, messageText, _PrivateFocusManagement, ) - useScrollFlash(scrollContainerRef) + useScrollFlash(readScrollContainerRef) const handleSelectAllChange = useCallback( (e: React.ChangeEvent) => { @@ -421,8 +437,10 @@ export function FilteredActionList({ ) function getBodyContent() { - if (loading && scrollContainerRef.current && loadingType.appearsInBody) { - return + if (loading && readScrollContainerRef.current && loadingType.appearsInBody) { + return ( + + ) } if (message) { return message @@ -559,7 +577,7 @@ export function FilteredActionList({ data-component="FilteredActionList" > )} {/* @ts-expect-error div needs a non nullable ref */} -
+
{getBodyContent()}
diff --git a/packages/react/src/FilteredActionList/FilteredActionListInput.tsx b/packages/react/src/FilteredActionList/FilteredActionListInput.tsx index 987f4d8ffed..776f4733755 100644 --- a/packages/react/src/FilteredActionList/FilteredActionListInput.tsx +++ b/packages/react/src/FilteredActionList/FilteredActionListInput.tsx @@ -5,7 +5,7 @@ import type {TextInputProps} from '../TextInput' import classes from './FilteredActionList.module.css' export interface FilteredActionListInputProps extends Partial> { - inputRef: React.RefObject + inputRef: React.Ref onInputChange?: (e: React.ChangeEvent) => void onInputKeyPress?: React.KeyboardEventHandler onInputKeyDown?: React.KeyboardEventHandler diff --git a/packages/react/src/PageHeader/PageHeader.test.tsx b/packages/react/src/PageHeader/PageHeader.test.tsx index 35966323b2c..63621ae4208 100644 --- a/packages/react/src/PageHeader/PageHeader.test.tsx +++ b/packages/react/src/PageHeader/PageHeader.test.tsx @@ -2,6 +2,7 @@ import {describe, expect, it, vi} from 'vitest' import React from 'react' import {render} from '@testing-library/react' import {PageHeader} from '.' +import {FeatureFlags} from '../FeatureFlags' import {implementsClassName} from '../utils/testing' import classes from './PageHeader.module.css' @@ -306,3 +307,45 @@ describe('PageHeader', () => { expect(getByRole('link', {name: /parent/i})).toHaveAttribute('href', '/somewhere') }) }) + +describe('PageHeader forwarded ref (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => { + it('forwards the Root ref object', () => { + const ref = React.createRef() + render( + + + Content + + , + ) + expect(ref.current).toBeInstanceOf(HTMLDivElement) + }) + + it('calls a Root callback ref', () => { + const refCallback = vi.fn() + render( + + + Content + + , + ) + expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLDivElement)).toBe(true) + }) + + it('forwards the TitleArea ref object', () => { + const ref = React.createRef() + render( + + + Heading + + , + ) + expect(ref.current).toBeInstanceOf(HTMLDivElement) + }) + }) + } +}) diff --git a/packages/react/src/PageHeader/PageHeader.tsx b/packages/react/src/PageHeader/PageHeader.tsx index 23217193a15..c334ec38440 100644 --- a/packages/react/src/PageHeader/PageHeader.tsx +++ b/packages/react/src/PageHeader/PageHeader.tsx @@ -1,4 +1,4 @@ -import React, {useEffect} from 'react' +import React, {useEffect, useRef} from 'react' import type {ResponsiveValue} from '../hooks/useResponsiveValue' import {isResponsiveValue} from '../hooks/useResponsiveValue' import Heading from '../Heading' @@ -10,7 +10,8 @@ import {getResponsiveAttributes} from '../internal/utils/getResponsiveAttributes import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {areAllValuesTheSame, haveRegularAndWideSameValue} from '../utils/getBreakpointDeclarations' import {warning} from '../utils/warning' -import {useProvidedRefOrCreate} from '../hooks' +import {useMergedRefs, useProvidedRefOrCreate} from '../hooks' +import {useFeatureFlag} from '../FeatureFlags' import type {AriaRole, FCWithSlotMarker} from '../utils/types' import {clsx} from 'clsx' @@ -49,7 +50,14 @@ export type PageHeaderProps = { const Root = React.forwardRef>( ({children, className, as: BaseComponent = 'div', 'aria-label': ariaLabel, role, hasBorder}, forwardedRef) => { - const rootRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + const rootRef = useRef(null) + const mergedRef = useMergedRefs(rootRef, forwardedRef) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readRef` with `rootRef` and `appliedRef` with `mergedRef`. + const providedOrCreatedRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const readRef = mergedRefEnabled ? rootRef : providedOrCreatedRef + const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef // Hoist title size + navigation visibility off children onto the root so // styling can use plain attribute selectors instead of `:has()`. We descend @@ -117,8 +125,8 @@ const Root = React.forwardRef { + if (!readRef.current || readRef.current.children.length <= 0) return + const titleArea = Array.from(readRef.current.children as HTMLCollection).find(child => { return child instanceof HTMLElement && child.getAttribute('data-component') === 'TitleArea' }) @@ -150,12 +158,12 @@ const Root = React.forwardRef>( ({children, className, hidden = false, variant = 'medium'}, forwardedRef) => { - const titleAreaRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the two declarations below, and replace all instances of `appliedRef` with `forwardedRef`. + const providedOrCreatedRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const appliedRef = mergedRefEnabled ? forwardedRef : providedOrCreatedRef return (
{ }) }) }) + +describe('TextInput forwarded ref (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => { + it('forwards a ref object to the element', () => { + const ref = createRef() + render( + + + , + ) + expect(ref.current).toBeInstanceOf(HTMLInputElement) + }) + + it('calls a callback ref with the element', () => { + const refCallback = vi.fn() + render( + + + , + ) + expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLInputElement)).toBe(true) + }) + }) + } +}) diff --git a/packages/react/src/TextInput/TextInput.tsx b/packages/react/src/TextInput/TextInput.tsx index b003a7ed4c8..de2c0e12af9 100644 --- a/packages/react/src/TextInput/TextInput.tsx +++ b/packages/react/src/TextInput/TextInput.tsx @@ -1,5 +1,5 @@ import type {MouseEventHandler} from 'react' -import React, {useCallback, useState, useId} from 'react' +import React, {useCallback, useState, useId, useRef} from 'react' import {isValidElementType} from 'react-is' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {clsx} from 'clsx' @@ -7,7 +7,6 @@ import {AlertFillIcon} from '@primer/octicons-react' import classes from './TextInput.module.css' import TextInputInnerVisualSlot from '../internal/components/TextInputInnerVisualSlot' -import {useProvidedRefOrCreate} from '../hooks' import type {Merge} from '../utils/types' import type {StyledWrapperProps} from '../internal/components/TextInputWrapper' import TextInputWrapper from '../internal/components/TextInputWrapper' @@ -18,6 +17,8 @@ import visuallyHiddenClasses from '../_VisuallyHidden.module.css' import {getCharacterCountState, SCREEN_READER_DELAY} from '../utils/character-counter' import {AriaStatus} from '../live-region' import Text from '../Text' +import {useMergedRefs, useProvidedRefOrCreate} from '../hooks' +import {useFeatureFlag} from '../FeatureFlags' export type TextInputNonPassthroughProps = { /** @deprecated Use `leadingVisual` or `trailingVisual` prop instead */ @@ -112,7 +113,14 @@ const TextInput = React.forwardRef( ref, ) => { const [isInputFocused, setIsInputFocused] = useState(false) - const inputRef = useProvidedRefOrCreate(ref as React.RefObject) + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + const inputRef = useRef(null) + const mergedRef = useMergedRefs(inputRef, ref) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readRef` with `inputRef` and `appliedRef` with `mergedRef`. + const providedOrCreatedRef = useProvidedRefOrCreate(ref as React.RefObject) + const readRef = mergedRefEnabled ? inputRef : providedOrCreatedRef + const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef // For uncontrolled usage we track the length of the input's content so the // character counter can be derived during render rather than synced from an @@ -141,8 +149,8 @@ const TextInput = React.forwardRef( const focusInput: MouseEventHandler = e => { // Don't call focus() if the input itself was clicked on date/time inputs. - if (e.target !== inputRef.current || !isSegmentedInputType) { - inputRef.current?.focus() + if (e.target !== readRef.current || !isSegmentedInputType) { + readRef.current?.focus() } } const leadingVisualId = useId() @@ -220,7 +228,7 @@ const TextInput = React.forwardRef( & { - ref?: React.RefObject + ref?: React.Ref } // map tooltip direction to anchoredPosition props @@ -126,7 +127,14 @@ export const Tooltip: ForwardRefExoticComponent< ) => { const tooltipId = useId(id) const child = Children.only(children) - const triggerRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + const triggerRef = useRef(null) + const mergedTriggerRef = useMergedRefs(triggerRef, forwardedRef) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readTriggerRef` with `triggerRef` and `appliedTriggerRef` with `mergedTriggerRef`. + const providedOrCreatedRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const readTriggerRef = mergedRefEnabled ? triggerRef : providedOrCreatedRef + const appliedTriggerRef = mergedRefEnabled ? mergedTriggerRef : providedOrCreatedRef const tooltipElRef = useRef(null) const [calculatedDirection, setCalculatedDirection] = useState(direction) @@ -141,13 +149,13 @@ export const Tooltip: ForwardRefExoticComponent< try { if ( tooltipElRef.current && - triggerRef.current && + readTriggerRef.current && tooltipElRef.current.hasAttribute('popover') && !tooltipElRef.current.matches(':popover-open') && !_privateDisableTooltip ) { const tooltip = tooltipElRef.current - const trigger = triggerRef.current + const trigger = readTriggerRef.current tooltip.showPopover() setIsPopoverOpen(true) /* @@ -188,7 +196,7 @@ export const Tooltip: ForwardRefExoticComponent< try { if ( tooltipElRef.current && - triggerRef.current && + readTriggerRef.current && tooltipElRef.current.hasAttribute('popover') && tooltipElRef.current.matches(':popover-open') ) { @@ -218,13 +226,13 @@ export const Tooltip: ForwardRefExoticComponent< const value = useMemo(() => ({tooltipId}), [tooltipId]) useEffect(() => { - if (!tooltipElRef.current || !triggerRef.current) return + if (!tooltipElRef.current || !readTriggerRef.current) return /* * ACCESSIBILITY CHECKS */ // Has trigger element or any of its children interactive elements? - const isTriggerInteractive = isInteractive(triggerRef.current) - const triggerChildren = triggerRef.current.childNodes + const isTriggerInteractive = isInteractive(readTriggerRef.current) + const triggerChildren = readTriggerRef.current.childNodes // two levels deep const hasInteractiveDescendant = Array.from(triggerChildren).some(child => { return ( @@ -241,8 +249,8 @@ export const Tooltip: ForwardRefExoticComponent< // If the tooltip is used for labelling the interactive element, the trigger element or any of its children should not have aria-label // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler if (type === 'label') { - const hasAriaLabel = triggerRef.current.hasAttribute('aria-label') - const hasAriaLabelInChildren = Array.from(triggerRef.current.childNodes).some( + const hasAriaLabel = readTriggerRef.current.hasAttribute('aria-label') + const hasAriaLabelInChildren = Array.from(readTriggerRef.current.childNodes).some( child => child instanceof HTMLElement && child.hasAttribute('aria-label'), ) warning( @@ -260,7 +268,7 @@ export const Tooltip: ForwardRefExoticComponent< const tooltip = tooltipElRef.current tooltip.setAttribute('popover', 'auto') - }, [tooltipElRef, triggerRef, direction, type]) + }, [tooltipElRef, readTriggerRef, direction, type]) useOnEscapePress( (event: KeyboardEvent) => { @@ -284,8 +292,8 @@ export const Tooltip: ForwardRefExoticComponent< <> {React.isValidElement(child) && React.cloneElement(child as React.ReactElement, { - // @ts-expect-error it needs a non nullable ref - ref: triggerRef, + // @ts-expect-error the provided-or-created ref path needs a non nullable ref + ref: appliedTriggerRef, // If it is a type description, we use tooltip to describe the trigger 'aria-describedby': (() => { // If tooltip is not a description type, keep the original aria-describedby diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index 92d9a0ed53d..bbd89bdae9f 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -1,5 +1,5 @@ import type React from 'react' -import {describe, expect, it} from 'vitest' +import {describe, expect, it, vi} from 'vitest' import type {TooltipProps} from '../Tooltip' import {Tooltip} from '../Tooltip' import {render as HTMLRender} from '@testing-library/react' @@ -12,6 +12,8 @@ import {XIcon} from '@primer/octicons-react' import classes from '../Tooltip.module.css' import type {JSX} from 'react' +import {createRef} from 'react' +import {FeatureFlags} from '../../FeatureFlags' import {implementsClassName, withExpectedConsoleError} from '../../utils/testing' const TooltipComponent = (props: Omit & {text?: string}) => ( @@ -225,3 +227,35 @@ describe('Tooltip data-component attributes', () => { expect(keybindingHintContainer).toBeInTheDocument() }) }) + +describe('Tooltip forwarded ref (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => { + it('forwards a ref object to the trigger element', () => { + const ref = createRef() + HTMLRender( + + + + + , + ) + expect(ref.current).toBeInstanceOf(HTMLButtonElement) + expect(ref.current).toHaveTextContent('Button Text') + }) + + it('calls a callback ref with the trigger element', () => { + const refCallback = vi.fn() + HTMLRender( + + + + + , + ) + expect(refCallback).toHaveBeenCalled() + expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLButtonElement)).toBe(true) + }) + }) + } +}) diff --git a/packages/react/src/experimental/Tabs/Tabs.examples.stories.tsx b/packages/react/src/experimental/Tabs/Tabs.examples.stories.tsx index b0c93053901..6f04fd5bd85 100644 --- a/packages/react/src/experimental/Tabs/Tabs.examples.stories.tsx +++ b/packages/react/src/experimental/Tabs/Tabs.examples.stories.tsx @@ -19,8 +19,7 @@ const CustomTabList = (props: React.PropsWithChildren) => { return (
- {/* @ts-expect-error it needs a non nullable ref */} - {props.children} + )}>{props.children}
) } diff --git a/packages/react/src/experimental/Tabs/Tabs.test.tsx b/packages/react/src/experimental/Tabs/Tabs.test.tsx index a5ca12995a1..c0d1c1956ab 100644 --- a/packages/react/src/experimental/Tabs/Tabs.test.tsx +++ b/packages/react/src/experimental/Tabs/Tabs.test.tsx @@ -3,6 +3,8 @@ import userEvent from '@testing-library/user-event' import React from 'react' import {describe, test, expect, vi} from 'vitest' import {Tabs, TabList, Tab, TabPanel} from './Tabs' +import {useTabList} from './useTabList' +import {FeatureFlags} from '../../FeatureFlags' import {implementsClassName} from '../../utils/testing' describe('Tabs', () => { @@ -582,3 +584,37 @@ describe('Tabs', () => { expect(tabC).toHaveAttribute('tabindex', '-1') }) }) + +function TabListRefHarness({tabRef}: {tabRef: React.Ref}) { + const {tabListProps} = useTabList({'aria-label': 'Test tabs', ref: tabRef}) + return ( + // @ts-expect-error it needs a non nullable ref +
+ ) +} + +describe('useTabList forwarded ref (primer_react_merged_forwarded_refs)', () => { + for (const enabled of [true, false]) { + describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => { + test('forwards a ref object to the tablist', () => { + const ref = React.createRef() + render( + + + , + ) + expect(ref.current).toBeInstanceOf(HTMLDivElement) + }) + + test('calls a callback ref with the tablist', () => { + const refCallback = vi.fn() + render( + + + , + ) + expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLDivElement)).toBe(true) + }) + }) + } +}) diff --git a/packages/react/src/experimental/Tabs/types.ts b/packages/react/src/experimental/Tabs/types.ts index 45337b1b9e8..b3ac345b982 100644 --- a/packages/react/src/experimental/Tabs/types.ts +++ b/packages/react/src/experimental/Tabs/types.ts @@ -86,7 +86,7 @@ export type TabsContextValue = { export type TabListHookProps = TabListProps & { /** Optional ref to use for the tablist. If none is provided, one will be generated automatically */ - ref?: React.RefObject + ref?: React.Ref } export type TabListHookResult = { @@ -96,7 +96,7 @@ export type TabListHookResult = { 'aria-orientation': AriaAttributes['aria-orientation'] 'aria-label': AriaAttributes['aria-label'] 'aria-labelledby': AriaAttributes['aria-labelledby'] - ref: React.RefObject + ref: React.Ref role: 'tablist' } } diff --git a/packages/react/src/experimental/Tabs/useTabList.ts b/packages/react/src/experimental/Tabs/useTabList.ts index f1f8c487d88..64ce593fe04 100644 --- a/packages/react/src/experimental/Tabs/useTabList.ts +++ b/packages/react/src/experimental/Tabs/useTabList.ts @@ -1,14 +1,23 @@ import type React from 'react' -import {useProvidedRefOrCreate} from '../../hooks' +import {useRef} from 'react' +import {useMergedRefs, useProvidedRefOrCreate} from '../../hooks' +import {useFeatureFlag} from '../../FeatureFlags' import type {TabListHookProps, TabListHookResult} from './types' export function useTabList(props: TabListHookProps): TabListHookResult { const {'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby, 'aria-orientation': ariaOrientation} = props - const ref = useProvidedRefOrCreate(props.ref) + const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') + const tabListRef = useRef(null) + const mergedRef = useMergedRefs(tabListRef, props.ref) + // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. + // At graduation: remove the three declarations below, and replace all instances of `readRef` with `tabListRef` and `appliedRef` with `mergedRef`. + const providedOrCreatedRef = useProvidedRefOrCreate(props.ref as React.RefObject) + const readRef = mergedRefEnabled ? tabListRef : providedOrCreatedRef + const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef const onKeyDown = (event: React.KeyboardEvent) => { - const {current: tablist} = ref + const {current: tablist} = readRef if (tablist === null) { return } @@ -57,7 +66,7 @@ export function useTabList(props: TabListHookProps): T return { tabListProps: { - ref, + ref: appliedRef, 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby, 'aria-orientation': ariaOrientation ?? 'horizontal', diff --git a/packages/react/src/hooks/__tests__/useMergedRefs.test.tsx b/packages/react/src/hooks/__tests__/useMergedRefs.test.tsx index 4e826cf1e32..87fbbbc69f9 100644 --- a/packages/react/src/hooks/__tests__/useMergedRefs.test.tsx +++ b/packages/react/src/hooks/__tests__/useMergedRefs.test.tsx @@ -1,5 +1,5 @@ import {render, renderHook} from '@testing-library/react' -import React, {forwardRef, type RefObject} from 'react' +import React, {forwardRef, type RefObject, version} from 'react' import {describe, expect, it, vi} from 'vitest' import {reactMajorVersion} from '../../utils/environment' import {useMergedRefs} from '../useMergedRefs' @@ -115,7 +115,7 @@ describe('useMergedRefs', () => { expect(refB).toHaveBeenCalledExactlyOnceWith('test') }) - it('handles React 18 null values correctly', () => { + it.runIf(version.startsWith('18'))('handles React 18 null values correctly', () => { const refA = vi.fn() const refB = vi.fn() @@ -132,7 +132,7 @@ describe('useMergedRefs', () => { expect(refB).toHaveBeenCalledWith(null) }) - it('handles React 19 cleanup functions correctly and independently', () => { + it.skipIf(version.startsWith('18'))('handles React 19 cleanup functions correctly and independently', () => { const refA = vi.fn() const cleanupRefB = vi.fn() const refB = vi.fn().mockReturnValue(cleanupRefB) @@ -153,7 +153,7 @@ describe('useMergedRefs', () => { } // React 19 will call cleanup function and not pass null - cleanup() + cleanup!() expect(refA).toHaveBeenCalledWith(null) expect(refB).not.toHaveBeenCalledWith(null) diff --git a/packages/react/src/hooks/useMergedRefs.ts b/packages/react/src/hooks/useMergedRefs.ts index 5e2516f6688..baebc75c2e9 100644 --- a/packages/react/src/hooks/useMergedRefs.ts +++ b/packages/react/src/hooks/useMergedRefs.ts @@ -1,7 +1,9 @@ import type {ForwardedRef, Ref as StandardRef, MutableRefObject} from 'react' -import {useCallback} from 'react' +import {useCallback, version} from 'react' import {isExperimentalReactVersion, reactMajorVersion} from '../utils/environment' +const majorReactVersion = parseInt(version.split('.')[0] ?? '18', 10) + /** * Cleanup functions for refs were introduced in React 19. For feature detection, * we look to see if current version of React is >= 19 or if it is an @@ -27,18 +29,18 @@ const supportsRefCleanup = reactMajorVersion >= 19 || isExperimentalReactVersion * // React 18 * const Example = forwardRef((props, forwardedRef) => { * const ref = useRef(null) - * const combinedRef = useMergedRefs(forwardedRef, ref) + * const mergedRef = useMergedRefs(forwardedRef, ref) * - * return