IconButton should include Tooltip by default

[iansan5653]

Summary

There is no documented example of how to render an icon-only button with an accessible tooltip in a way that would meet all of the following standard requirements:

• Render as an icon-only button with a single focus target
• Read the label one time in a manner that ties it to the button, for screen reader users
• Show the label on hover, for mouse users
• Show the label on focus, for keyboard users

If this is a supported behavior, then some documentation is needed to show how to do it. If this is not supported, it would be very useful functionality to add to Primer.

The IconButton documentation provides an example of how to label the component in an accessible manner:

<IconButton aria-label="Search" icon={SearchIcon} />

And the Tooltip documentation has the same:

<Tooltip aria-label="Hello, Tooltip!">Text with a tooltip</Tooltip>

Both of these examples use aria-label however, which provides no obvious way to create an icon button that shows its accessible label on hover as a tooltip.

Use Cases

This is a very common requirement. See for example these instances on github.com:

Notifications bell:

Markdown editor toolbar:

Code view:

This is recommended as a best practice by Primer's own tooltip guide:

Attempts

I've tried all of the following examples but I haven't been able to find a satisfying way to do this:

Label on tooltip: Tooltip does not appear on button focus and the button appears as unlabelled in the accessibility tree.

<Tooltip aria-label="Bold">
  <IconButton icon={BoldIcon} />
</Tooltip>

Label on button only: Tooltip does not work at all.

<Tooltip>
  <IconButton icon={BoldIcon} aria-label="Bold" />
</Tooltip>

Label on both: The label will be read twice to screen readers (something like "Tooltip: Bold, Button: Bold") which is unnecessarily redundant. Also the tooltip still doesn't appear on focus.

<Tooltip aria-label="Bold">
  <IconButton icon={BoldIcon} aria-label="Bold" />
</Tooltip>

Tooltip as button: Seems like a promising idea but Tooltip does not support the as prop.

<Tooltip aria-label="Bold" as={IconButton} icon={BoldIcon} />

Button as tooltip: Renders an unfocuseable span instead of a button.

<IconButton icon={BoldIcon} as={Tooltip} aria-label="Bold" />

Tooltip in button children: Does not work because the IconButton ignores children.

<IconButton icon={BoldIcon} aria-label="Bold">
  <Tooltip aria-label="Bold" role="presentation">Bold</Tooltip>
</IconButton>

Tooltip as button icon: This is horribly inelegant, but it does generate a good accessibility tree and still shows the tooltip on hover. It unfortunately still doesn't show the tooltip on focus, but it's the best I have for now.

<IconButton
  icon={() => <Tooltip aria-label="Bold" role="presentation"><BoldIcon /></Tooltip>}
  aria-label="Bold"
/>

Proposed Solution

My ideal solution would be to just show the IconButton's aria-label as a tooltip by default.

To configure this, I propose adding two new props to the IconButton component:

• disableTooltip: Turn off the tooltip.
• tooltipProps: Configure the tooltip by passing props to the underlying Tooltip component.

Then I could achieve my goal simply with:

<IconButton icon={BoldIcon} aria-label="Bold" />

[siddharthkp]

My ideal solution would be to just show the IconButton's aria-label as a tooltip by default

+1 to that. I like the idea of having a tooltip by default.

disableTooltip: What are the instances where you would want to disable the tooltip?

We'd need to workshop tooltipProps a bit before committing to an API 🤔

[iansan5653]

What are the instances where you would want to disable the tooltip?

I figured there might be a case where the built-in tooltip conflicts with another tooltip or hover effect, but I can't think of a concrete example. Maybe it's actually not necessary to support disabling it.

[lesliecdubs]

I'm going to run this idea of having a tooltip by default at tomorrow's Primer Patterns working group session to get any feedback or concerns before we run with that idea.

In the meantime - @khiga8 could you please take a quick look at this issue and the proposal to adding a tooltip-by-default on IconButton to ensure this approach is in line with our accessibility needs?

[khiga8]

Hi @lesliecdubs!

@hectahertz can speak more to the work on IconButton in PVC! I believe we were leaning towards having tooltip by default for all IconButton for accessibility purposes. Discussion can be found in: primer/view_components#1062.

As a side note, I notice in all the examples listed in this issue, the tooltip is always semantically associated as a label. I'm not sure if this is out of scope, but in the long run, the tooltip should also be allowed to be associated to the icon button as a supplementary description.

You can check out the markup of the markdown toolbar in dotcom as an example. These were recently updated to use tooltips that are associated semantically using aria-describedby with aria-label directly set on the button with a brief accessible name. This was done (at least in dotcom) because the tooltip content is kind of long and contains keyboard shortcuts which makes the tooltip inappropriate as an accessible name. Accessible names should be brief. Therefore you'll notice we have a label like Bold directly on the button and a tooltip associated as description that contains, Add bold text, <Cmd+b>.

We don't have this in dotcom, but similarly in the Notifications button example, it may be appropriate to set aria-label as Notifications, and set a tooltip as supplementary description with You have unread notifications.

We've recently worked on a bunch of accessibility doc improvements on the PVC and Primer Design side to encourage proper tooltip usage with valid tooltip use examples. Here are some references that may be helpful as Primer React team works on tooltip:

• PVC: Tooltip
• Tooltip alternatives

[lesliecdubs]

Thank you for weighing in @khiga8, this is very helpful context to inform our next steps!

It does indeed look like PVC came to the same decision about always showing a Tooltip on Icon Button in primer/view_components#1062:

After primer/view_components#1062 (comment), we decided to always include a Tooltip on IconButton. The aria-label` property is always required, and this will improve the experience for sighted users. Hopefully, this approach will simplify the API and will keep the instances of IconButton consistent and accessible.

@hectahertz can you weigh in on any other API decisions that have been made in PVC that may be relevant? Perhaps @hectahertz and @siddharthkp can sync in these comments on whether we want to provide support for props disableTooltip or tooltipProps as proposed in this issue, and whether there are other specific learnings from the PVC work that should inform PRC's approach.

[siddharthkp]

From: primer/view_components#1062, IconButton has an argument called tooltip_direction which is the only customisation tooltip allows. (tooltip arguments)

@hectahertz @khiga8 Are there plans on expanding this list of arguments?

As a side note, I notice in all the examples listed in this issue, the tooltip is always semantically associated as a label. I'm not sure if this is out of scope, but in the long run, the tooltip should also be allowed to be associated to the icon button as a supplementary description.

Thanks for the example, this is really helpful! It was one of the questions I had :)

[siddharthkp]

tl;dr: we should copy what @hectahertz did in primer/view_components#1062

 

long version:

The tooltip from view_components only has one relevant attribute for the conversation: tooltip_direction, but the Tooltip in primer/react is more customisable, so the API could look end up looking slightly different:

 

Here are 2 use cases:

1. IconButton with an aria-label:

   

   Without tooltip:

   <IconButton icon={KebabIcon} aria-label="More options" />

    

   Option 1: wrap IconButton in a Tooltip

   <Tooltip text="More options" direction="sw"  align="left">
     <IconButton icon={KebabIcon} /> // need to make aria-label optional
   </Tooltip>

   👍 If you've used the Tooltip component before, composing it this way is intuitive
   👍 Tooltip attributes go on the tooltip as you would expected

   👎 Need to make aria-label on IconButton optional so that it is not repeated twice. Can result in an IconButton without a Tooltip

    

   Option 2: bake in tooltip with IconButton (use aria-label used for tooltip text)

   <IconButton icon={KebabIcon} aria-label="More options" tooltipDirection="sw" tooltipAlign="left" />
   // or
   <IconButton icon={KebabIcon} aria-label="More options" tooltipProps={{direction: 'sw', align: 'left'}} />

   👍 Can ensure there will always be a tooltip with IconButton

   👎 tooltipProps on the IconButton aren't intuitive
   👎 while unintuitive, it's hopefully rare to want to customize the tooltip's position. we can attempt to restrict the options to just tooltipDirection to avoid the more awkward tooltipProps API

   While option 1 is more composable, we should bake in the tooltip in Option 2 to make the recommended path the default one.

    

2. For the markdown editor with aria-label and aria-description

   • aria-label is "Bold"
   • aria-description = tooltip text is "Add bold text, <Cmd+b>"
   

   Option 1:

   <Tooltip text="Add bold text, <Cmd+b>" direction="s">
     <IconButton icon={BoldIcon} aria-label="Bold" />
   </Tooltip>

   👍 If you've used the Tooltip component before, composing it this way is intuitive
   👍 Tooltip attributes go on the tooltip as you would expected
   👍 (compared to previous example) Don't need to make aria-label optional, this is perfect.

   Option 2:

   <IconButton 
     icon={BoldIcon}
     aria-label="Bold"
     aria-description="Add bold text, <Cmd+b>"  // aria-description used for tooltip
     tooltipDirection="s"
   />

   👍 collocating aria-description right next to aria-label on the IconButton itself makes a lot of sense while reading the code (for someone who works with accessible markup in mind, this is very intuitive. I have secondary information about this button, so that goes on aria-description)
   👎 It's not immediately obvious that the tooltip will pick up aria-description instead of aria-label (for someone who is approaching this visually, it isn't intuitive. I want to add a tooltip, but instead of using a Tooltip component, i should add aria-description because this is an IconButton)
   👎 2 ways of doing the same thing - if someone writes the code from Option 1, that would also work and give the same exact output 😅

 

Personal opinion

If we look at the library as a whole and not just IconButton, it makes more sense to go with Option 1, re-use Tooltip and ask folks to wrap IconButton with a Tooltip. It's easy to miss, but we can enforce this with help of our linter or/and by adding warnings in dev mode. example of warning, sandbox

However, I think being accessible out of the box makes Option 2 very attractive despite the awkwardness around customising tooltip position. If we know developers would always have to wrap an IconButton with a tooltip, we should do it for them.

Having 2 ways of doing one thing from the markdown example feels wrong, but is an okay tradeoff to make as long as the output for both approaches is accessible.

[vdepizzol]

I'm going to run this idea of having a tooltip by default at tomorrow's Primer Patterns working group session to get any feedback or concerns before we run with that idea.

@lesliecdubs thank you for bringing this issue to the Primer Patterns working group attention 👋

@siddharthkp said:

(...) I think being accessible out of the box makes Option 2 very attractive despite the awkwardness around customising tooltip position. If we know developers would always have to wrap an IconButton with a tooltip, we should do it for them.

I agree we should provide components that are accessible by default.

A few considerations:

aria-label + aria-describedby

I wonder if we could abstract the aria-label into a more generic label prop for IconButton, while still generating aria-label + aria-describedby at the end. That way, it's not that aria-label also gets rendered as a tooltip, but that the IconButton label is initially used both as an aria-label, and as a tooltip:

<IconButton
  icon={HeartIcon}
  label="Sponsor"
/>

aria-describedby, as the HTML attribute specifies, asks for an element ID. In this case, we're passing a string directly. That could cause some confusion. Maybe we could call it tooltipText or tooltipDescription, to make it obvious it's a variation of the label specifically for the tooltip text.

<IconButton
  icon={BoldIcon}
  label="Bold"
  tooltipText="Add bold text"
/>

Optional tooltips

Although tooltips should come by default and be present in most IconButton components, it might make sense to decouple the tooltip behavior if necessary.

I can imagine larger components using IconButton for convenience (such as expanders, split buttons, etc) which wouldn't need a tooltip based on their context (but would need an aria-label, of course!).

An example I can think about is the TreeGrid I'm currently working on. The placement and context of the icon are enough for describing behavior. In that case we could have something like disableTooltip:

<IconButton
  icon={ChevronRightIcon}
  label="Expand folder"
  disableTooltip
/>

That could also be useful when a custom rendering is used, or when there's a need for a custom tooltip with extra parameters:

<Tooltip text="Add bold text" direction="s">
  <IconButton
    icon={HeartIcon}
    label="Sponsor"
    disableTooltip
  />
</Tooltip>

Or, if an IconButton is a child of Tooltip, could we enforce a disableTooltip directly?

<Tooltip text="Add bold text" direction="s">
  <IconButton
    icon={HeartIcon}
    label="Sponsor"
  />
</Tooltip>

Note that the label prop remains required.

Shortcuts

I think we should have a better way to reference shortcuts in the tooltips, instead of bundling them together with the description.

@maximedegreve has done some explorations for improved <kbd>, which I think could be brought into tooltips as well:

Here's a quick test:

An ideal scenario could have a Tooltip prop with common keys (Command, Shift, etc) automatically switching to their correct glyphs, and supporting multiple OSs:

<IconButton
  icon={ChevronRightIcon}
  label="More actions"
  shortcutPreview={{
    mac: ['Command', 'Shift', 'M'],
    win: ['Ctrl', 'Shift', 'M']
  }}
/>

In the example above I'm using shortcutPreview to make sure the prop doesn't make it sound like it's actually adding a shortcut to the action. 😁

@siddharthkp let me know your thoughts! 🙌

[siddharthkp]

aria-describedby, as the HTML attribute specifies asks for an element ID.

Oops, I should have used aria-description, not aria-describedby. My bad, fixed in the code now.

I wonder if we could abstract the aria-label into a more generic label prop for IconButton,

I mostly agree with using descriptive names over html attributes here.

<IconButton
  icon={BoldIcon}
- aria-label="Bold"
+ label="Bold"
- aria-description="Add bold text"
+ tooltip="Add bold text"
/>

I think we should have a better way to reference shortcuts in the tooltips, instead of bundling them together with the description.
An ideal scenario could have a Tooltip prop with common keys (Command, Shift, etc) automatically switching to their correct glyphs, and supporting multiple OSs

I agree with the latter but not with the first part. API based on an config object work well when we have tight control over the contents in the library, but not when the text is passed by the application layer.

I'd prefer to build a tiny reusable Key component that can be used with Tooltip:

import {Key} from '@primer/react'

<Tooltip 
  text={<>Add bold text, <Key meta>B</Key></>}
  direction="sw"
>
  <IconButton icon={BoldIcon} label="Bold" />
</Tooltip>

---

[siddharthkp]

I've slept over this and have a slightly different approach: opt-in composition

But first, Revisiting option 1 and option 2 again after @vdepizzol's comments:

Option 1 - Composition
Option 2 - Collocation / config-esque

1. IconButton with tooltip

// option 1
// note: as label and tooltip text are same, we would add only one of them.
<Tooltip text="More options" direction="sw">
  <IconButton icon={KebabIcon} label="More options" />
</Tooltip>


// option 2 
<IconButton icon={KebabIcon} label="More options" tooltipDirection="sw" />
// note: tooltipDirection is an optional customisation option

// ⭐️ we like option 2 more because it has the tooltip baked in.

2. IconButton with tooltip text that's different than label

// option 1: to add a different tooltip text than label, 
// wrap the iconbutton in a tooltip.
// note: label is still required
<Tooltip text="You have no unread notifications" direction="sw" >
  <IconButton icon={BellIcon} label="Notifications" />
</Tooltip>


// option 2: to add a different tooltip text than label, 
// pass a tooltip prop. predictable API smell of having tooltip text + tooltip prop on anchor component.
<IconButton
  icon={BellIcon}
  label="Notifications"
  tooltip="You have no unread notifications"
  tooltipDirection="sw"
/>

// option 2 isn't ideal for this use case, but the tradeoff isn't too bad to optimise for use case 1 above.

3. IconButton with no tooltip

// option 1: nothing to do here, because there was no tooltip
<IconButton icon={ChevronRightIcon} label="Expand folder" />

// option 2: disable tooltip by passing false instead of text
// note: label is still required 
<IconButton icon={ChevronRightIcon} label="Expand folder" tooltip={false} />

// both options are pretty good.

4. IconButton with rich content tooltip

import {Key} from '@primer/react}

// option 1:
<Tooltip 
  text={<>Add bold text, <Key meta>B</Key></>}
  direction="sw"
>
  <IconButton icon={BoldIcon} label="Bold" />
</Tooltip>

// option 2:
<IconButton
  icon={BoldIcon}
  label="Bold"
  tooltip={<>Add bold text, <Key meta>B</Key></>} 
  tooltipDirection="sw"
/>

// both options are pretty similar. option 1 feels a tiny bit better because the long form content is separated out.

 

Option 3: Opt-in composition

Best of both worlds

1. IconButton with tooltip

// by default, we use label to add tooltip. 
// ⭐️ accessible by default
<IconButton icon={KebabIcon} label="More options" />

2. IconButton with tooltip text that's different than label

// if you want to change the tooltip text or direction,
// opt-in into composition and wrap the IconButton in a tooltip component
<Tooltip text="You have no unread notifications" direction="sw" >
  <IconButton icon={BellIcon} label="Notifications" />
</Tooltip>

// good things: Tooltip props are on the Tooltip. If you've used the Tooltip before, this is very intuitive.

3. IconButton with no tooltip

// silly little prop to disable default behavior 😅
<IconButton icon={KebabIcon} label="More options" tooltip={false} />

4. IconButton with rich content tooltip

import {Key} from '@primer/react'

// opt-into composition
<Tooltip 
  text={<>Add bold text, <Key meta>B</Key></>}
  direction="sw"
>
  <IconButton icon={BoldIcon} label="Bold" />
</Tooltip>

// note: only Tooltip supports rich text, IconButton label is still boring old string.

[iansan5653]

I like 'Option 3: Opt-in composition' - it provides significant power for complex scenarios. Having label be always required and of type string ensures that the button's aria-label is always valid.

I can see the last example generating a structure akin to:

<div>
  <button aria-label="Bold" aria-describedby="tooltipId"><!--bold icon--></button>
  <span id="tooltipId">Add bold text, <kbd>⌘B</kbd></span>
</div>

