fix(ui): keep badge tooltips visible inside scrollable containers - #9157
fix(ui): keep badge tooltips visible inside scrollable containers#9157btea wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
Moderate accessibility and viewport-positioning issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates Badge tooltips with CSS anchor positioning to avoid clipping in scrollable MetaBar containers and adds Storybook coverage.
Changes:
- Adds anchored tooltip rendering and fallback styling.
- Adds a long MetaBar reproduction story.
- Updates tooltip positioning and theme styles.
File summaries
| File | Changes and review findings |
|---|---|
packages/ui-components/src/Containers/MetaBar/index.stories.tsx |
Adds the scrollable MetaBar story. Moderate: Add an aria-label exposing the full stability value. |
packages/ui-components/src/Common/Badge/index.tsx |
Renders anchored tooltip elements. Moderate: Associate the tooltip with the badge using a stable id and aria-describedby. |
packages/ui-components/src/Common/Badge/index.module.css |
Styles and positions anchored tooltips. Moderate: Keep inactive tooltips hidden from the accessibility tree and add viewport edge fallback or clamping. |
Review details
Suppressed comments (3)
packages/ui-components/src/Common/Badge/index.module.css:124
- This makes the inactive tooltip
display: blockand hides it only withopacity: 0. Opacity does not remove text from the accessibility tree; becauseBadgeis rendered inside the MetaBar link, screen readers can includeLegacy/Deprecatedin every link's accessible name even when the tooltip is not shown. Keep the tooltipvisibility: hiddenuntil the hover/focus selectors make it visible, and associate it with the trigger if it is intended to be announced.
.tooltip {
@apply block;
packages/ui-components/src/Common/Badge/index.module.css:130
- These explicit anchor coordinates always center the tooltip below the badge. A trailing badge near the right edge, or a badge near the bottom of the viewport, can still place part of this fixed box outside the viewport, so escaping the scrollable
dldoes not prevent clipping in the edge-aligned cases from #938. Add an anchor-position fallback/flip or clamp the tooltip to the viewport edges.
margin-block-start: 0.5rem;
position: fixed;
position-anchor: var(--tooltip-anchor);
top: anchor(bottom);
translate: -50% 0;
packages/ui-components/src/Containers/MetaBar/index.stories.tsx:193
- The new keyboard target exposes only
entry.stability[0](D,E, orL) as its accessible content;data-tooltipis not an accessible name. Add anaria-labelwith the full stability value so this story does not present an ambiguous one-letter control to screen-reader users, as the existing DataTag component does.
<Badge
className={entry.badgePosition === 'leading' ? 'mr-1' : 'ml-1'}
data-tooltip={entry.stability}
tabIndex={0}
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9157 +/- ##
==========================================
- Coverage 86.12% 85.52% -0.60%
==========================================
Files 86 86
Lines 6046 6087 +41
Branches 357 357
==========================================
- Hits 5207 5206 -1
- Misses 835 877 +42
Partials 4 4 ☔ View full report in Codecov by Harness. |
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
I'm unsure if the best approach here is a dedicated HTML element, I worry this will add many many many more HTML elements to this already very inflatted DOM (thinking of doc-kit and API docs)
I I'm -1 for this, I do believe the solution can be CSS-only; Also the usage of "@supports (anchor-name: --doc-kit-badge-tooltip) {" with a .tooltip using raw CSS instead of Tailwind also is against our styling guidelines 👀
|
Thanks for the review! As far as I can tell, this is the only solution that meets the requirements while staying at the HTML/CSS level. On my testing, pseudo-elements alone can't satisfy them in a pure-CSS approach: a pseudo-element can't resolve That said, I might be missing a more reasonable approach — if you have one in mind, or know of a tracked Chromium fix for the pseudo-element cases, I'm happy to rework the PR around it. Also noted on the styling guidelines — I'll move the raw declarations to |
Description
close nodejs/doc-kit#938
Validation
Related Issues
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.