You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
KeybindingHintdoes 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:
<Tooltiptext="This change cannot be undone."keybindingHint="Delete"><Button>Delete</Button></Tooltip><IconButtonaria-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:
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.
The text was updated successfully, but these errors were encountered:
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
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.
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 🙌
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?
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 🤗
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:The notable exception here is
IconButton
. There's no room on the button itself, but we do now render a tooltip onIconButton
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 ofIconButton
) 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 insideTooltip
. 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 astring
that we can pass through toKeybindingHint
as thekeys
prop.Proposal
Let's add a new prop,
keybindingHint: string
, toTooltip v2
andIconButton
. 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: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 inActionMenu
) 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 renderaria-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:
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 anaria-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.The text was updated successfully, but these errors were encountered: