Proposal: Keybinding hints in tooltips / icon buttons

[iansan5653]

Note

This is closely related to https://github.com/github/primer/issues/1710, but I want to specifically discuss the API / accessibility implications in @primer/react.

Background

Assuming #4750 ships, the new KeybindingHint component will allow for keyboard shortcut discoverability for most common Primer interactive components:

// Buttons
<Button trailingVisual={() => <KeybindingHint keys="Mod+Enter" />}>Submit</Button>

// ActionLists/ActionMenus
<ActionList.Item>
  Paste
  <ActionList.TrailingVisual>
    <KeybindingHint keys="Mod+v" />
  </ActionList.TrailingVisual>
</ActionList.Item>

// TextInputs
<TextInput trailingVisual={() => <KeybindingHint keys="/" />} aria-label="Search" />

The notable exception here is IconButton. There's no room on the button itself, but we do now render a tooltip on IconButton by default. This is almost definitely where we'd want to indicate a keybinding.

However, there's currently no way to render keybinding info in Tooltip v2 since it can only render plain text. This is intentional due to accessibility constraints -- the tooltip becomes the accessible description (label in the case of IconButton) of the targeted element, so it can only contain plain text.

KeybindingHint does have an accessible plain text representation however, so it shouldn't be problematic to allow rendering it inside Tooltip. The API for this can be pretty simple - there's no need to customize the format or variant of the hint, so all we need to do is accept a string that we can pass through to KeybindingHint as the keys prop.

Proposal

Let's add a new prop, keybindingHint: string, to Tooltip v2 and IconButton. The naming here indicates that this is just a hint for discoverability - ie, it's not going to automatically wire up any sort of keyboard shortcut support. Usage would look like this:

<Tooltip text="This change cannot be undone." keybindingHint="Delete">
  <Button>Delete</Button>
</Tooltip>

<IconButton aria-label="Search" keybindingHint="Mod+g" icon={SearchIcon} />

Accessibility

The most obvious solution for accessibility is to use aria-keyshortcuts to indicate the availability of a shortcut. However, this property does not have universal support, and it's more typically used to indicate mnemonics (such as the current usage in ActionMenu) than shortcuts.

Instead, I'd propose we include the shortcut hint as part of the accessible label/description. This provides a consistent UX across non-sighted and sighted users, and it means we don't need to add any special support to Button and other interactive elements which don't currently render aria-keyshortcuts by default.

When rendering, I think we should consider offsetting the hint with visually-hidden punctuation, so that the label/description has a more natural cadence when read by screen readers. "Search (command g), button" is much easier to understand than "Search command g, button". After all, the hint is offset for sighted users by a visual border. In code, this would be pretty straightforward:

<>
  {text}
  <VisuallyHidden>(</VisuallyHidden><KeybindingHint variant="onEmphasis" keys={keybindingHint} /><VisuallyHidden>)</VisuallyHidden>
</>

I don't know what the standard practice is here as far as which punctuation to use - it may be more ergonomic to offset the shortcut with a comma or something else.

It's possible that specifically for IconButton it might be worth considering moving the shortcut hint to an aria-description rather than appending it to the label, so that we can keep button labels succinct. This would require a special case but wouldn't be too difficult if that's the route we'd rather take. However the flip side of that is that a description that is just a keyboard shortcut could be confusing.

[broccolinisoup]

I haven't looked at the proposal deeply yet but wanted to link the recent changes we did in icon buttons around keyboard shortcuts #4707 and see if there is any conflicts

[iansan5653]

That's great! I didn't know that work was in progress. In that case, this proposal would touch more on how we render the shortcuts than on the API as it looks like an API has already been decided on. I would still propose that we use the KeybindingHint component to render those keyboard shortcuts in the tooltip, which should result in a better experience for all users.

[broccolinisoup]

I would still propose that we use the KeybindingHint component to render those keyboard shortcuts in the tooltip, which should result in a better experience for all users.

That sounds good to me! I can certainly explore that 🙌

[lesliecdubs]

Triage notes: it seems like once #4750 merges, we may want to look further into this proposal. @broccolinisoup is this within the realm of the work you are already doing related to IconButton?

[broccolinisoup]

Ah sorry @lesliecdubs I missed the ping!

is this within the realm of the work you are already doing related to IconButton?

I would say so. It is not directly related since we completed the Icon button tooltip work but it is an improvement and I'd be happy to work on it if it is possible 🤗