diff --git a/.changeset/strong-tooltips-anchor.md b/.changeset/strong-tooltips-anchor.md new file mode 100644 index 00000000000..79e79c69871 --- /dev/null +++ b/.changeset/strong-tooltips-anchor.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Tooltip: Prevent React Fragment triggers from crashing and provide a clear development warning \ No newline at end of file diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx index 840ded8fc91..4c8f7f42867 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -128,6 +128,11 @@ export const Tooltip: ForwardRefExoticComponent< ) => { const tooltipId = useId(id) const child = Children.only(children) + const isFragmentTrigger = React.isValidElement(child) && child.type === React.Fragment + warning( + isFragmentTrigger, + 'The `Tooltip` component does not support a React Fragment as its trigger. Pass a single interactive element instead.', + ) const elementChild = React.isValidElement(child) ? child : undefined const childRef = reactMajorVersion > 18 ? elementChild?.props.ref : (elementChild as {ref?: React.Ref} | undefined)?.ref @@ -153,7 +158,7 @@ export const Tooltip: ForwardRefExoticComponent< try { if ( tooltipElRef.current && - readTriggerRef.current && + readTriggerRef.current instanceof HTMLElement && tooltipElRef.current.hasAttribute('popover') && !tooltipElRef.current.matches(':popover-open') && !_privateDisableTooltip @@ -231,12 +236,19 @@ export const Tooltip: ForwardRefExoticComponent< useEffect(() => { if (!tooltipElRef.current || !readTriggerRef.current) return + const trigger = readTriggerRef.current + const isInvalidTrigger = !(trigger instanceof HTMLElement) + warning( + isInvalidTrigger, + 'The `Tooltip` component expects its trigger ref to resolve to an HTML element. Ensure the trigger forwards its ref to a single interactive element instead of a React Fragment.', + ) + if (isInvalidTrigger) return /* * ACCESSIBILITY CHECKS */ // Has trigger element or any of its children interactive elements? - const isTriggerInteractive = isInteractive(readTriggerRef.current) - const triggerChildren = readTriggerRef.current.childNodes + const isTriggerInteractive = isInteractive(trigger) + const triggerChildren = trigger.childNodes // two levels deep const hasInteractiveDescendant = Array.from(triggerChildren).some(child => { return ( @@ -253,8 +265,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 = readTriggerRef.current.hasAttribute('aria-label') - const hasAriaLabelInChildren = Array.from(readTriggerRef.current.childNodes).some( + const hasAriaLabel = trigger.hasAttribute('aria-label') + const hasAriaLabelInChildren = Array.from(trigger.childNodes).some( child => child instanceof HTMLElement && child.hasAttribute('aria-label'), ) warning( @@ -291,6 +303,8 @@ export const Tooltip: ForwardRefExoticComponent< // Normalize keybindingHint to an array for uniform rendering const keybindingHints = Array.isArray(keybindingHint) ? keybindingHint : [keybindingHint] + if (isFragmentTrigger) return child + return ( <> diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index de25eca6c11..46ca86f8d88 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -2,7 +2,7 @@ import type React from 'react' import {describe, expect, it, vi} from 'vitest' import type {TooltipProps} from '../Tooltip' import {Tooltip} from '../Tooltip' -import {render as HTMLRender} from '@testing-library/react' +import {fireEvent, render as HTMLRender} from '@testing-library/react' import BaseStyles from '../../BaseStyles' import {Button, IconButton} from '../../Button' import {ActionMenu} from '../../ActionMenu' @@ -12,9 +12,9 @@ import {XIcon} from '@primer/octicons-react' import classes from '../Tooltip.module.css' import type {JSX} from 'react' -import {createRef} from 'react' +import {createRef, forwardRef, useImperativeHandle} from 'react' import {FeatureFlags} from '../../FeatureFlags' -import {implementsClassName, withExpectedConsoleError} from '../../utils/testing' +import {implementsClassName, withExpectedConsoleError, withExpectedConsoleWarning} from '../../utils/testing' const TooltipComponent = (props: Omit & {text?: string}) => ( @@ -31,6 +31,17 @@ const TooltipComponentWithExistingDescription = (props: Omit ) +const InvalidRefTrigger = forwardRef>( + ({children, ...props}, forwardedRef) => { + useImperativeHandle(forwardedRef, () => ({}) as HTMLElement, []) + return ( + + ) + }, +) + // eslint-disable-next-line @typescript-eslint/no-explicit-any function ExampleWithActionMenu({actionMenuTrigger}: {actionMenuTrigger: React.ReactElement}): JSX.Element { return ( @@ -146,6 +157,20 @@ describe('Tooltip', () => { ) }) }) + it('should warn and render the trigger unchanged if it is a React Fragment', () => { + withExpectedConsoleWarning(() => { + const {getByRole, queryByText} = HTMLRender( + + <> + + + , + ) + + expect(getByRole('button', {name: 'Button Text'})).toBeInTheDocument() + expect(queryByText('Tooltip text')).not.toBeInTheDocument() + }) + }) it('does not throw an error when the trigger element is a button in a fieldset', () => { const {getByRole} = HTMLRender(
@@ -288,6 +313,20 @@ describe('Tooltip forwarded ref (primer_react_merged_forwarded_refs)', () => { expect(buttonRefCallback).toHaveBeenCalledWith(button) expect(tooltipRefCallback).toHaveBeenCalledWith(button) }) + + it('warns without crashing when a custom trigger ref does not resolve to an HTML element', () => { + withExpectedConsoleWarning(() => { + const {getByRole} = HTMLRender( + + + Button Text + + , + ) + + expect(() => fireEvent.focus(getByRole('button', {name: 'Button Text'}))).not.toThrow() + }) + }) }) } })