-
Notifications
You must be signed in to change notification settings - Fork 98
Make tooltips use the browser's Popover API so they don't ever get cut off #9501
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
With a polyfill for anchor positioning in CSS for Firefox et al
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
src/components/Tooltip.module.css
Outdated
| inset-inline-start: anchor(50%); | ||
| inset-block-end: anchor(end); |
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.
Incorrect anchor positioning for top tooltip. anchor(end) refers to the bottom edge of the anchor element, which would position the tooltip's bottom edge at the anchor's bottom (overlapping/below the anchor). For a top tooltip, it should be:
inset-block-end: anchor(start);This positions the tooltip's bottom edge at the anchor's top edge, placing it above the anchor.
| inset-inline-start: anchor(50%); | |
| inset-block-end: anchor(end); | |
| inset-inline-start: anchor(50%); | |
| inset-block-end: anchor(start); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
src/components/Tooltip.module.css
Outdated
|
|
||
| .tooltipWrapper.bottom-left { | ||
| inset-block-start: 100%; | ||
| inset-block-start: anchor(start); |
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.
The bottom-left tooltip positioning is incorrect. It uses anchor(start) which positions the tooltip's top edge at the anchor's top edge, causing it to overlap the parent element instead of appearing below it.
The old code was inset-block-start: 100% (position tooltip below parent), so the new code should use anchor(end) to position at the bottom edge of the anchor:
.tooltipWrapper.bottom-left {
inset-block-start: anchor(end);
}Note that the .bottom class correctly uses anchor(end) on line 133, making this inconsistency a clear bug.
| inset-block-start: anchor(start); | |
| inset-block-start: anchor(end); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
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.
Nah I understand the miscommunication here. These are poorly named but this recreates the existing behavior
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.
Are these snapshot changes expected? It looks like before the sidebar tooltips were visible and now they are not.
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.
No something just changed and broke
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.
Well yes, the snapshot changes are expected I think, because the tooltips go away when you click the pane button now, but there was an issue making nothing show up fixed here
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 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.
No thank is not expected, thank you! I must have messed up one of the definitions. When you say you prefer the old behavior do you mean the feature tree tooltip position only?
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.
Yes, I'm only referring to the Feature Tree. Everything else I tested in the Vercel preview looked the same or better.
2c3ffcf to
523df3a
Compare


Following the MDN docs on Using the Popover API. Positioning non-modal popovers basically requires CSS Anchor Positioning, which isn't supported in Firefox yet, so I've also included this polyfill provided by Oddbird.
In future, we should learn to wield
position-try-fallbacksto get the exact "never overflow" behavior we want from our Tooltips and popover menus. I figured this was a good first step without messing with too much. It's comical how long it took me to figure out these APIs, there is a crazy dearth of useful guidance on what's needed and what's not.TODOs
focusandblurwell when the Tooltip is applied as a sibling to the focusable element, not a child of it. I believe some of our E2E tests rely on the "accessible name" of several buttons that would be changed by putting the Tooltip within them, but I'm not positive. Adding mouse events to any DOM element is fine, but only some elements are focusable. As such, we should get the closest focusable element and if that exists attach the focus and blur event listeners. Lord I can't wait for the interestinvoker API.Before
After
Also I think this will unblock me making the modeling toolbar scrollable, which was stuck because I needed
overflow-visiblefor the tooltips to render there, so that's neat.