Tooltip keybinding hints

[iansan5653]

Closes #4759

Currently we have minimal support for keybinding hints on IconButton and TooltipV2 by simply rendering the shortcuts as a string separated by a comma. But now that we have a KeybindingHint component that renders visually nice keybinding hints, there's no reason we can't migrate to the new component and get a much nicer visual presentation.

This PR does that, updating TooltipV2 to add a keybindingHint prop that renders the hint using KeybindingHint at the end of the tooltip:

In addition, this also requires updating IconButton to take advantage of the new functionality. IconButton does already have keyshortcuts, which assigns the aria-keyshortcuts attribute and also adds the shortcut to the label. For consistency, and considering that this prop is entirely unused in github/github, I decided to rename this to keybindingHint. I think this more explicitly implies that the prop is not binding a shortcut, it's just rendering a visual hint. It also better ties the prop to the underlying component, which does have some requirements about the format of the string.

Accessibility concerns

aria-keyshortcuts

Currently we are still sending the keybinding hint string to the aria-keyshortcuts prop in IconButton. I'm not sure that we actually should keep doing this, because this causes the hint to be presented in both the label and the ARIA attribute, which could be annoying / repetitive. We could alternatively remove the hint from the label, but support for aria-keyshortcuts is not universal. In addition, the format of the string expected by KeybindingHint does not necessarily match the format expected by aria-keyshortcuts.

aria-label bug

There is one known bug with rendering KeybindingHint as part of the label. Normally, the plain-text representation of KeybindingHint is actually very good - it replaces the symbolic key icons with full plain-text key names to ensure that screen readers pronounce them accurately. However, there is a bug in Chrome that causes aria-hidden regions inside popovers to become part of the generated accessible name, which means that the symbolic key icons are incorrectly becoming part of the name.

So, for example, the accessible name for the bold button pictured above is Bold (command ⌘ b B) instead of the expected Bold (command b) 😞.

We could work around this with some effort, but I'm not sure that it's worth it since this is a bug in Chrome (does not reproduce in Safari) and is not excessively disruptive. If we do feel that this is a blocker I can spend a bit more time trying to come up with a workaround. We'd probably have to exclude the KeybindingHint from the accessible name by moving the aria-labelledby target to only wrap some of the tooltip contents, and then append a visually hidden, accessible plain-text version of the hint onto the name instead. Not terrible but not trivial either.

Changelog

New

• Adds formal support for rendering keybinding hints in TooltipV2 through the new keybindingHint prop, which renders a KeybindingHint component under the hood

Changed

• Renames the keyshortcuts prop on IconButton to keybindingHint (deprecating the old prop name) and updates it to render a KeybindingHint component instead of a plain string

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: 3335c4c

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	Minor

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]

Uh oh! @iansan5653, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

[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]

Uh oh! @iansan5653, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

[iansan5653]

The bot is totally wrong, my images do have alt text. Strange 😕

Also wow that's a lot of Actions comments 😄

[github-actions]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	102.49 KB (+1.13% 🔺)
packages/react/dist/browser.umd.js	102.85 KB (+1.31% 🔺)

[maximedegreve]

@iansan5653 So excited to see this added. 🎉

Could we add a few tweaks in?

• Set a min-width of 16px on the keyboard shortcut box
• Set padding left and right to 3px (this doesn't exist in our tokens so it will have to be hardcoded)
• Set padding top and bottom to 2px
• System font: 11px. I think that's already correct.
• Display the shortcuts always as uppercase
• Use the counter bg color to ensure enough contrast and remove the border line
• Border radius: 3px
• When there is a tooltip set the padding right to 6px instead of 8px to ensure consistent space around the bounding box on the right.
• Could we automatically translate ⌘ to Ctrl for Windows?

🖌️ Figma

[iansan5653]

I'm happy to make those changes (I think that design looks much better!) however as I mentioned on the call that does increase the 'blast radius' of this PR by impacting existing uses of KeybindingHint. The component is experimental, however, and not used in very many places, so I'm not too concerned about that.

There are very few places where this component is currently used in dotcom, since it's still relatively new.

---

I actually think the bigger risk than breaking existing usages may just be introducing yet another design for keyboard shortcut hints. We already have so much disparity because there's no 'official' Primer component for this.

Currently there are two distinct designs:

• The 'raised' style used mainly in Markdown, the keyboard shortcuts hint dialog, and documentation. Also scattered through some other places because it's the default CSS for kbd elements
• The 'outlined' style used mainly in React and in some keyboard shortcut hints in menus and apps. Probably not as common as the raised styling but fits much better in the UI.

Take the PR homepage for example - on the same screen you have two different designs used for the same thing 🙃:

Given that very few places are using the React component now, introducing yet another design might just make the problem worse by eventually having three different designs on the same view.

---

I think we need a broader initiative here to standardize this design and make it work both in tooltips and in menus/text/etc. And use it everywhere except maybe in user-generated Markdown content.

Also worth noting it looks like you have a different font size for tooltips in your designs than what is currently implemented in the Tooltip component.

[iansan5653]

Update: Per our discussion in Slack, I've made two changes:

• Updated the onEmphasis variant to use a solid background and no border
• Added a size prop with a small option for rendering in places like tooltips

You can see the changes in Storybook:

• Updated onEmphasis design
• New small size
• Updated tooltip combination
• Updated primary button combination

NOTE: I don't think the updated primary button design looks great 😕 . The problem is the background color token (from counter) looks great against tooltip backgrounds but not against button backgrounds. We could just ignore this, since it's very uncommon in reality. Or we could introduce a third variant for the new design (filled maybe?) instead of onEmphasis. Or we could say the tooltip design is a one-off and just use style overrides for it, keeping the outline design everywhere else. Or something else?

[TylerJDev]

LGTM! One small note, but non-blocking for this PR!

[TylerJDev]

I noticed VO announced shift twice due to the usage of ⇧. Similar behavior in NVDA, but NVDA announces the icon's description rather than shift.

I see this was hidden via aria-hidden, but still announces due to the tooltip referencing it via aria-describedby, which nullifies the default behavior of it being ignored. Not a big blocker since it still conveys the meaning, just with added redundancy. Wonder if we'd want to address this somehow in KeyboardingHint.

[iansan5653]

Is this in Chrome? I noticed this bug and I think it specifically has something to do with the popover. Unfortunately as it's a browser bug I was unable to find a good workaround

[TylerJDev]

Yup was chrome for VO, and Firefox for NVDA. Thanks for the context too! I'll put some time into looking into it more, but it's not a blocker for shipping this PR 🚢

[iansan5653]

😕 Not sure why all the CI is failing or what I can do about it. I assumed it was snapshots but adding the label didn't seem to help

[TylerJDev]

😕 Not sure why all the CI is failing or what I can do about it. I assumed it was snapshots but adding the label didn't seem to help

I'm thinking this might be caused by the upgrade to Playwright from 1.47.2 to 1.49.0 🤔 I think there's a few extra steps we need to take to upgrade it (context PR). Do we need to bump it in this PR, or could we open a separate PR with the bump?

[iansan5653]

Do we need to bump it in this PR

🤔 I think my VSCode extension did that automatically. I'll revert and see if it helps