From 11ba75df1426bd249461da1da374f1b021d1ecee Mon Sep 17 00:00:00 2001 From: Taylor Jones Date: Tue, 10 Nov 2026 12:15:49 -0600 Subject: [PATCH 1/2] fix(Tooltip): make tooltips with empty text inert A tooltip with empty `text` (and no `keybindingHint`) no longer opens and no longer labels or describes its trigger. That means you can turn a tooltip off with `text={allowed ? '' : 'Not allowed'}` instead of conditionally wrapping the trigger, which remounts it and drops focus. Fixes #8571 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .changeset/tooltip-empty-text.md | 5 ++ .../react/src/TooltipV2/Tooltip.docs.json | 5 +- .../TooltipV2/Tooltip.features.stories.tsx | 13 +++ packages/react/src/TooltipV2/Tooltip.tsx | 30 +++++-- .../src/TooltipV2/__tests__/Tooltip.test.tsx | 82 ++++++++++++++++++- 5 files changed, 126 insertions(+), 9 deletions(-) create mode 100644 .changeset/tooltip-empty-text.md diff --git a/.changeset/tooltip-empty-text.md b/.changeset/tooltip-empty-text.md new file mode 100644 index 00000000000..5c011e4f222 --- /dev/null +++ b/.changeset/tooltip-empty-text.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Tooltip: An empty `text` no longer opens an empty tooltip or labels/describes the trigger, so a tooltip can be turned off conditionally without remounting its trigger. diff --git a/packages/react/src/TooltipV2/Tooltip.docs.json b/packages/react/src/TooltipV2/Tooltip.docs.json index 019128ada7d..fc1dcfe36b4 100644 --- a/packages/react/src/TooltipV2/Tooltip.docs.json +++ b/packages/react/src/TooltipV2/Tooltip.docs.json @@ -32,6 +32,9 @@ }, { "id": "components-tooltipv2-features--on-action-menu-anchor" + }, + { + "id": "components-tooltipv2-features--conditional-text" } ], "props": [ @@ -45,7 +48,7 @@ "name": "text", "required": true, "type": "string", - "description": "The text to be displayed in the tooltip" + "description": "The text to be displayed in the tooltip. When this is an empty string and there is no `keybindingHint`, the tooltip does not open and does not label or describe the trigger. Use this to turn a tooltip off conditionally while keeping the trigger mounted, so that it keeps focus." }, { "name": "type", diff --git a/packages/react/src/TooltipV2/Tooltip.features.stories.tsx b/packages/react/src/TooltipV2/Tooltip.features.stories.tsx index d7915fc75d5..f58d96d30ef 100644 --- a/packages/react/src/TooltipV2/Tooltip.features.stories.tsx +++ b/packages/react/src/TooltipV2/Tooltip.features.stories.tsx @@ -1,3 +1,4 @@ +import {useState} from 'react' import {IconButton, Button} from '../Button' import Link from '../Link' import {ActionMenu} from '../ActionMenu' @@ -222,3 +223,15 @@ export const WithLongDelay = () => ( ) + +// An empty `text` turns the tooltip off without unmounting the trigger, so the trigger keeps focus +export const ConditionalText = () => { + const [showTooltip, setShowTooltip] = useState(true) + return ( +
+ + + +
+ ) +} diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx index 82cd3c36ed0..74c0bc14442 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -12,6 +12,7 @@ import {getAccessibleKeybindingHintString, KeybindingHint, type KeybindingHintPr import {usePlatform} from '../KeybindingHint/platform' import VisuallyHidden from '../_VisuallyHidden' import useSafeTimeout from '../hooks/useSafeTimeout' +import {useEffectCallback} from '../internal/hooks/useEffectCallback' import type {SlotMarker} from '../utils/types' import {TooltipContext} from './TooltipContext' import {reactMajorVersion} from '../utils/environment' @@ -19,6 +20,11 @@ import {reactMajorVersion} from '../utils/environment' export type TooltipDirection = 'nw' | 'n' | 'ne' | 'e' | 'se' | 's' | 'sw' | 'w' export type TooltipProps = React.PropsWithChildren<{ direction?: TooltipDirection + /** + * The text to display in the tooltip. When this is an empty string and there is no `keybindingHint`, the tooltip + * does not open and does not label or describe the trigger. Use this to turn a tooltip off conditionally while + * keeping the trigger mounted, so that it keeps focus. + */ text: string type?: 'label' | 'description' keybindingHint?: KeybindingHintProps['keys'] | Array @@ -146,6 +152,10 @@ export const Tooltip: ForwardRefExoticComponent< const appliedTriggerRef = mergedRefEnabled ? mergedTriggerRef : providedOrCreatedRef const tooltipElRef = useRef(null) + // Normalize keybindingHint to an array for uniform rendering + const keybindingHints = Array.isArray(keybindingHint) ? keybindingHint : [keybindingHint] + const hasContent = Boolean(text) || keybindingHints.length > 0 + const [calculatedDirection, setCalculatedDirection] = useState(direction) const [isPopoverOpen, setIsPopoverOpen] = useState(false) @@ -161,7 +171,8 @@ export const Tooltip: ForwardRefExoticComponent< readTriggerRef.current instanceof HTMLElement && tooltipElRef.current.hasAttribute('popover') && !tooltipElRef.current.matches(':popover-open') && - !_privateDisableTooltip + !_privateDisableTooltip && + hasContent ) { const tooltip = tooltipElRef.current const trigger = readTriggerRef.current @@ -231,6 +242,14 @@ export const Tooltip: ForwardRefExoticComponent< } } + // Close the tooltip and cancel any pending open if its content is removed while it is open. This syncs the + // imperative popover state with props, so it can't move to an event handler. + const closeTooltipFromEffect = useEffectCallback(closeTooltip) + useEffect(() => { + // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler + if (!hasContent) closeTooltipFromEffect() + }, [hasContent, closeTooltipFromEffect]) + // context value const value = useMemo(() => ({tooltipId}), [tooltipId]) @@ -302,9 +321,6 @@ export const Tooltip: ForwardRefExoticComponent< const platform = usePlatform() const hasAriaLabel = 'aria-label' in rest - // Normalize keybindingHint to an array for uniform rendering - const keybindingHints = Array.isArray(keybindingHint) ? keybindingHint : [keybindingHint] - if (isFragmentTrigger) return child return ( @@ -316,8 +332,8 @@ export const Tooltip: ForwardRefExoticComponent< 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 - if (type !== 'description') { + // If tooltip is not a description type or has no content, keep the original aria-describedby + if (type !== 'description' || !hasContent) { return child.props['aria-describedby'] } @@ -331,7 +347,7 @@ export const Tooltip: ForwardRefExoticComponent< return tooltipId })(), // If it is a label type, we use tooltip to label the trigger - 'aria-labelledby': type === 'label' ? tooltipId : child.props['aria-labelledby'], + 'aria-labelledby': type === 'label' && hasContent ? tooltipId : child.props['aria-labelledby'], onBlur: (event: React.FocusEvent) => { closeTooltip() child.props.onBlur?.(event) diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index 46ca86f8d88..015f41fa8f2 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -1,5 +1,6 @@ import type React from 'react' import {describe, expect, it, vi} from 'vitest' +import {page, userEvent} from 'vitest/browser' import type {TooltipProps} from '../Tooltip' import {Tooltip} from '../Tooltip' import {fireEvent, render as HTMLRender} from '@testing-library/react' @@ -12,7 +13,7 @@ import {XIcon} from '@primer/octicons-react' import classes from '../Tooltip.module.css' import type {JSX} from 'react' -import {createRef, forwardRef, useImperativeHandle} from 'react' +import {createRef, forwardRef, useImperativeHandle, useState} from 'react' import {FeatureFlags} from '../../FeatureFlags' import {implementsClassName, withExpectedConsoleError, withExpectedConsoleWarning} from '../../utils/testing' @@ -239,6 +240,85 @@ describe('Tooltip', () => { }) }) +describe('Tooltip with empty text', () => { + const getTooltipElement = (container: HTMLElement) => + container.querySelector('[data-component="Tooltip"]')! + + it('does not describe the trigger', () => { + const {getByRole} = HTMLRender() + expect(getByRole('button')).not.toHaveAttribute('aria-describedby') + }) + + it('does not label the trigger when the tooltip type is label', () => { + const {getByRole} = HTMLRender() + const trigger = getByRole('button') + expect(trigger).not.toHaveAttribute('aria-labelledby') + expect(trigger).toHaveAccessibleName('Button Text') + }) + + it("keeps the trigger's own aria-describedby", () => { + const {getByRole} = HTMLRender() + expect(getByRole('button')).toHaveAttribute('aria-describedby', 'external-description') + }) + + it('still labels the trigger when a keybinding hint is provided', () => { + const {getByRole, container} = HTMLRender() + expect(getByRole('button')).toHaveAttribute( + 'aria-labelledby', + getTooltipElement(container).querySelector('[id]')!.id, + ) + }) + + it('does not open on keyboard focus', async () => { + const {container} = HTMLRender() + await userEvent.tab() + expect(page.getByRole('button', {name: 'Button Text', exact: true}).element()).toHaveFocus() + expect(getTooltipElement(container).matches(':popover-open')).toBe(false) + }) + + it('closes an open tooltip when the text becomes empty', async () => { + const {container, rerender} = HTMLRender() + const tooltip = getTooltipElement(container) + await userEvent.tab() + expect(tooltip.matches(':popover-open')).toBe(true) + + rerender() + await expect.poll(() => tooltip.matches(':popover-open')).toBe(false) + expect(getTooltipElement(container)).toBe(tooltip) + }) + + it('keeps the trigger mounted and focused when the text is toggled', async () => { + function ConditionalTooltip() { + const [allowed, setAllowed] = useState(false) + return ( + + + + ) + } + const {container} = HTMLRender() + const button = page.getByRole('button', {name: 'Toggle', exact: true}) + const trigger = button.element() + const tooltip = getTooltipElement(container) + + await userEvent.tab() + expect(trigger).toHaveFocus() + expect(tooltip.matches(':popover-open')).toBe(true) + expect(trigger).toHaveAccessibleDescription('Not allowed') + + await userEvent.keyboard('{Enter}') + await expect.element(button).not.toHaveAttribute('aria-describedby') + await expect.poll(() => tooltip.matches(':popover-open')).toBe(false) + expect(button.element()).toBe(trigger) + expect(trigger).toHaveFocus() + + await userEvent.keyboard('{Enter}') + await expect.element(button).toHaveAccessibleDescription('Not allowed') + expect(button.element()).toBe(trigger) + expect(trigger).toHaveFocus() + }) +}) + describe('Tooltip data-component attributes', () => { it('renders Tooltip with data-component attribute', () => { const {getByText} = HTMLRender() From a187843e6340a149550e3470fdcd19ff70b75c38 Mon Sep 17 00:00:00 2001 From: Taylor Jones Date: Fri, 13 Nov 2026 16:05:30 -0600 Subject: [PATCH 2/2] fix(Tooltip): keep a pending hover open across empty text The empty-text effect cancelled the pending hover timer. If the text came back while the pointer was already over the trigger, no new mouseover fired, so the tooltip never opened. The effect now only hides an open popover. The hover timer reads the latest props when it fires: it opens if content has returned and does nothing if the text is still empty. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- packages/react/src/TooltipV2/Tooltip.tsx | 29 +++++++++----- .../src/TooltipV2/__tests__/Tooltip.test.tsx | 40 ++++++++++++++++++- 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx index 74c0bc14442..9e5d49cddf3 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -208,11 +208,7 @@ export const Tooltip: ForwardRefExoticComponent< } } } - const closeTooltip = () => { - if (openTimeoutRef.current) { - safeClearTimeout(openTimeoutRef.current) - openTimeoutRef.current = null - } + const hideTooltip = () => { try { if ( tooltipElRef.current && @@ -242,13 +238,24 @@ export const Tooltip: ForwardRefExoticComponent< } } - // Close the tooltip and cancel any pending open if its content is removed while it is open. This syncs the - // imperative popover state with props, so it can't move to an event handler. - const closeTooltipFromEffect = useEffectCallback(closeTooltip) + const closeTooltip = () => { + if (openTimeoutRef.current) { + safeClearTimeout(openTimeoutRef.current) + openTimeoutRef.current = null + } + hideTooltip() + } + + // A pending hover open outlives re-renders, so it must read the latest props when it fires. + const openTooltipFromTimeout = useEffectCallback(openTooltip) + + // Hide the tooltip if its content is removed while it is open. This syncs the imperative popover state with props, + // so it can't move to an event handler. A pending hover open is kept, since it checks the content when it fires. + const hideTooltipFromEffect = useEffectCallback(hideTooltip) useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler - if (!hasContent) closeTooltipFromEffect() - }, [hasContent, closeTooltipFromEffect]) + if (!hasContent) hideTooltipFromEffect() + }, [hasContent, hideTooltipFromEffect]) // context value const value = useMemo(() => ({tooltipId}), [tooltipId]) @@ -380,7 +387,7 @@ export const Tooltip: ForwardRefExoticComponent< openTimeoutRef.current = safeSetTimeout(() => { // if the mouse is already moved out, do not show the tooltip if (!openTimeoutRef.current) return - openTooltip() + openTooltipFromTimeout() child.props.onMouseEnter?.(event) }, delayTime) }, diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index 015f41fa8f2..315cead812b 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -3,7 +3,7 @@ import {describe, expect, it, vi} from 'vitest' import {page, userEvent} from 'vitest/browser' import type {TooltipProps} from '../Tooltip' import {Tooltip} from '../Tooltip' -import {fireEvent, render as HTMLRender} from '@testing-library/react' +import {act, fireEvent, render as HTMLRender} from '@testing-library/react' import BaseStyles from '../../BaseStyles' import {Button, IconButton} from '../../Button' import {ActionMenu} from '../../ActionMenu' @@ -287,6 +287,44 @@ describe('Tooltip with empty text', () => { expect(getTooltipElement(container)).toBe(tooltip) }) + it('opens from a pending hover when the text returns before the delay elapses', () => { + vi.useFakeTimers() + try { + const {container, rerender, getByRole} = HTMLRender() + const tooltip = getTooltipElement(container) + + fireEvent.mouseOver(getByRole('button')) + rerender() + rerender() + act(() => { + vi.advanceTimersByTime(100) + }) + + expect(tooltip.matches(':popover-open')).toBe(true) + expect(tooltip).toHaveTextContent('Back again') + } finally { + vi.useRealTimers() + } + }) + + it('does not open from a pending hover when the text is still empty after the delay elapses', () => { + vi.useFakeTimers() + try { + const {container, rerender, getByRole} = HTMLRender() + const tooltip = getTooltipElement(container) + + fireEvent.mouseOver(getByRole('button')) + rerender() + act(() => { + vi.advanceTimersByTime(100) + }) + + expect(tooltip.matches(':popover-open')).toBe(false) + } finally { + vi.useRealTimers() + } + }) + it('keeps the trigger mounted and focused when the text is toggled', async () => { function ConditionalTooltip() { const [allowed, setAllowed] = useState(false)