diff --git a/.changeset/curvy-ties-breathe.md b/.changeset/curvy-ties-breathe.md new file mode 100644 index 00000000000..7d29df58b73 --- /dev/null +++ b/.changeset/curvy-ties-breathe.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Add workaround for Chrome bug where `KeybindingHint` in combination with `aria-labelledby` results in incorrect label diff --git a/packages/react/src/Button/__tests__/Button.test.tsx b/packages/react/src/Button/__tests__/Button.test.tsx index a2cdb05f674..79189ba1b73 100644 --- a/packages/react/src/Button/__tests__/Button.test.tsx +++ b/packages/react/src/Button/__tests__/Button.test.tsx @@ -338,7 +338,7 @@ describe('Button', () => { }) it('should append the keyshortcuts to the tooltip text that labels the icon button when keyshortcuts prop is passed', () => { const {getByRole} = render() - const triggerEl = getByRole('button', {name: 'Heart ( command h )'}) + const triggerEl = getByRole('button', {name: 'Heart (command h)'}) expect(triggerEl).toBeInTheDocument() }) it('should render aria-keyshortcuts on an icon button when keyshortcuts prop is passed (Description Type)', () => { @@ -353,6 +353,6 @@ describe('Button', () => { , ) const triggerEl = getByRole('button', {name: 'Heart'}) - expect(triggerEl).toHaveAccessibleDescription('Love is all around ( command h )') + expect(triggerEl).toHaveAccessibleDescription('Love is all around (command h)') }) }) diff --git a/packages/react/src/KeybindingHint/KeybindingHint.tsx b/packages/react/src/KeybindingHint/KeybindingHint.tsx index c7c9a70995a..0e8354fe8d5 100644 --- a/packages/react/src/KeybindingHint/KeybindingHint.tsx +++ b/packages/react/src/KeybindingHint/KeybindingHint.tsx @@ -26,8 +26,7 @@ export const KeybindingHint = memo(({className, ...props}: KeybindingHintProps) KeybindingHint.displayName = 'KeybindingHint' /** - * AVOID: `KeybindingHint` is nearly always sufficient for providing both visible and accessible keyboard hints, and - * will result in a good screen reader experience when used as the target for `aria-describedby` and `aria-labelledby`. + * AVOID: `KeybindingHint` is nearly always sufficient for providing both visible and accessible keyboard hints. * However, there may be cases where we need a plain string version, such as when building `aria-label` or * `aria-description`. In that case, this plain string builder can be used instead. * diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx index 9fe332b5dc8..ba9e9dbb746 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -1,7 +1,7 @@ import React, {Children, useEffect, useRef, useState, useMemo} from 'react' import type {SxProp} from '../sx' import sx from '../sx' -import {useId, useProvidedRefOrCreate, useOnEscapePress} from '../hooks' +import {useId, useProvidedRefOrCreate, useOnEscapePress, useIsMacOS} from '../hooks' import {invariant} from '../utils/invariant' import {warning} from '../utils/warning' import styled from 'styled-components' @@ -13,7 +13,7 @@ import {toggleStyledComponent} from '../internal/utils/toggleStyledComponent' import {clsx} from 'clsx' import classes from './Tooltip.module.css' import {useFeatureFlag} from '../FeatureFlags' -import {KeybindingHint, type KeybindingHintProps} from '../KeybindingHint' +import {getAccessibleKeybindingHintString, KeybindingHint, type KeybindingHintProps} from '../KeybindingHint' import VisuallyHidden from '../_VisuallyHidden' import useSafeTimeout from '../hooks/useSafeTimeout' @@ -348,6 +348,8 @@ export const Tooltip = React.forwardRef( [isPopoverOpen], ) + const isMacOS = useIsMacOS() + return ( <> @@ -400,17 +402,24 @@ export const Tooltip = React.forwardRef( role={type === 'description' ? 'tooltip' : undefined} // stop AT from announcing the tooltip twice: when it is a label type it will be announced with "aria-labelledby",when it is a description type it will be announced with "aria-describedby" aria-hidden={true} - id={tooltipId} // mouse leave and enter on the tooltip itself is needed to keep the tooltip open when the mouse is over the tooltip onMouseEnter={openTooltip} onMouseLeave={closeTooltip} > - {text} + + {text} + {/* There is a bug in Chrome browsers where `aria-hidden` text inside the target of an `aria-labelledby` + still gets included in the accessible label. `KeybindingHint` renders the symbols as `aria-hidden` text + and renders full key names as `VisuallyHidden` text. Due to the browser bug this causes the label text + to duplicate the symbols and key names. To work around this, we exclude the hint from being part of the + label and instead render the plain keybinding description string. */} + {keybindingHint && ( + ({getAccessibleKeybindingHintString(keybindingHint, isMacOS)}) + )} + {keybindingHint && ( - - ( + - ) )} diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index c019df7c042..71bccb68fd1 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -38,11 +38,11 @@ describe('Tooltip', () => { it('renders `data-direction="s"` by default', () => { const {getByText} = HTMLRender() - expect(getByText('Tooltip text')).toHaveAttribute('data-direction', 's') + expect(getByText('Tooltip text').closest('[role=tooltip]')).toHaveAttribute('data-direction', 's') }) it('renders `data-direction` attribute with the correct value when the `direction` prop is specified', () => { const {getByText} = HTMLRender() - expect(getByText('Tooltip text')).toHaveAttribute('data-direction', 'n') + expect(getByText('Tooltip text').closest('[role=tooltip]')).toHaveAttribute('data-direction', 'n') }) it('should label the trigger element by its tooltip when the tooltip type is label', () => { const {getByRole, getByText} = HTMLRender() @@ -52,11 +52,11 @@ describe('Tooltip', () => { }) it('should render aria-hidden on the tooltip element when the tooltip is label type', () => { const {getByText} = HTMLRender() - expect(getByText('Tooltip text')).toHaveAttribute('aria-hidden', 'true') + expect(getByText('Tooltip text').parentElement).toHaveAttribute('aria-hidden', 'true') }) it('should render aria-hidden on the tooltip element when the tooltip is description type', () => { const {getByText} = HTMLRender() - expect(getByText('Tooltip text')).toHaveAttribute('aria-hidden', 'true') + expect(getByText('Tooltip text').closest('[role=tooltip]')).toHaveAttribute('aria-hidden', 'true') }) it('should describe the trigger element by its tooltip when the tooltip type is description (by default)', () => { const {getByRole, getByText} = HTMLRender() @@ -66,7 +66,7 @@ describe('Tooltip', () => { }) it('should render the tooltip element with role="tooltip" when the tooltip type is description (by default)', () => { const {getByText} = HTMLRender() - expect(getByText('Tooltip text')).toHaveAttribute('role', 'tooltip') + expect(getByText('Tooltip text').closest('[role=tooltip]')).toHaveAttribute('role', 'tooltip') }) it('should spread the accessibility attributes correctly on the trigger (ActionMenu.Button) when tooltip is used in an action menu', () => { diff --git a/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap b/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap index 3d19d97cca1..5ddf224c971 100644 --- a/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap +++ b/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap @@ -3354,11 +3354,14 @@ exports[`TextInput renders trailingAction icon button 1`] = ` aria-hidden={true} className="c5" data-direction="s" - id=":r2j:" onMouseEnter={[Function]} onMouseLeave={[Function]} > - Icon label + + Icon label + @@ -4115,12 +4118,15 @@ exports[`TextInput renders trailingAction text button with a tooltip 1`] = ` aria-hidden={true} className="c6" data-direction="s" - id=":r2d:" onMouseEnter={[Function]} onMouseLeave={[Function]} role="tooltip" > - Clear input + + Clear input +