Revert "Revert "Add workaround for Chrome bug where KeybindingHint in combination with aria-labelledby results in incorrect label""

.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

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(<IconButton icon={HeartIcon} aria-label="Heart" keyshortcuts="Command+H" />)
-    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', () => {
       <IconButton icon={HeartIcon} aria-label="Heart" description="Love is all around" keyshortcuts="Command+H" />,
     )
     const triggerEl = getByRole('button', {name: 'Heart'})
-    expect(triggerEl).toHaveAccessibleDescription('Love is all around ( command h )')
+    expect(triggerEl).toHaveAccessibleDescription('Love is all around (command h)')
   })
 })

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.
  *

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 (
       <TooltipContext.Provider value={value}>
         <>
@@ -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 && (
-              <span className={clsx(classes.KeybindingHintContainer, text && classes.HasTextBefore)}>
-                <VisuallyHidden>(</VisuallyHidden>
-                <KeybindingHint keys={keybindingHint} format="condensed" variant="onEmphasis" size="small" />
-                <VisuallyHidden>)</VisuallyHidden>
-              </span>
+            {keybindingHint ? (
+              <>
+                <span id={hasAriaLabel ? undefined : tooltipId}>
+                  {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. */}
+                  <VisuallyHidden>({getAccessibleKeybindingHintString(keybindingHint, isMacOS)})</VisuallyHidden>
+                </span>
+                <span className={clsx(classes.KeybindingHintContainer, text && classes.HasTextBefore)} aria-hidden>
+                  <KeybindingHint keys={keybindingHint} format="condensed" variant="onEmphasis" size="small" />
+                </span>
+              </>
+            ) : (
+              text
             )}
           </StyledTooltip>
         </>

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(<TooltipComponent type="label" keybindingHint="Control+K" />)
+    expect(getByRole('button', {name: 'Tooltip text (control k)'})).toBeInTheDocument()
+  })
+  it('allows overriding the accessible label with aria-label', () => {
+    const {getByRole} = HTMLRender(
+      <TooltipComponent type="label" keybindingHint="Control+K" aria-label="Overridden label" />,
+    )
+    expect(getByRole('button', {name: 'Overridden label'})).toBeInTheDocument()
+  })
 })