Add Tooltip support to IconButton

[hectahertz]

What are you trying to accomplish?

The API of IconButton should be updated so Tooltip can be rendered via the component arguments.

• Closes https://github.com/github/primer/issues/780
• See the Accessibility story issue https://github.com/github/accessibility/issues/802

What approach did you choose and why?

We decided to use a slot to render the Tooltip as it allows for explicit control of the defaults, which we want to ensure proper integration and an accessible outcome. We will follow the same pattern for the rest of the components that integrate Tooltip

For example, we set type: :label as the default, as for IconButton there's usually no other label.

After some discussion in the PR comments, 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.

Docs

With aria-label

With aria-label and aria-description

[khiga8]

This may be a design-level decision but one thing that came up in a convo with @inkblotty was whether tooltips (whether it be type: :label or type: :description) should always be set on icon-only buttons so that sighted users have more context. Here's a related audit issue.

What do you think? (I don't want to block this PR so I'd be ok following up on this in a separate PR too) cc: @alliethu

[alliethu]

This may be a design-level decision but one thing that came up in a convo with @inkblotty was whether tooltips (whether it be type: :label or type: :description) should always be set on icon-only buttons so that sighted users have more context. Here's a related audit issue.

What do you think? (I don't want to block this PR so I'd be ok following up on this in a separate PR too) cc: @alliethu

@khiga8 I think this is a great idea that can scale pretty seamlessly. Also, based on the fact that we now have tooltip types (label/description), it seems like a good opportunity to establish guidance for how to structure the tooltip language across the system.

[hectahertz]

What do you think? (I don't want to block this PR so I'd be ok following up on this in a separate PR too) cc: @alliethu

Oh, I 100% agree.

I can see two approaches here;

• Making it required now and fixing those instances as we port them to this implementation.
• Keeping it optional for now, porting all instances to use it, and then introducing the restriction and fixing the missing labels.

I'd like to go for the second approach, as it will allow us to reduce heterogeneity in implementations sooner, but both can work fine.

[inkblotty]

Re: @hectahertz in comment:

• Making it required now and fixing those instances as we port them to this implementation.
• Keeping it optional for now, porting all instances to use it, and then introducing the restriction and fixing the missing labels.

Is the aria-label or similar prop already required? If so, I don't think we would need to add a new prop for it. We could reuse the existing accessible name. If it isn't required, that's a larger discussion.

[hectahertz]

Re: @inkblotty in comment:

Is the aria-label or similar prop already required? If so, I don't think we would need to add a new prop for it. We could reuse the existing accessible name. If it isn't required, that's a larger discussion.

You are 100% right, it is! This definitely makes things easier; we'll still be missing the label on the instances where IconButton is not being used yet, but it should not be such a big issue.

The proposed API here might not be the best then; maybe a better solution would be to ask for an accessible label only once and control at the framework level how we want to use it (showing it on a tooltip or not, etc.). cc: @alliethu

Allowing users granular control of showing a tooltip or not, with custom content different to the aria-label might lead us to unpredictable outcomes. We might not need that level of customization.

[github-actions]

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

[inkblotty]

@hectahertz @khiga8 -- what do we want to do with this?

[khiga8]

@hectahertz is there anything blocking this?

[jonrohan]

👋🏻 I can take over here, I'd like to update this with a lookbook preview

[khiga8]

@jonrohan

Sorry I just caught that this tooltip element is nested inside the Button. This should be adjacent to the Button. Similar problem with Button component and Link component.

This current nesting of the tooltip will result in the button (or link) being triggered when one tries to copy the tooltip text and potentially other unexpected behavior.

I think that in the case of tooltip, there will need to be a wrapper div or span element over the component and tooltip.