From 565a64425f934113c86710955ddb4bf224a936c7 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 31 Mar 2025 12:44:41 -0400 Subject: [PATCH 1/4] =?UTF-8?q?Revert=20"Revert=20"Add=20workaround=20for?= =?UTF-8?q?=20Chrome=20bug=20where=20`KeybindingHint`=20in=20combi?= =?UTF-8?q?=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit eb134ce6e4aefa9357c57fde16adf11218d52946. --- .changeset/curvy-ties-breathe.md | 5 ++++ .../src/Button/__tests__/Button.test.tsx | 4 ++-- .../src/KeybindingHint/KeybindingHint.tsx | 3 +-- packages/react/src/TooltipV2/Tooltip.tsx | 23 +++++++++++++------ .../src/TooltipV2/__tests__/Tooltip.test.tsx | 10 ++++---- .../__snapshots__/TextInput.test.tsx.snap | 14 +++++++---- 6 files changed, 39 insertions(+), 20 deletions(-) create mode 100644 .changeset/curvy-ties-breathe.md 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 + From 3ee65bf17fce7dc3a7f682fca21982724e72225b Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 31 Mar 2025 17:41:07 +0000 Subject: [PATCH 2/4] Fix label overriding and add more tests --- packages/react/src/TooltipV2/Tooltip.tsx | 27 +++++++++++-------- .../src/TooltipV2/__tests__/Tooltip.test.tsx | 10 +++++++ 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx index ba9e9dbb746..bf0a9bd7d91 100644 --- a/packages/react/src/TooltipV2/Tooltip.tsx +++ b/packages/react/src/TooltipV2/Tooltip.tsx @@ -349,6 +349,7 @@ export const Tooltip = React.forwardRef( ) const isMacOS = useIsMacOS() + const hasAriaLabel = 'aria-label' in rest return ( @@ -405,22 +406,26 @@ export const Tooltip = React.forwardRef( // 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} - {/* There is a bug in Chrome browsers where `aria-hidden` text inside the target of an `aria-labelledby` + {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. */} - {keybindingHint && ( - ({getAccessibleKeybindingHintString(keybindingHint, isMacOS)}) - )} - - {keybindingHint && ( - - - + ({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 71bccb68fd1..c79f55dab44 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() + }) }) From 26d1176f2f2e25cd673fb13e188c76fdd63b7ace Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 31 Mar 2025 17:49:09 +0000 Subject: [PATCH 3/4] Update tests --- .../react/src/TooltipV2/__tests__/Tooltip.test.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx b/packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx index c79f55dab44..605fae4d4a7 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').closest('[role=tooltip]')).toHaveAttribute('data-direction', 's') + expect(getByText('Tooltip text')).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').closest('[role=tooltip]')).toHaveAttribute('data-direction', 'n') + expect(getByText('Tooltip text')).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').parentElement).toHaveAttribute('aria-hidden', 'true') + expect(getByText('Tooltip text')).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').closest('[role=tooltip]')).toHaveAttribute('aria-hidden', 'true') + expect(getByText('Tooltip text')).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').closest('[role=tooltip]')).toHaveAttribute('role', 'tooltip') + expect(getByText('Tooltip text')).toHaveAttribute('role', 'tooltip') }) it('should spread the accessibility attributes correctly on the trigger (ActionMenu.Button) when tooltip is used in an action menu', () => { From 1d81179d8b2659404bcb17ac0ad64ec1697e1c82 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 31 Mar 2025 17:49:50 +0000 Subject: [PATCH 4/4] Update snapshots --- .../__snapshots__/TextInput.test.tsx.snap | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap b/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap index 5ddf224c971..3d19d97cca1 100644 --- a/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap +++ b/packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap @@ -3354,14 +3354,11 @@ 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 @@ -4118,15 +4115,12 @@ 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