-
Notifications
You must be signed in to change notification settings - Fork 645
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.
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: eb209fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
Tooltip when menu is activeTooltip when menu is active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where TooltipV2 was displayed even when an ActionMenu was open. The solution involves detecting when an IconButton has an active popup (specifically a menu) and suppressing the tooltip in that state.
Key changes:
- IconButton now checks for active popup state (expanded + has menu popup) to conditionally hide tooltips
- AnchoredOverlay accepts an
aria-haspopupprop to allow customization of the ARIA attribute - ActionMenu explicitly sets
aria-haspopup="menu"on its overlay
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/Button/IconButton.tsx | Adds logic to hide tooltip when button has an active menu popup by checking aria-expanded and aria-haspopup props |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx | Adds aria-haspopup prop to allow consumers to specify the type of popup, defaulting to 'true' |
| packages/react/src/ActionMenu/ActionMenu.tsx | Passes aria-haspopup="menu" to AnchoredOverlay to indicate menu popup type |
| .changeset/wild-aliens-itch.md | Documents the change as a minor version update |
357076e to
9e892b4
Compare
|
😢 Hi from github/github-ui. The integration workflow has failed: https://github.com/github/github-ui/actions/runs/19170653419 |
|
🟢 ci completed with status |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
joshblack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random, since we already are attaching event listeners to the child could we attach something like onClick that will trigger hiding the tooltip? Or is that too broad of an event?
It seems like we want cases where button:active is true to dismiss the tooltip if I'm understanding right, let me know if not 👀
| * | ||
| * @default false | ||
| */ | ||
| closeTooltip?: boolean |
There was a problem hiding this comment.
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:
- Using TooltipContext that IconButton is already aware of
- Using css to hide the tooltip, would something like
:has(button[aria-expanded=true])work? _privateDisableTooltipprop that looks private and we don't document (we have a similar prop in ActionList)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Just to confirm, is the child in this case the one within |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
Closes https://github.com/github/primer/issues/6003.
Video example
Screen.Recording.2025-11-06.at.8.32.08.AM.mov
Hides tooltip based on ARIA props passed to
IconButton. This solution mainly targetsActionMenuusage withIconButton, but may be applicable elsewhere.Changelog
New
Rollout strategy
Testing & Reviewing
Merge checklist