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

[iansan5653]

KeybindingHint renders the key symbols as aria-hidden spans and the key names as VisuallyHidden spans to provide a good experience for both sighted and screen reader users.

However, there is a bug in Chrome where aria-hidden text is included in a label generated using aria-labelledby. More context from https://github.com/github/accessibility/issues/8018#issuecomment-2751656621:

KeybindingHint (the Primer component we are using under the hood here) does try to account for this by rendering the symbol in an aria-hidden span and rendering the name of that symbol in an sr-only span next to it. However, there seems to be a bug in Chrome where the accessibility tree is incorrectly accounting for aria-hidden text in the target of an aria-labelledby. So we get the following (abridged) HTML:

... Add attachment ( command ⌘ shift ⇧ at @ ) This results in an incorrectly calculated accessible label of "Add attachment ( command ⌘ shift ⇧ at @ )".

If the label was being calculated correctly by the browser, we'd get a label of "Add attachment (command shift at)" (which looks wrong at first glance but sounds correct when read aloud by a screen reader - you don't say "command plus shift plus at" out loud).

Ideally we really want to fix this bug in the component so that consumers don't have to manually work around, so the approach used in primer/react#5626 works but it isn't ideal - it relies on consumers doing things correctly.

I don't think the way KeybindingHint itself works can really be improved much. If we were to update it to render the hidden keys and label next to each other, rather than mixed in, it would slightly improve what is being read but the data would still be duplicated. KeybindingHint's approach does work well in cases that don't involve aria-labelledby - when read as part of the document flow the output is correct.

I think the solution probably has to be in IconButton or TooltipV2. KeybindingHint does export a helper function that gives you the plain accessible string representation of a shortcut, so we could build the aria-label string ourselves rather than using aria-labelledby in IconButton. Or, a more robust approach might be to update TooltipV2 to move the id prop into a child element so that the tooltip could better control exactly what makes it into the label; then we could move KeybindingHint outside that element and add an sr-only element inside with just the string representation of the shortcut.

To work around this bug, I've taken the approach proposed in the quote to update TooltipV2 to exclude the keybinding hint from the id-tagged container. Then I added the keybinding hint text as a plain hidden string inside that container.

Before in VoiceOver/Chrome:

After:

Note this isn't a perfect example because obviously "question mark" is combined with "shift", but it's pulled from storybook so I just kept it. Intuitively you might think the label should be "shift + question mark", but when read by a screen reader this can result in the awkward label "shift plus question mark" - it's better to just exclude the punctuation here.

Changelog

New

Changed

Fixed a Chrome bug impacting accessible labels rendered by TooltipV2 with the keybindingHint prop.

Removed

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan
• None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

[changeset-bot]

🦋 Changeset detected

Latest commit: 6b96334

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

[github-actions]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	105.6 KB (+0.04% 🔺)
packages/react/dist/browser.umd.js	105.94 KB (+0.02% 🔺)

[iansan5653]

Note: This is indicative of the one potential breaking change I see for dotcom - we may need to update some test selectors to remove spaces around the keybinding hint in the labels. This is an intentional change.