diff --git a/.changeset/smart-chefs-fail.md b/.changeset/smart-chefs-fail.md new file mode 100644 index 00000000000..0622f8f544b --- /dev/null +++ b/.changeset/smart-chefs-fail.md @@ -0,0 +1,7 @@ +--- +'@primer/react': minor +--- + +IconButton: introduce tooltips on icon buttons behind the `unsafeDisableTooltip` prop for an incremental rollout. + +In the next release, we plan to update the default value of `unsafeDisableTooltip` to `false` so that the tooltip behaviour becomes the default. diff --git a/.playwright/snapshots/components/Dialogv1.test.ts-snapshots/Dialogv1-Default-dark-high-contrast-linux.png b/.playwright/snapshots/components/Dialogv1.test.ts-snapshots/Dialogv1-Default-dark-high-contrast-linux.png index 2b157f2f76b..dc89f8fc0d8 100644 Binary files a/.playwright/snapshots/components/Dialogv1.test.ts-snapshots/Dialogv1-Default-dark-high-contrast-linux.png and b/.playwright/snapshots/components/Dialogv1.test.ts-snapshots/Dialogv1-Default-dark-high-contrast-linux.png differ diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index b549e1b23e0..d33212d8a87 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -1,4 +1,4 @@ -import React from 'react' +import React, {useEffect, useState} from 'react' import {TriangleDownIcon} from '@primer/octicons-react' import type {AnchoredOverlayProps} from '../AnchoredOverlay' import {AnchoredOverlay} from '../AnchoredOverlay' @@ -147,6 +147,17 @@ const Overlay: React.FC> = ({ const containerRef = React.useRef(null) useMenuKeyboardNavigation(open, onClose, containerRef, anchorRef) + // If the menu anchor is an icon button, we need to label the menu by tooltip that also labelled the anchor. + const [anchorAriaLabelledby, setAnchorAriaLabelledby] = useState(null) + useEffect(() => { + if (anchorRef.current) { + const ariaLabelledby = anchorRef.current.getAttribute('aria-labelledby') + if (ariaLabelledby) { + setAnchorAriaLabelledby(ariaLabelledby) + } + } + }, [anchorRef]) + return ( > = ({ value={{ container: 'ActionMenu', listRole: 'menu', - listLabelledBy: ariaLabelledby || anchorId, + // If there is a custom aria-labelledby, use that. Otherwise, if exists, use the id that labels the anchor such as tooltip. If none of them exist, use anchor id. + listLabelledBy: ariaLabelledby || anchorAriaLabelledby || anchorId, selectionAttribute: 'aria-checked', // Should this be here? afterSelect: onClose, }} diff --git a/packages/react/src/Button/IconButton.dev.stories.tsx b/packages/react/src/Button/IconButton.dev.stories.tsx index 36e75d6b350..87b0c60a31c 100644 --- a/packages/react/src/Button/IconButton.dev.stories.tsx +++ b/packages/react/src/Button/IconButton.dev.stories.tsx @@ -7,7 +7,14 @@ export default { } export const CustomSize = () => ( - + ) export const CustomSizeWithMedia = () => { @@ -17,11 +24,19 @@ export const CustomSizeWithMedia = () => { variant="primary" size="small" icon={ChevronDownIcon} + unsafeDisableTooltip={false} sx={{'@media (min-width: 123px)': {width: 16, height: 16}}} /> ) } export const CustomIconColor = () => ( - + ) diff --git a/packages/react/src/Button/IconButton.features.stories.tsx b/packages/react/src/Button/IconButton.features.stories.tsx index 0f27d91c397..dc81b62bdfa 100644 --- a/packages/react/src/Button/IconButton.features.stories.tsx +++ b/packages/react/src/Button/IconButton.features.stories.tsx @@ -1,21 +1,90 @@ -import {HeartIcon} from '@primer/octicons-react' +import {HeartIcon, InboxIcon, ChevronDownIcon} from '@primer/octicons-react' import React from 'react' import {IconButton} from '.' +import {ActionMenu} from '../ActionMenu' +import {ActionList} from '../ActionList' +import {Tooltip} from '../TooltipV2' +import {default as TooltipV1} from '../Tooltip' export default { title: 'Components/IconButton/Features', } -export const Primary = () => +export const Primary = () => ( + +) -export const Danger = () => +export const Danger = () => ( + +) -export const Invisible = () => +export const Invisible = () => ( + +) -export const Disabled = () => +export const Disabled = () => ( + +) -export const Small = () => +export const Small = () => ( + +) -export const Medium = () => +export const Medium = () => ( + +) -export const Large = () => +export const Large = () => ( + +) + +export const WithDescription = () => ( + +) + +export const ExternalTooltip = () => ( + + + +) + +export const ExternalTooltipVersion1 = () => ( + + + +) + +export const AsAMenuAnchor = () => ( + + + + + + + + alert('Copy link clicked')}> + Copy link + ⌘C + + alert('Quote reply clicked')}> + Quote reply + ⌘Q + + alert('Edit comment clicked')}> + Edit comment + ⌘E + + + alert('Delete file clicked')}> + Delete file + ⌘D + + + + +) diff --git a/packages/react/src/Button/IconButton.stories.tsx b/packages/react/src/Button/IconButton.stories.tsx index 845fa4b5415..04c7dc64470 100644 --- a/packages/react/src/Button/IconButton.stories.tsx +++ b/packages/react/src/Button/IconButton.stories.tsx @@ -46,4 +46,4 @@ Playground.args = { icon: HeartIcon, } -export const Default = () => +export const Default = () => diff --git a/packages/react/src/Button/IconButton.tsx b/packages/react/src/Button/IconButton.tsx index c1d28df3840..d7b06d39db0 100644 --- a/packages/react/src/Button/IconButton.tsx +++ b/packages/react/src/Button/IconButton.tsx @@ -4,20 +4,75 @@ import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../uti import {ButtonBase} from './ButtonBase' import {defaultSxProp} from '../utils/defaultSxProp' import {generateCustomSxProp} from './Button' +import {TooltipContext, Tooltip} from '../TooltipV2/Tooltip' +import {TooltipContext as TooltipContextV1} from '../Tooltip/Tooltip' -const IconButton = forwardRef(({sx: sxProp = defaultSxProp, icon: Icon, ...props}, forwardedRef): JSX.Element => { - let sxStyles = sxProp - // grap the button props that have associated data attributes in the styles - const {size} = props +const IconButton = forwardRef( + ( + { + sx: sxProp = defaultSxProp, + icon: Icon, + 'aria-label': ariaLabel, + description, + disabled, + tooltipDirection, + // This is planned to be a temporary prop until the default tooltip on icon buttons are fully rolled out. + unsafeDisableTooltip = true, + ...props + }, + forwardedRef, + ): JSX.Element => { + let sxStyles = sxProp + // grap the button props that have associated data attributes in the styles + const {size} = props - if (sxProp !== null && Object.keys(sxProp).length > 0) { - sxStyles = generateCustomSxProp({size}, sxProp) - } + if (sxProp !== null && Object.keys(sxProp).length > 0) { + sxStyles = generateCustomSxProp({size}, sxProp) + } - return ( - // @ts-expect-error StyledButton wants both Anchor and Button refs - - ) -}) as PolymorphicForwardRefComponent<'button' | 'a', IconButtonProps> + // If the icon button is already wrapped in a tooltip, do not add one. + const {tooltipId} = React.useContext(TooltipContext) // Tooltip v2 + const {tooltipId: tooltipIdV1} = React.useContext(TooltipContextV1) // Tooltip v1 + + const hasExternalTooltip = tooltipId || tooltipIdV1 + const withoutTooltip = + unsafeDisableTooltip || disabled || ariaLabel === undefined || ariaLabel === '' || hasExternalTooltip + + if (withoutTooltip) { + return ( + + ) + } else { + return ( + + + + ) + } + }, +) as PolymorphicForwardRefComponent<'button' | 'a', IconButtonProps> export {IconButton} diff --git a/packages/react/src/Button/__tests__/Button.test.tsx b/packages/react/src/Button/__tests__/Button.test.tsx index 186f4a029a7..af116f09530 100644 --- a/packages/react/src/Button/__tests__/Button.test.tsx +++ b/packages/react/src/Button/__tests__/Button.test.tsx @@ -1,4 +1,4 @@ -import {SearchIcon} from '@primer/octicons-react' +import {SearchIcon, HeartIcon} from '@primer/octicons-react' import {render, screen, fireEvent} from '@testing-library/react' import {axe} from 'jest-axe' import React from 'react' @@ -113,4 +113,28 @@ describe('Button', () => { const position = screen.getByText('content').compareDocumentPosition(screen.getByTestId('trailingVisual')) expect(position).toBe(Node.DOCUMENT_POSITION_FOLLOWING) }) + + it('should render tooltip on an icon button when unsafeDisableTooltip prop is passed as false', () => { + const {getByRole, getByText} = render( + , + ) + const triggerEL = getByRole('button') + const tooltipEl = getByText('Heart') + expect(triggerEL).toHaveAttribute('aria-labelledby', tooltipEl.id) + }) + it('should render description type tooltip on an icon button when unsafeDisableTooltip prop is passed as false', () => { + const {getByRole, getByText} = render( + , + ) + const triggerEL = getByRole('button') + expect(triggerEL).toHaveAttribute('aria-label', 'Heart') + const tooltipEl = getByText('Love is all around') + expect(triggerEL).toHaveAttribute('aria-describedby', tooltipEl.id) + }) + it('should not render tooltip on an icon button by default', () => { + const {getByRole} = render() + const triggerEl = getByRole('button') + expect(triggerEl).not.toHaveAttribute('aria-labelledby') + expect(triggerEl).toHaveAttribute('aria-label', 'Heart') + }) }) diff --git a/packages/react/src/Button/types.ts b/packages/react/src/Button/types.ts index 5ddc0b221b9..c41e3a0e5c5 100644 --- a/packages/react/src/Button/types.ts +++ b/packages/react/src/Button/types.ts @@ -3,6 +3,7 @@ import styled from 'styled-components' import type {SxProp} from '../sx' import sx from '../sx' import getGlobalFocusStyles from '../internal/utils/getGlobalFocusStyles' +import type {TooltipDirection} from '../TooltipV2' export const StyledButton = styled.button` ${getGlobalFocusStyles('-2px')}; @@ -77,6 +78,9 @@ export type ButtonProps = { export type IconButtonProps = ButtonA11yProps & { icon: React.ElementType + unsafeDisableTooltip?: boolean + description?: string + tooltipDirection?: TooltipDirection } & Omit // adopted from React.AnchorHTMLAttributes diff --git a/packages/react/src/Tooltip/Tooltip.test.tsx b/packages/react/src/Tooltip/Tooltip.test.tsx index 7ef6ef466d8..61bb185d818 100644 --- a/packages/react/src/Tooltip/Tooltip.test.tsx +++ b/packages/react/src/Tooltip/Tooltip.test.tsx @@ -2,8 +2,9 @@ import React from 'react' import type {TooltipProps} from './Tooltip' import Tooltip from './Tooltip' import {render, renderClasses, rendersClass, behavesAsComponent, checkExports} from '../utils/testing' -import {render as HTMLRender} from '@testing-library/react' +import {render as HTMLRender, screen} from '@testing-library/react' import {axe} from 'jest-axe' +import {CodeIcon} from '@primer/octicons-react' /* Tooltip v1 */ @@ -49,4 +50,14 @@ describe('Tooltip', () => { it('respects the "wrap" prop', () => { expect(rendersClass(, 'tooltipped-multiline')).toBe(true) }) + it('should label the link', () => { + HTMLRender( + + + + + , + ) + expect(screen.getByRole('link')).toHaveAccessibleName('Tooltip text') + }) }) diff --git a/packages/react/src/Tooltip/Tooltip.tsx b/packages/react/src/Tooltip/Tooltip.tsx index c499c77d72b..1596d45fc73 100644 --- a/packages/react/src/Tooltip/Tooltip.tsx +++ b/packages/react/src/Tooltip/Tooltip.tsx @@ -1,10 +1,11 @@ import clsx from 'clsx' -import React from 'react' +import React, {useMemo} from 'react' import styled from 'styled-components' import {get} from '../constants' import type {SxProp} from '../sx' import sx from '../sx' import type {ComponentProps} from '../utils/types' +import {useId} from '../hooks' /* Tooltip v1 */ @@ -193,7 +194,9 @@ export type TooltipProps = { wrap?: boolean } & ComponentProps -function Tooltip({direction = 'n', children, className, text, noDelay, align, wrap, ...rest}: TooltipProps) { +export const TooltipContext = React.createContext<{tooltipId?: string}>({}) +function Tooltip({direction = 'n', children, className, text, noDelay, align, wrap, id, ...rest}: TooltipProps) { + const tooltipId = useId(id) const classes = clsx( className, `tooltipped-${direction}`, @@ -201,10 +204,15 @@ function Tooltip({direction = 'n', children, className, text, noDelay, align, wr noDelay && 'tooltipped-no-delay', wrap && 'tooltipped-multiline', ) + + const value = useMemo(() => ({tooltipId}), [tooltipId]) return ( - - {children} - + // This provider is used to check if an icon button is wrapped with tooltip or not. + + + {children} + + ) } diff --git a/packages/react/src/TooltipV2/Tooltip.examples.stories.tsx b/packages/react/src/TooltipV2/Tooltip.examples.stories.tsx index 827cbd66e65..337e5268761 100644 --- a/packages/react/src/TooltipV2/Tooltip.examples.stories.tsx +++ b/packages/react/src/TooltipV2/Tooltip.examples.stories.tsx @@ -1,7 +1,8 @@ -import React from 'react' +import React, {useState, useCallback, useRef} from 'react' import {Button, IconButton, Breadcrumbs, ActionMenu, ActionList, NavList} from '..' import {PageHeader} from '../PageHeader' import {Tooltip} from './Tooltip' +import {Dialog} from '../drafts' import { GitBranchIcon, KebabHorizontalIcon, @@ -190,3 +191,39 @@ export const Hyperlist = () => ( ) + +export const DialogTrigger = () => { + const [isOpen, setIsOpen] = useState(false) + const [secondOpen, setSecondOpen] = useState(false) + const buttonRef = useRef(null) + const onDialogClose = useCallback(() => setIsOpen(false), []) + const onSecondDialogClose = useCallback(() => setSecondOpen(false), []) + const openSecondDialog = useCallback(() => setSecondOpen(true), []) + return ( + <> + + setIsOpen(!isOpen)} icon={CheckIcon} aria-label="Check mark" /> + + {isOpen && ( + + The icon button that triggers the dialog, takes the focus back when the dialog is closed however the the + tooltip is not shown again if the dialog is closed with a mouse. Because the tooltip is shown only on + focus-visible. + {secondOpen && ( + + Hello world + + )} + + )} + + ) +} diff --git a/packages/react/src/TooltipV2/Tooltip.playground.stories.tsx b/packages/react/src/TooltipV2/Tooltip.playground.stories.tsx index d133e7f0748..106f636aae5 100644 --- a/packages/react/src/TooltipV2/Tooltip.playground.stories.tsx +++ b/packages/react/src/TooltipV2/Tooltip.playground.stories.tsx @@ -3,7 +3,7 @@ import {Button, Box} from '..' import {Tooltip} from './Tooltip' import type {Meta, StoryFn} from '@storybook/react' -export default { +const meta: Meta = { title: 'Components/TooltipV2/Playground', component: Tooltip, @@ -22,7 +22,9 @@ export default { control: false, }, }, -} as Meta +} + +export default meta // Description type, north direction by default export const Playground: StoryFn = args => { diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx index 07bfe8e102a..5023094eb88 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -1,4 +1,4 @@ -import React, {Children, useEffect, useRef, useState} from 'react' +import React, {Children, useEffect, useRef, useState, useMemo} from 'react' import type {SxProp} from '../sx' import sx from '../sx' import {useId, useProvidedRefOrCreate, useOnEscapePress} from '../hooks' @@ -123,7 +123,7 @@ const StyledTooltip = styled.span` ${sx}; ` -type TooltipDirection = 'nw' | 'n' | 'ne' | 'e' | 'se' | 's' | 'sw' | 'w' +export type TooltipDirection = 'nw' | 'n' | 'ne' | 'e' | 'se' | 's' | 'sw' | 'w' export type TooltipProps = React.PropsWithChildren< { direction?: TooltipDirection @@ -235,6 +235,9 @@ export const Tooltip = React.forwardRef( } } + // context value + const value = useMemo(() => ({tooltipId}), [tooltipId]) + useEffect(() => { if (!tooltipElRef.current || !triggerRef.current) return /* @@ -285,7 +288,7 @@ export const Tooltip = React.forwardRef( ) return ( - + <> {React.isValidElement(child) && React.cloneElement(child as React.ReactElement, { @@ -299,6 +302,14 @@ export const Tooltip = React.forwardRef( child.props.onBlur?.(event) }, onFocus: (event: React.FocusEvent) => { + // only show tooltip on :focus-visible, not on :focus + try { + if (!event.target.matches(':focus-visible')) return + } catch (error) { + // jsdom (jest) does not support `:focus-visible` yet and would throw an error + // https://github.com/jsdom/jsdom/issues/3426 + } + openTooltip() child.props.onFocus?.(event) }, diff --git a/packages/react/src/__tests__/ActionMenu.test.tsx b/packages/react/src/__tests__/ActionMenu.test.tsx index 1a933717348..8ec6fd70624 100644 --- a/packages/react/src/__tests__/ActionMenu.test.tsx +++ b/packages/react/src/__tests__/ActionMenu.test.tsx @@ -8,7 +8,7 @@ import {Tooltip as TooltipV2} from '../TooltipV2/Tooltip' import {behavesAsComponent, checkExports} from '../utils/testing' import {SingleSelect} from '../ActionMenu/ActionMenu.features.stories' import {MixedSelection} from '../ActionMenu/ActionMenu.examples.stories' -import {KebabHorizontalIcon} from '@primer/octicons-react' +import {SearchIcon, KebabHorizontalIcon} from '@primer/octicons-react' function Example(): JSX.Element { return ( @@ -435,4 +435,33 @@ describe('ActionMenu', () => { expect(button.id).toBe(buttonId) }) + it('should use the tooltip id to name the menu when the anchor is icon button', async () => { + const component = HTMLRender( + + + + + + + + + + + alert('Copy link clicked')}> + Copy link + ⌘C + + + + + + + , + ) + + const toggleButton = component.getByRole('button', {name: 'More actions'}) + await userEvent.click(toggleButton) + expect(toggleButton).toHaveAttribute('aria-labelledby') + expect(component.getByRole('menu')).toHaveAttribute('aria-labelledby', toggleButton.getAttribute('aria-labelledby')) + }) }) diff --git a/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap b/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap index 697fb58c1ad..c31d29b8352 100644 --- a/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap +++ b/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap @@ -312,6 +312,7 @@ exports[`@primer/react/drafts should not update exports without a semver change "type TitleProps", "Tooltip", "TooltipContext", + "type TooltipDirection", "type TooltipProps", "type Trigger", "type TriggerPropsType", @@ -401,6 +402,7 @@ exports[`@primer/react/experimental should not update exports without a semver c "type TitleProps", "Tooltip", "TooltipContext", + "type TooltipDirection", "type TooltipProps", "type Trigger", "type TriggerPropsType",