From a CSS perspective, it wouldn't be too hard to have a containing Tooltip prevent the aria-label tooltip from appearing and to have the container show its tooltip on focus-within.

[lesliecdubs]

Next step on Primer side: let's have a working session to align on the component API. @siddharthkp can you organize?

[vdepizzol]

@siddharthkp love where this is going! Option 3 makes a lot of sense.

Just a comment on shortcut placement. I believe we'd still benefit from a specific slot for shortcuts instead of placing them inline with the tooltip text. I think it'd be a more future-proof solution, and it'd give us more flexibility on how to handle their rendering... Example:

<Tooltip 
  text="Add bold text"
  shortcutPreview={<Key meta>B</Key>}
  direction="sw"
>
  <IconButton icon={BoldIcon} label="Bold" />
</Tooltip>

[mperrotti]

@siddharthkp - I agree that 'Option 3: Opt-in composition' is the right way forward.

Just a few observations:

1. Since IconButton renders a tooltip by default, a Primer customer could feel uncertainty when nesting it inside of a Tooltip

// This will show a tooltip that reads "Notifications"
<IconButton icon={BellIcon} label="Notifications" />

// Am I supposed to pass `tooltip={false}` to prevent two tooltips from being shown?
<Tooltip text="You have no unread notifications" direction="sw">
  <IconButton icon={BellIcon} label="Notifications" tooltip={false} />
</Tooltip>

Did you have an idea for how we could suppress the IconButton tooltip if it's a child of Tooltip? I'm assuming we'd use React Context.

2. Instead of tooltip={false} to override the default behavior, I'd suggest we phrase it like hideTooltip. That way, we can just check if hideTooltip is true instead of checking if it's defined and that it's false. Small bonus: users can just pass hideTooltip instead of tooltip={false}. See the visuallyHidden prop on FormControl.Label for another example.

3. I think @vdepizzol's shortcutPreview idea is helpful for maintaining consistent layouts, but I don't like that it's specific to keyboard shortcuts, and I think it will end up being used to render things other than keyboard shortcuts. Maybe we change it to something more generic like trailingContent or a child component like Tooltip.TrailingContent?

If we want to keep shortcutPreview and only allow keyboard shortcuts, maybe it could accept an array of keyboard keys instead:

<Tooltip 
  text="Add bold text"
  shortcutPreview={['meta', 'B']}
  direction="sw"
>
  <IconButton icon={BoldIcon} label="Bold" />
</Tooltip>

[pksjce]

Love this discussion.

First off, I agree with @khiga8 that the label and the tooltip content must be separate entities. They both have different functions. Label is an immediate quick recognition of the button's functionality while the tooltip is an elaborate context driven description. If the tooltip text is going to be the same as the button label text, that is not an appropriate use of the tooltip.

Additionally, providing a default tooltip with the button is not a great idea. This leads to unnecessary API overload on the Button like tooltipProps and hideTooltip. We want to use something like hideTooltip sparingly but with this pattern it might become a norm than an escape hatch.

My vote is for the Tooltip component to intentionally wrap around the IconButton when a tooltip is required. This can then become a consistent pattern for all Tooltip usage. For example, it's not uncommon to have tooltips on normal Button components, Input, Link components etc as shown here.

So in the simple use case we have

<Tooltip 
  text="Add bold text"
  direction="sw"
>
  <IconButton icon={BoldIcon} label="Bold" />
</Tooltip>

Further, we should have additional API for Tooltip to separate the trigger and rendered content to support more complex usecases like components in tooltips and dynamically rendered tooltips.

import {Key} from '@primer/react'
import * from '@primer/react/tooltip'

<Tooltip direction="sw">
  <TooltipTrigger>
    <IconButton icon={BoldIcon} label="Bold" />
  </TooltipTrigger>
  <TooltipContent>
    <>Add bold text, <Key meta>B</Key></>
  </TooltipContent>
</Tooltip>

[siddharthkp]

hideTooltip:

@mperotti: Am I supposed to pass tooltip={false} to prevent two tooltips from being shown? Instead of tooltip={false} to override the default behavior, I'd suggest we phrase it like hideTooltip.

Yep, I agree with you. tooltip=false only made sense while there was a tooltip="some text" prop on the IconButton.

We should disable it automatically. Both context and children magic would work here.

@pksjce: We want to use something like hideTooltip sparingly but with this pattern it might become a norm than an escape hatch.

Yes! 💯

This should be a super rare use case. We could even start by not adding it to the public API at all and only using it internally, for example in TreeGrid.

---

Automatic tooltip:

