Tooltip keybinding hints #5252

iansan5653 · 2024-11-08T19:42:30Z

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:

'Help' icon link with tooltip, hinting at question mark keyboard shortcut

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.

'Bold' icon button with command+b shortcut hint in tooltip

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 · 2024-11-08T19:42:33Z

🦋 Changeset detected

Latest commit: 41f8e01

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 · 2024-11-08T19:42:46Z

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 · 2024-11-08T19:43:01Z

👋 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 · 2024-11-08T19:43:16Z

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 · 2024-11-08T19:44:18Z

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

Also wow that's a lot of Actions comments 😄

github-actions · 2024-11-08T19:45:41Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	101.79 KB (+1.24% 🔺)
packages/react/dist/browser.umd.js	102.17 KB (+1.15% 🔺)

maximedegreve · 2024-11-11T12:07:24Z

@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 · 2024-11-11T14:46:24Z

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 · 2024-11-12T14:53:11Z

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:

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 · 2024-11-20T21:08:00Z

packages/react/src/TooltipV2/Tooltip.tsx

+            {keybindingHint && (
+              <span className={clsx(classes.keybindingHintContainer, text && classes.hasTextBefore)}>
+                <VisuallyHidden>(</VisuallyHidden>
+                <KeybindingHint keys={keybindingHint} format="condensed" variant="onEmphasis" size="small" />
+                <VisuallyHidden>)</VisuallyHidden>
+              </span>
+            )}


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.

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

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 · 2024-11-20T21:27:30Z

😕 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 · 2024-11-21T14:15:41Z

😕 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 added 3 commits November 8, 2024 18:44

Make keybinding hint font-size dynamic to parent font-size

156a2f4

Add keybinding hint support to Tooltip

faf6f1e

Move IconButton to keybindingHint

c462f35

iansan5653 requested a review from a team as a code owner November 8, 2024 19:42

iansan5653 requested a review from TylerJDev November 8, 2024 19:42

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Nov 8, 2024

iansan5653 temporarily deployed to github-pages November 8, 2024 19:44 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5252 November 8, 2024 19:44 Inactive

Add small size and update design of onEmphasis variant

c0f57a1

iansan5653 temporarily deployed to github-pages November 12, 2024 14:06 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5252 November 12, 2024 14:06 Inactive

Fix unit tests

db782fa

iansan5653 temporarily deployed to github-pages November 12, 2024 14:59 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5252 November 12, 2024 15:00 Inactive

Add changeset

b30905d

iansan5653 temporarily deployed to github-pages November 12, 2024 15:02 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5252 November 12, 2024 15:02 Inactive

iansan5653 added the update snapshots label Nov 12, 2024

Merge branch 'main' into tooltip-keybinding-hints

51a64d9

iansan5653 temporarily deployed to github-pages November 20, 2024 20:17 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5252 November 20, 2024 20:18 Inactive

Update story IDs in JSON

07a837c

lesliecdubs requested a review from maximedegreve November 20, 2024 20:44

iansan5653 temporarily deployed to github-pages November 20, 2024 20:44 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5252 November 20, 2024 20:44 Inactive

Update test config

38499b2

iansan5653 temporarily deployed to github-pages November 20, 2024 20:54 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5252 November 20, 2024 20:55 Inactive

Reduce right spacing

31096ac

iansan5653 temporarily deployed to github-pages November 20, 2024 21:00 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5252 November 20, 2024 21:01 Inactive

TylerJDev approved these changes Nov 20, 2024

View reviewed changes

Still not quite right on the spacing...

41f8e01

iansan5653 deployed to github-pages November 20, 2024 21:31 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-5252 November 20, 2024 21:31 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tooltip keybinding hints #5252

Tooltip keybinding hints #5252

iansan5653 commented Nov 8, 2024 •

edited

Loading

changeset-bot bot commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024

github-actions bot commented Nov 8, 2024

github-actions bot commented Nov 8, 2024

iansan5653 commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading

maximedegreve commented Nov 11, 2024 •

edited

Loading

iansan5653 commented Nov 11, 2024 •

edited

Loading

iansan5653 commented Nov 12, 2024

TylerJDev left a comment

TylerJDev Nov 20, 2024

iansan5653 Nov 20, 2024

TylerJDev Nov 21, 2024

iansan5653 commented Nov 20, 2024

TylerJDev commented Nov 21, 2024 •

edited

Loading

Tooltip keybinding hints #5252

Are you sure you want to change the base?

Tooltip keybinding hints #5252

Conversation

iansan5653 commented Nov 8, 2024 • edited Loading

Accessibility concerns

aria-keyshortcuts

aria-label bug

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Nov 8, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Nov 8, 2024

github-actions bot commented Nov 8, 2024

github-actions bot commented Nov 8, 2024

iansan5653 commented Nov 8, 2024 • edited Loading

github-actions bot commented Nov 8, 2024 • edited Loading

size-limit report 📦

maximedegreve commented Nov 11, 2024 • edited Loading

iansan5653 commented Nov 11, 2024 • edited Loading

iansan5653 commented Nov 12, 2024

TylerJDev left a comment

Choose a reason for hiding this comment

TylerJDev Nov 20, 2024

Choose a reason for hiding this comment

iansan5653 Nov 20, 2024

Choose a reason for hiding this comment

TylerJDev Nov 21, 2024

Choose a reason for hiding this comment

iansan5653 commented Nov 20, 2024

TylerJDev commented Nov 21, 2024 • edited Loading

iansan5653 commented Nov 8, 2024 •

edited

Loading

`aria-keyshortcuts`

`aria-label` bug

changeset-bot bot commented Nov 8, 2024 •

edited

Loading

iansan5653 commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading

maximedegreve commented Nov 11, 2024 •

edited

Loading

iansan5653 commented Nov 11, 2024 •

edited

Loading

TylerJDev commented Nov 21, 2024 •

edited

Loading