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..9e5d49cddf3 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 @@ -197,11 +208,7 @@ export const Tooltip: ForwardRefExoticComponent< } } } - const closeTooltip = () => { - if (openTimeoutRef.current) { - safeClearTimeout(openTimeoutRef.current) - openTimeoutRef.current = null - } + const hideTooltip = () => { try { if ( tooltipElRef.current && @@ -231,6 +238,25 @@ export const Tooltip: ForwardRefExoticComponent< } } + 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) hideTooltipFromEffect() + }, [hasContent, hideTooltipFromEffect]) + // context value const value = useMemo(() => ({tooltipId}), [tooltipId]) @@ -302,9 +328,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 +339,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 +354,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) @@ -364,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 46ca86f8d88..315cead812b 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -1,8 +1,9 @@ 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' +import {act, fireEvent, render as HTMLRender} from '@testing-library/react' import BaseStyles from '../../BaseStyles' import {Button, IconButton} from '../../Button' import {ActionMenu} from '../../ActionMenu' @@ -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,123 @@ 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('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) + 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()