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..bf0a9bd7d91 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,9 @@ export const Tooltip = React.forwardRef( [isPopoverOpen], ) + const isMacOS = useIsMacOS() + const hasAriaLabel = 'aria-label' in rest + return ( <> @@ -400,18 +403,29 @@ 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} + // If there is an aria-label prop, always assign the ID to the parent so the accessible label can be overridden + id={hasAriaLabel || !keybindingHint ? tooltipId : undefined} > - {text} - {keybindingHint && ( - - ( - - ) - + {keybindingHint ? ( + <> + + {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. */} + ({getAccessibleKeybindingHintString(keybindingHint, isMacOS)}) + + + + + + ) : ( + text )} diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index c019df7c042..605fae4d4a7 100644 --- a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx +++ b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx @@ -157,4 +157,14 @@ describe('Tooltip', () => { const triggerEL = getByText('Button 1') expect(triggerEL).toBeInTheDocument() }) + it('includes keybinding hints in the label text', () => { + const {getByRole} = HTMLRender() + expect(getByRole('button', {name: 'Tooltip text (control k)'})).toBeInTheDocument() + }) + it('allows overriding the accessible label with aria-label', () => { + const {getByRole} = HTMLRender( + , + ) + expect(getByRole('button', {name: 'Overridden label'})).toBeInTheDocument() + }) })