@pksjce: My vote is for the Tooltip component to intentionally wrap around the IconButton when a tooltip is required. This can then become a consistent pattern for all Tooltip usage.
I agree with @khiga8 that the label and the tooltip content must be separate entities. They both have different functions.

Vote recorded!

You're right, ideally tooltip content should be a description instead of simple label. But, I fear folks would often not use one 😢

Personal opinion: anything we can do by default, without getting in the way of them doing better is a step in the right direction.

longer personal opinion from above:

If we look at the library as a whole and not just IconButton, it makes more sense to go with Option 1, re-use Tooltip and ask folks to wrap IconButton with a Tooltip. It's easy to miss, but we can enforce this with help of our linter or/and by adding warnings in dev mode. example of warning, sandbox

However, I think being accessible out of the box makes Option 2 very attractive despite the awkwardness around customising tooltip position. If we know developers would always have to wrap an IconButton with a tooltip, we should do it for them.

---

Rich text Tooltips:

@mperrotti: Maybe we change it to something more generic like trailingContent or a child component like Tooltip.TrailingContent?

@pksjce: Further, we should have additional API for Tooltip to separate the trigger and rendered content to support more complex usecases like components in tooltips and dynamically rendered tooltips.

I agree with both of you. But, I think we should exclude shortcuts and other rich text for this conversation and come back to it after along with other components that use shortcuts. There is a broader pattern there.

---

Summary

Leaning towards these decisions:

1. No additions to the IconButton API
2. Make aria-label compulsory on IconButton
3. Automatic add tooltip for IconButton if it's not wrapped in a Tooltip. (tooltip text = aria-label)
4. Avoid multiple tooltips automatically, don't add hideTooltip to public API just yet.

[iansan5653]

I really like where this is going. One more small thing I noticed today with respect to keyboard shortcuts - there is an aria-keyshortcuts attribute that should probably be set on the button.

Maybe the default tooltip content should be something like (rough pseudocode):

<>
  {ariaLabel} {ariaKeyshortcuts.split(' ').map(
    shortcut => shortcut.split('+').map(key => <Key>{key}</Key>).join(' + ')
  ).join(' / ')}
</>

In other words, maybe we should render the aria-keyshortcuts into the tooltip by default if we are already rendering the aria-label there.

[siddharthkp]

Turns out our Tooltip component isn't really accessible, so that's a slight increase in scope, updated size to rock 😅

[siddharthkp]

Reopening because we reverted #2006, now tackled in #2094

[siddharthkp]

Hi! I moved this issue to Unprioritized backlog so that we can get a chance to prioritise this and make an attempt once again. tagging @lesliecdubs so that it doesn't just live in my head :D

context:

In #2006, I tried to do 3 things,

1. make Tooltip accessible
2. use useAnchoredPosition to position Tooltip
3. add Tooltip to IconButton by default

There were many breaking changes in memex (some because there's a version of IconButton + Tooltip component in memex, others because useAnchoredPosition didn't always work), so I had to roll this back to attempt again in the future.

[mperrotti]

This also affects the icon-only SegmentedControl buttons because they use a tooltip to show the hidden label.

Instead of creating a new a11y issue for SegmentedControl, I'm including this info here. Let me know if a new issue specific to SegmentedControl would be more helpful, and I'll create it.

[lesliecdubs]

I've moved this from Inbox back to Unprioritized Backlog for now. @siddharthkp I think everyone else is heads down right now, but if you had space to take a look at this again this quarter, we could consider moving it up next. If not, we can reassess before the next planning cycle at the end of September.

[maximedegreve]

Cross posting: https://github.com/github/primer/issues/1244

[broccolinisoup]

Hi everyone 👋🏻 I am working on remediating Tooltip accessibility issues and IconButton is a part of it. Please see the batch issue https://github.com/github/primer/issues/2137 if you are curious about the details of the overall remediations and the release strategy.

I just caught up with all the comments here and as far as I understand, the suggested PRC IconButton API for label and description type tooltip looks like below (copy-paste from earlier)

IconButton with tooltip

// by default, we use label to add tooltip. 
// ⭐️ accessible by default
<IconButton icon={KebabIcon} label="More options" />

IconButton with tooltip text that's different than label (Supplementary text)

// if you want to change the tooltip text or direction,
// opt-in into composition and wrap the IconButton in a tooltip component
<Tooltip text="You have no unread notifications" direction="sw" >
  <IconButton icon={BellIcon} label="Notifications" />
</Tooltip>
// good things: Tooltip props are on the Tooltip. If you've used the Tooltip before, this is very intuitive.

