From 6ed5f6e3be015e30b4bdf0eca9f68aca22cb7528 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Fri, 11 Sep 2026 15:58:53 +0000 Subject: [PATCH] Fix Tooltip Fragment trigger handling --- .changeset/strong-tooltips-anchor.md | 5 +++ packages/react/src/TooltipV2/Tooltip.tsx | 24 +++++++--- .../src/TooltipV2/__tests__/Tooltip.test.tsx | 45 +++++++++++++++++-- 3 files changed, 66 insertions(+), 8 deletions(-) create mode 100644 .changeset/strong-tooltips-anchor.md 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 e436b7bd761..68827c3a9b8 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -127,6 +127,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 mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs') const triggerRef = useRef(null) const mergedTriggerRef = useMergedRefs(triggerRef, forwardedRef) @@ -149,7 +154,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 @@ -227,12 +232,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 ( @@ -249,8 +261,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( @@ -287,6 +299,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 bbd89bdae9f..8e8ab345cc6 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('should not throw an error when the trigger element is a button in a fieldset', () => { const {getByRole} = HTMLRender(
@@ -256,6 +281,20 @@ describe('Tooltip forwarded ref (primer_react_merged_forwarded_refs)', () => { expect(refCallback).toHaveBeenCalled() expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLButtonElement)).toBe(true) }) + + 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() + }) + }) }) } })