From 2ba474a81096310b6b1174b5f3c43e30398e8575 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Thu, 27 Mar 2025 14:32:11 +0000 Subject: [PATCH 01/10] Hide tooltips on touchend event --- packages/react/src/TooltipV2/Tooltip.tsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx index 9fe332b5dc8..fc2ce8effb7 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -151,6 +151,7 @@ type TriggerPropsType = { 'aria-labelledby'?: string 'aria-label'?: string onBlur?: React.FocusEventHandler + onTouchEnd?: React.TouchEventHandler onFocus?: React.FocusEventHandler onMouseEnter?: React.MouseEventHandler onMouseLeave?: React.MouseEventHandler @@ -362,6 +363,10 @@ export const Tooltip = React.forwardRef( closeTooltip() child.props.onBlur?.(event) }, + onTouchEnd: (event: React.TouchEvent) => { + closeTooltip() + child.props.onTouchEnd?.(event) + }, onFocus: (event: React.FocusEvent) => { // only show tooltip on :focus-visible, not on :focus try { From 6cfcb9aba3a968ba81362edd97374b34c6f00b79 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Thu, 27 Mar 2025 10:35:36 -0400 Subject: [PATCH 02/10] Create chilly-jars-jog.md --- .changeset/chilly-jars-jog.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/chilly-jars-jog.md diff --git a/.changeset/chilly-jars-jog.md b/.changeset/chilly-jars-jog.md new file mode 100644 index 00000000000..d652dee0216 --- /dev/null +++ b/.changeset/chilly-jars-jog.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Hide `TooltipV2` tooltips on `touchend` event From 2df9231dae07753a81ce9ceff4b3f67881288132 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Thu, 27 Mar 2025 14:40:20 +0000 Subject: [PATCH 03/10] Update snapshots --- .../react/src/__tests__/__snapshots__/TextInput.test.tsx.snap | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap b/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap index 3d19d97cca1..357c22a7342 100644 --- a/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap +++ b/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap @@ -3327,6 +3327,7 @@ exports[`TextInput renders trailingAction icon button 1`] = ` onFocus={[Function]} onMouseEnter={[Function]} onMouseLeave={[Function]} + onTouchEnd={[Function]} type="button" > Date: Thu, 27 Mar 2025 15:16:24 +0000 Subject: [PATCH 04/10] Use a timeout so we don't conflict with the focus event --- packages/react/src/TooltipV2/Tooltip.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx index fc2ce8effb7..cd12e1ce7f3 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -364,8 +364,9 @@ export const Tooltip = React.forwardRef( child.props.onBlur?.(event) }, onTouchEnd: (event: React.TouchEvent) => { - closeTooltip() child.props.onTouchEnd?.(event) + // setTimeout to take effect after the `focus` event + setTimeout(() => closeTooltip()) }, onFocus: (event: React.FocusEvent) => { // only show tooltip on :focus-visible, not on :focus From 8ee6f3e7f91be05168d380f24ad5de4009ea6ea5 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Fri, 28 Mar 2025 13:07:11 -0400 Subject: [PATCH 05/10] Add tests --- packages/react/src/TooltipV2/Tooltip.tsx | 45 +++++++++++-------- .../src/TooltipV2/__tests__/Tooltip.test.tsx | 29 +++++++++++- 2 files changed, 54 insertions(+), 20 deletions(-) diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx index cd12e1ce7f3..1c50c8f10bc 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -146,15 +146,18 @@ export type TooltipProps = React.PropsWithChildren< > & React.HTMLAttributes -type TriggerPropsType = { - 'aria-describedby'?: string - 'aria-labelledby'?: string - 'aria-label'?: string - onBlur?: React.FocusEventHandler - onTouchEnd?: React.TouchEventHandler - onFocus?: React.FocusEventHandler - onMouseEnter?: React.MouseEventHandler - onMouseLeave?: React.MouseEventHandler +type TriggerPropsType = Pick< + React.HTMLAttributes, + | 'aria-describedby' + | 'aria-labelledby' + | 'onBlur' + | 'onTouchEnd' + | 'onFocus' + | 'onMouseOverCapture' + | 'onMouseLeave' + | 'onTouchCancel' + | 'onTouchEnd' +> & { ref?: React.RefObject } @@ -215,7 +218,7 @@ export const Tooltip = React.forwardRef( const [isPopoverOpen, setIsPopoverOpen] = useState(false) - const timeoutRef = React.useRef(null) + const openTimeoutRef = React.useRef(null) const {safeSetTimeout, safeClearTimeout} = useSafeTimeout() @@ -262,6 +265,10 @@ export const Tooltip = React.forwardRef( } } const closeTooltip = () => { + if (openTimeoutRef.current) { + safeClearTimeout(openTimeoutRef.current) + openTimeoutRef.current = null + } try { if ( tooltipElRef.current && @@ -365,8 +372,10 @@ export const Tooltip = React.forwardRef( }, onTouchEnd: (event: React.TouchEvent) => { child.props.onTouchEnd?.(event) - // setTimeout to take effect after the `focus` event - setTimeout(() => closeTooltip()) + + // Hide tooltips on tap to essentially disable them on touch devices; + // this still allows viewing the tooltip on tap-and-hold + safeSetTimeout(() => closeTooltip(), 10) }, onFocus: (event: React.FocusEvent) => { // only show tooltip on :focus-visible, not on :focus @@ -380,19 +389,17 @@ export const Tooltip = React.forwardRef( openTooltip() child.props.onFocus?.(event) }, - onMouseEnter: (event: React.MouseEvent) => { - // show tooltip after mosue has been hovering for at least 50ms + onMouseOverCapture: (event: React.MouseEvent) => { + // We use a `capture` event to ensure this is called first before + // events that might cancel the opening timeout (like `onTouchEnd`) + // show tooltip after mouse has been hovering for at least 50ms // (prevent showing tooltip when mouse is just passing through) - timeoutRef.current = safeSetTimeout(() => { + openTimeoutRef.current = safeSetTimeout(() => { openTooltip() child.props.onMouseEnter?.(event) }, 50) }, onMouseLeave: (event: React.MouseEvent) => { - if (timeoutRef.current) { - safeClearTimeout(timeoutRef.current) - timeoutRef.current = null - } closeTooltip() child.props.onMouseLeave?.(event) }, diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index c019df7c042..e593fead729 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -2,7 +2,7 @@ import React from 'react' import type {TooltipProps} from '../Tooltip' import {Tooltip} from '../Tooltip' import {checkStoriesForAxeViolations} from '../../utils/testing' -import {render as HTMLRender} from '@testing-library/react' +import {act, fireEvent, render as HTMLRender} from '@testing-library/react' import theme from '../../theme' import {Button, IconButton, ActionMenu, ActionList, ThemeProvider, BaseStyles, ButtonGroup} from '../..' import {XIcon} from '@primer/octicons-react' @@ -33,6 +33,8 @@ function ExampleWithActionMenu(actionMenuTrigger: React.ReactElement): JSX.Eleme ) } +jest.useFakeTimers() + describe('Tooltip', () => { checkStoriesForAxeViolations('Tooltip.features', '../TooltipV2/') @@ -157,4 +159,29 @@ describe('Tooltip', () => { const triggerEL = getByText('Button 1') expect(triggerEL).toBeInTheDocument() }) + + it('should become visible after delay when the target is hovered over', async () => { + const {getByRole, getByText} = HTMLRender() + const triggerEL = getByRole('button') + const tooltip = getByText('Tooltip text') + expect(tooltip).not.toHaveClass(':popover-open') + fireEvent.mouseOver(triggerEL) + expect(tooltip).not.toHaveClass(':popover-open') + act(() => jest.advanceTimersByTime(50)) + expect(tooltip).toHaveClass(':popover-open') + }) + + it('should not become visible after delay when the target is tapped', async () => { + const {getByRole, getByText} = HTMLRender() + const triggerEL = getByRole('button') + const tooltip = getByText('Tooltip text') + expect(tooltip).not.toHaveClass(':popover-open') + // 'sort of' simulating a tap event here. MouseOver normally happens after + // touchEnd but we are 'capture'ing it in the component + fireEvent.mouseOver(triggerEL) + fireEvent.focus(triggerEL) + fireEvent.touchEnd(triggerEL) + act(() => jest.advanceTimersByTime(50)) + expect(tooltip).not.toHaveClass(':popover-open') + }) }) From 57b5e8e435a8e09d6ec6b4a9536e8bac506f2e1a Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 31 Mar 2025 14:23:29 +0000 Subject: [PATCH 06/10] Let's try one more thing before I give up on the test --- .../src/TooltipV2/__tests__/Tooltip.test.tsx | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index e593fead729..658dbde18c7 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -16,6 +16,14 @@ const TooltipComponent = (props: Omit & {text?: string}) = ) +function expectToBeOpen(tooltip: HTMLElement) { + expect(tooltip).toMatch(':popover-open, .:popover-open') +} + +function expectNotToBeOpen(tooltip: HTMLElement) { + expect(tooltip).not.toMatch(':popover-open, .:popover-open') +} + function ExampleWithActionMenu(actionMenuTrigger: React.ReactElement): JSX.Element { return ( @@ -164,24 +172,24 @@ describe('Tooltip', () => { const {getByRole, getByText} = HTMLRender() const triggerEL = getByRole('button') const tooltip = getByText('Tooltip text') - expect(tooltip).not.toHaveClass(':popover-open') + expectNotToBeOpen(tooltip) fireEvent.mouseOver(triggerEL) expect(tooltip).not.toHaveClass(':popover-open') act(() => jest.advanceTimersByTime(50)) - expect(tooltip).toHaveClass(':popover-open') + expectToBeOpen(tooltip) }) it('should not become visible after delay when the target is tapped', async () => { const {getByRole, getByText} = HTMLRender() const triggerEL = getByRole('button') const tooltip = getByText('Tooltip text') - expect(tooltip).not.toHaveClass(':popover-open') + expectNotToBeOpen(tooltip) // 'sort of' simulating a tap event here. MouseOver normally happens after // touchEnd but we are 'capture'ing it in the component fireEvent.mouseOver(triggerEL) fireEvent.focus(triggerEL) fireEvent.touchEnd(triggerEL) act(() => jest.advanceTimersByTime(50)) - expect(tooltip).not.toHaveClass(':popover-open') + expectNotToBeOpen(tooltip) }) }) From 71dcc3b94e6a58693362ec2afa94f310a8feef8a Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 31 Mar 2025 16:53:58 +0000 Subject: [PATCH 07/10] Remove test :( --- .../src/TooltipV2/__tests__/Tooltip.test.tsx | 35 +------------------ 1 file changed, 1 insertion(+), 34 deletions(-) diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index 658dbde18c7..21519374301 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -2,7 +2,7 @@ import React from 'react' import type {TooltipProps} from '../Tooltip' import {Tooltip} from '../Tooltip' import {checkStoriesForAxeViolations} from '../../utils/testing' -import {act, fireEvent, render as HTMLRender} from '@testing-library/react' +import {render as HTMLRender} from '@testing-library/react' import theme from '../../theme' import {Button, IconButton, ActionMenu, ActionList, ThemeProvider, BaseStyles, ButtonGroup} from '../..' import {XIcon} from '@primer/octicons-react' @@ -16,14 +16,6 @@ const TooltipComponent = (props: Omit & {text?: string}) = ) -function expectToBeOpen(tooltip: HTMLElement) { - expect(tooltip).toMatch(':popover-open, .:popover-open') -} - -function expectNotToBeOpen(tooltip: HTMLElement) { - expect(tooltip).not.toMatch(':popover-open, .:popover-open') -} - function ExampleWithActionMenu(actionMenuTrigger: React.ReactElement): JSX.Element { return ( @@ -167,29 +159,4 @@ describe('Tooltip', () => { const triggerEL = getByText('Button 1') expect(triggerEL).toBeInTheDocument() }) - - it('should become visible after delay when the target is hovered over', async () => { - const {getByRole, getByText} = HTMLRender() - const triggerEL = getByRole('button') - const tooltip = getByText('Tooltip text') - expectNotToBeOpen(tooltip) - fireEvent.mouseOver(triggerEL) - expect(tooltip).not.toHaveClass(':popover-open') - act(() => jest.advanceTimersByTime(50)) - expectToBeOpen(tooltip) - }) - - it('should not become visible after delay when the target is tapped', async () => { - const {getByRole, getByText} = HTMLRender() - const triggerEL = getByRole('button') - const tooltip = getByText('Tooltip text') - expectNotToBeOpen(tooltip) - // 'sort of' simulating a tap event here. MouseOver normally happens after - // touchEnd but we are 'capture'ing it in the component - fireEvent.mouseOver(triggerEL) - fireEvent.focus(triggerEL) - fireEvent.touchEnd(triggerEL) - act(() => jest.advanceTimersByTime(50)) - expectNotToBeOpen(tooltip) - }) }) From 707c8b29c08b1dc9f8caf4735bb0560b207bdf94 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 31 Mar 2025 17:11:43 +0000 Subject: [PATCH 08/10] Update snapshots --- .../react/src/__tests__/__snapshots__/TextInput.test.tsx.snap | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap b/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap index 357c22a7342..818ffd62114 100644 --- a/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap +++ b/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap @@ -3325,8 +3325,8 @@ exports[`TextInput renders trailingAction icon button 1`] = ` onBlur={[Function]} onClick={[MockFunction]} onFocus={[Function]} - onMouseEnter={[Function]} onMouseLeave={[Function]} + onMouseOverCapture={[Function]} onTouchEnd={[Function]} type="button" > @@ -4091,8 +4091,8 @@ exports[`TextInput renders trailingAction text button with a tooltip 1`] = ` onBlur={[Function]} onClick={[MockFunction]} onFocus={[Function]} - onMouseEnter={[Function]} onMouseLeave={[Function]} + onMouseOverCapture={[Function]} onTouchEnd={[Function]} style={ { From 7f7064d476524ad374c99bfa1a1ee2678dbaf20b Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 31 Mar 2025 17:17:18 +0000 Subject: [PATCH 09/10] Remove useFakeTimers call --- packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index 21519374301..c019df7c042 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -33,8 +33,6 @@ function ExampleWithActionMenu(actionMenuTrigger: React.ReactElement): JSX.Eleme ) } -jest.useFakeTimers() - describe('Tooltip', () => { checkStoriesForAxeViolations('Tooltip.features', '../TooltipV2/') From 5d4987db0e1c8fb75c3c202e9de1ba66f8b2ae91 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Thu, 3 Apr 2025 14:48:19 -0400 Subject: [PATCH 10/10] Update .changeset/chilly-jars-jog.md Co-authored-by: Jon Rohan --- .changeset/chilly-jars-jog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/chilly-jars-jog.md b/.changeset/chilly-jars-jog.md index d652dee0216..cb414e294b4 100644 --- a/.changeset/chilly-jars-jog.md +++ b/.changeset/chilly-jars-jog.md @@ -1,5 +1,5 @@ --- -"@primer/react": patch +"@primer/react": minor --- Hide `TooltipV2` tooltips on `touchend` event