However in PVC, all comes within the box. So the equivalent of PVC API would be

IconButton with tooltip

// Same except the prop name PVC uses `aria-label` versus `label`
// ⭐️ accessible by default
<IconButton icon={KebabIcon} aria-label="More options" />

IconButton with tooltip text that's different than label (Supplementary text)

  <IconButton icon={BellIcon} aria-label="Notifications" aria-description="You have no unread notifications"/>

I am very curious to hear what you think. Feel free to add your questions and concerns and hopefully we will be resolving this issue soon ⭐

[joshblack]

Definitely down to match the existing PVC work if that helps with consistency 👍 (meaning that we match the aria-label and aria-description combination instead of the wrapping Tooltip convention) Personally, it'd be great to just use label and description but, no matter what, having these built-in to IconButton would be a huge win 🥳

[siddharthkp]

Disclaimer: I did some work on Tooltip before and went down this same road, so I have a lot of thoughts. Sorry!

 

IconButton with tooltip

// Same except the prop name PVC uses aria-label versus label
// ⭐️ accessible by default
<IconButton icon={KebabIcon} aria-label="More options" />

perfect default case. love it, ship it

 

IconButton with tooltip text that's different than label (Supplementary text)

<IconButton icon={BellIcon} aria-label="Notifications" aria-description="You have no unread notifications"/>

Obviously biased, so feel free to ignore, I like my suggestion of wrapping Tooltip explicitly when you want any customisations for tooltip.

I think bringing the API from PVC has some issues:

<IconButton
  icon={BellIcon}
  aria-label="Notifications"
  aria-description="You have no unread notifications"
/>

1. API's not very intuitive: It's not obvious looking at the API that if you want to customise tooltip text, you should add aria-description. It's something you have to know or find in the docs, not something you can guess.

   While I like the idea of using aria-props in the API when applicable, I feel like we are misusing the prop here because aria-description doesn't actually go on the button in PVC. Instead, we use it as visible text in the tooltip and add aria-describedby to the button for screen readers.

   // API:
   <IconButton
     icon={BellIcon}
     aria-label="Notifications"
     aria-description={<>Add bold text, <Key meta>B</Key></>}
   />
   
   // vs rendered: 
   <div class="position-relative">
      <button
        aria-label="Bold"
        id="icon-button-hashed"
        aria-describedby="tooltip-hashed"
      >
        <svg aria-hidden="true"></svg>
      </button>
      <tool-tip
        role="tooltip"
        id="tooltip-hashed"
        for="icon-button-hashed"
        data-type="description"
      >
        Add bold text, Cmd+b
      </tool-tip>
   </div>

   Another use case is when the tooltip can have rich text (not interactive) inside it, which would be invalid for aria-description but are valid for tooltip text. Asking to use rich text in aria-description because it's not actually aria-description feels unintuitive. (example from memex, need to validate if this is a use case we should support or is it being used to create inaccessible ui)

   <IconButton
    icon={BellIcon}
    aria-label="Notifications"
    aria-description={<>Add bold text, <Key meta>B</Key></>}
   />

