Skip to content

TooltipV2: Hide Tooltip when menu is active #7142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TylerJDev merged 11 commits into from
Nov 10, 2025
Merged

TooltipV2: Hide Tooltip when menu is active #7142

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7bb11f9
Hide menus or "popups" when active, based on ARIA attributes provided
TylerJDev Nov 5, 2025
968fcd2
Add `aria-haspopup="menu"
TylerJDev Nov 6, 2025
b3f7042
Add changeset
TylerJDev Nov 6, 2025
9e892b4
Add back `aria-haspopup="true"`
TylerJDev Nov 7, 2025
6da129f
update changeset
TylerJDev Nov 7, 2025
89fdb06
Only apply if `aria-expanded="true"
TylerJDev Nov 7, 2025
42f261d
Check for both boolean and string `true`
TylerJDev Nov 7, 2025
097f616
Maybe fix react 19 test?
TylerJDev Nov 7, 2025
1ec498d
Modify solution to use prop
TylerJDev Nov 7, 2025
417bea6
Update changeset
TylerJDev Nov 7, 2025
eb209fe
Add prop as private
TylerJDev Nov 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wild-aliens-itch.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

IconButton: Hide tooltip when popup is open via new `closeTooltip` prop in `Tooltip`.
8 changes: 8 additions & 0 deletions packages/react/src/Button/IconButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,14 @@ const IconButton = forwardRef(
const {tooltipId} = React.useContext(TooltipContext) // Tooltip v2
const {tooltipId: tooltipIdV1} = React.useContext(TooltipContextV1) // Tooltip v1

const {'aria-expanded': isExpanded, 'aria-haspopup': hasPopup} = props

const hasExternalTooltip = tooltipId || tooltipIdV1

// If the button has an active "popup" (like a menu), we don't want to show the tooltip.
// This is mostly for `ActionMenu`, but could be applicable elsewhere.
const hasActivePopup = (isExpanded === true || isExpanded === 'true') && hasPopup === 'true'

const withoutTooltip =
unsafeDisableTooltip || disabled || ariaLabel === undefined || ariaLabel === '' || hasExternalTooltip

Expand All @@ -55,6 +62,7 @@ const IconButton = forwardRef(
type={description ? undefined : 'label'}
direction={tooltipDirection}
keybindingHint={keybindingHint ?? keyshortcuts}
closeTooltip={hasActivePopup}
>
<ButtonBase
icon={Icon}
Expand Down
14 changes: 13 additions & 1 deletion packages/react/src/TooltipV2/Tooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@ export type TooltipProps = React.PropsWithChildren<{
* long (1200ms)
*/
delay?: 'short' | 'medium' | 'long'
/**
* Prevents the tooltip from opening if `true`.
*
* Accessibility note: This prop should be used with caution. Only use when needing to
* programmatically close the tooltip in response to a specific user action, such as
* opening a menu, or content where the tooltip could overlap with interactive content.
*
* @default false
*/
closeTooltip?: boolean
Copy link
Member

@siddharthkp siddharthkp Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this an internal mechanism instead of part of the API? Don't want to accidentally introduce/promote a way of disabling tooltips unless that is the explicit intention.

If it exists, it will get used; we still have 57 instances of unsafeDisableTooltip that have not been cleaned up 😅

Possible solutions:

  1. Using TooltipContext that IconButton is already aware of
  2. Using css to hide the tooltip, would something like :has(button[aria-expanded=true]) work?
  3. _privateDisableTooltip prop that looks private and we don't document (we have a similar prop in ActionList)

Copy link
Member Author

@TylerJDev TylerJDev Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely can! I'm thinking option 3 might be the best, but let me know what you think.

Using TooltipContext that IconButton is already aware of

This could work, we are exposed to TooltipContext, which currently provides the ID. We're mostly relying on what ActionMenu (through AnchoredOverlay) provides to IconButton to determine if it should be hidden or not (if aria-expanded="true" and aria-haspopup="true" exist on the IconButton)

I'm not too sure what we could pass to context for Tooltip here 🤔 We could check within Tooltip for if it's a menu, but that might add more logic to handle. Let me know what you think though 👀

Using css to hide the tooltip, would something like :has(button[aria-expanded=true]) work?

This would definitely work! The only concern is how we'd scope it to only apply if it's both an IconButton and that button triggers a menu.

e.g. :has(button[aria-expanded=true][aria-haspopup=true])

<!-- We'd only want the CSS to apply to the second button (IconButton) -->
<button aria-haspopup="true" aria-expanded="true">Not an IconButton</button>
<button className="IconButton" aria-haspopup="true" aria-expanded="true">Is an IconButton</button>

There doesn't' seem to be a good identifier between the buttons 🤔

_privateDisableTooltip prop that we don't document (we have a similar prop in ActionList)

This works! Ideally we wouldn't have to use a prop to begin with, but I figured that this approach is less likely to interfere with any existing Tooltip or IconButton instances.

Copy link
Member

@siddharthkp siddharthkp Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't' seem to be a good identifier between the buttons 🤔

data-component="IconButton"?

I'm not too sure what we could pass to context for Tooltip here 🤔 We could check within Tooltip for if it's a menu, but that might add more logic to handle. Let me know what you think though 👀

The provider is in the tooltip which wraps the button, so would have to pass a function down in context for button to call. Not my favorite.

Definitely can! I'm thinking option 3 might be the best

Good backup if the css other does not work reliably

Copy link
Member Author

@TylerJDev TylerJDev Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried the CSS way, but since the Tooltip is a sibling of the IconButton it's not as feasible of a solution 😭

Example 1: &[popover]:not(:has(> [data-component="IconButton"][aria-haspopup='true'][aria-expanded='true'])):popover-open - this would apply if the tooltip was wrapped around the IconButton, but it's a sibling.

Example 2: &[popover]:not(:has(~ [data-component="IconButton"][aria-haspopup='true'][aria-expanded='true'])):popover-open - this only targets siblings after the Tooltip in the DOM, components using Tooltip will always come before, not after.

I might be missing another way, but I think the prop might be the option to run with.

Copy link
Member

@siddharthkp siddharthkp Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, fair enough!

}> &
React.HTMLAttributes<HTMLElement>

Expand Down Expand Up @@ -107,6 +117,7 @@ export const Tooltip: ForwardRefExoticComponent<
className,
keybindingHint,
delay = 'short',
closeTooltip: hideTooltip = false,
...rest
}: TooltipProps,
forwardedRef,
Expand All @@ -130,7 +141,8 @@ export const Tooltip: ForwardRefExoticComponent<
tooltipElRef.current &&
triggerRef.current &&
tooltipElRef.current.hasAttribute('popover') &&
!tooltipElRef.current.matches(':popover-open')
!tooltipElRef.current.matches(':popover-open') &&
!hideTooltip
) {
const tooltip = tooltipElRef.current
const trigger = triggerRef.current
Expand Down
Loading