2. Tooltip props need to go on the IconButton: An easy way to solve the above issue would be to hide implementation detail and accept a tooltip specific prop like tooltip or tooltipText instead.

   <IconButton
     icon={BellIcon}
     aria-label="Notifications"
     tooltip="Add bold text, Cmd+b"
   />

   It's not pretty, but it's totally fine if it's just one prop.

   But it's probably not one prop, if you want to customise the direction of the tooltip, you need to accept another prop on IconButton which looks something like tooltip_direction from PVC.

   If we want to allow style customisation, we need to accept props like tooltipSx or tooltipClassName to passthrough as well, which feels like the props are on the wrong place. (note: tooltip is a sibling of button, not child, it's not possible to target the tooltip from button's sx / classname)

   <IconButton
     icon={BellIcon}
     aria-label="Notifications"
     tooltipText="You have no unread notifications"
     tooltipDirection="sw"
     tooltipClassName="icon-button-with-tooltip__tooltip"  // from memex
   />
   
   // or 
   
   ```tsx
   <IconButton
     icon={BellIcon}
     aria-label="Notifications"
     tooltipProps={{
       text="You have no unread notifications"
       direction="sw"
       className="icon-button-with-tooltip__tooltip"  // from memex
     }}
   />

 

Suggestion: Opt-in composition

1. Default happy case: By default, we use label to add tooltip. It's perfect, no notes!

<IconButton icon={KebabIcon} label="More options" />

2. IconButton with tooltip text that's different than label (Supplementary text): If you want to customise the tooltip text, direction or styles, you can opt-in to composition and wrap the IconButton in a tooltip component

<Tooltip text="You have no unread notifications" direction="sw"  className="icon-button-tooltip">
  <IconButton icon={BellIcon} label="Notifications" />
</Tooltip>

👍 Tooltip props are on the Tooltip.
👍 Looks like other usage of tooltip, consistent is intuitive

Also works for rich content (if we want to allow it):

<Tooltip 
  text={<>Add bold text, <Key meta>B</Key></>}
  direction="sw"
>
  <IconButton icon={BoldIcon} label="Bold" />
</Tooltip>

Different API for PVC and PRC?

I think we can bring these suggestions to PVC, if we like this API more as a group.

Personally, I think it's also okay to have some API differences between PVC and PRC based on what feels intuitive in the mental model of Rails and React. For me, It's always great when we can have the same API, but not compulsory when there are tradeoffs.

[iansan5653]

Regarding rich content, I find it hard to come up with another valid example outside of keyboard shortcuts. I do think the keyboard shortcuts example is important though - the comment box solution of putting the shortcut in angle brackets (Add bold text, <Cmd+b>) is not pretty or particularly accessible.

In Memex we use the shared KeyboardKey component for this in most places, but can't use it in tooltips because they only support plain text. This component has been carefully designed to be accessible - ie, it may render ^B visually but the resulting text that would be extracted for a description or label will be Control B so that screen readers don't read "caretB".

With that in mind, perhaps it would be better to add an explicit keyboard shortcut prop that we then render using KeyboardKey and other than that, only allow plain text. This prevents consumers from, for example, putting a link inside the tooltip. It also ensures that shortcuts are rendered consistently and accessibly.

---

Now how can we represent that concept in the API? To render this, we probably need to add a keyboardShortcut prop to the Tooltip component. This could be useful in other cases as well outside of just icon buttons.

However, that doesn't solve the happy path - what if I have a button with a simple label and keyboard shortcut? We could add keyboardShortcut to the IconButton component as well as Tooltip:

<IconButton icon={BoldIcon} label="Bold" keyboardShortcut="CommandOrControl+B" />

Note: One caveat we'd have to make clear in documentation is that keyboardShortcut doesn't actually bind the shortcut. I absolutely think we leave that part up to the consumer. This is not data-hotkey.

But then what happens in the case where we define it in both places?

<Tooltip text="Add bold text" keyboardShortcut="Alt+B">
  <IconButton icon={BoldIcon} label="Bold" keyboardShortcut="CommandOrControl+B" />
</IconButton>

Personally I prefer the approach of forwarding the tooltipProps from IconButton rather than trying to make this API composable. Then we can simply eliminate tooltipProps.keyboardShortcut on a type level and solve that problem.

More importantly, it would then be obvious how to override the tooltip without having to refer to documentation examples to realize that wrapping in Tooltip will have a 'magic' side effect of hiding the default tooltip. I generally don't like the magic side effect hacks we've had to make to get composable-style APIs - they make for nice code but they are pretty hard to understand and debug.

<IconButton
  icon={BoldIcon}
  label="Bold"
  keyboardShortcut="CommandOrControl+B"
  tooltipProps={{text: "Add bold text"}}
/>

[joshblack]

I feel like a great point that you bring up @iansan5653 is that it'd be great if we can make illegal (or non-ideal) states unrepresentable. I think you had a great example with:

<Tooltip text="Add bold text" keyboardShortcut="Alt+B">
  <IconButton icon={BoldIcon} label="Bold" keyboardShortcut="CommandOrControl+B" />
</IconButton>

To me this demonstrates a challenge that can come up where you have ambiguity as to what will happen in the composition case (specifically what prop value "wins" here). I do think that over time you can infer that this is a merge between the two and that the inner one wins out, ultimately.

I think an advantage of IconButton owning the props in this scenario is that it is able to control the set of permissible values provided to Tooltip that make sense when used alongside IconButton.

Either way, curious what you would recommend @broccolinisoup with all the work that you've been doing to see what would help make this the easiest change on your end 😅 lol

[broccolinisoup]

👋🏻 Here is the API proposal for the IconButton 🎉 https://github.com/github/primer/pull/2339

We left the keyboard shortcut stuff to deal with later and tried addressing more prominent issues in the first iteration. Other than that, I think we addressed all issues we talked here. Let me know if I miss anything and I am looking forward to hearing your feedback on the PR 🙌🏻

[github-actions]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.