fix(#487): decide tag tooltips from the rendered tag, not an off-screen measurement - #489
Open
gnbm wants to merge 1 commit into
Open
fix(#487): decide tag tooltips from the rendered tag, not an off-screen measurement#489gnbm wants to merge 1 commit into
gnbm wants to merge 1 commit into
Conversation
…en measurement With showValueAsTags, the overflow check measured the label against .vscomp-toggle-button (~73px wider than the space the tag text gets) at the button's 14px instead of the tag's 12px, so truncated tags could miss their tooltip and fitting tags could carry one. The tooltip attributes now go on the tag's content span with data-tooltip-ellipsis-only, so tooltip-plugin evaluates scrollWidth > offsetWidth on the real box at hover time. No overflow measurement happens at render at all. This also keeps the answer correct after a resize and for a control first rendered inside a hidden container - neither of which a render-time measurement can do. The content span rather than .vscomp-value-tag: the tag is inline-flex and the span carries width: calc(100% - 24px), so the span clips while the tag reports no overflow of its own (measured on a clipped tag: tag 260/262, content 259/225). It matches how the non-tag value text has always worked. Utils.willTextOverflow() and the shared off-screen measurer are removed (supersedes the earlier one-shared-node optimisation); perf-text-measurer.cy.ts, which asserted the measurer's existence, is replaced by tag-tooltip-overflow.cy.ts (6 cases, 4 red against master). Those cases assert the tooltip appearing on hover rather than the presence of an attribute, so they survive a change of mechanism. Two follow-on test changes: security-quote-escaping reads data-tooltip off the content span now (the escaping path, and so the guarantee, is unchanged - the [data-pwned] breakout assertion passed throughout), and mountVs() takes hostStyle applied before init() because .vscomp-ele caps at max-width 250px, which silently made the old geometry inert. Suite: 414/414 across 26 specs; tsc/eslint/stylelint clean.
PR Test Results — ✅ all checks passed
Tested commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue number: resolves #487
What is the current behavior?
With
showValueAsTags: true, a tag gets a tooltip only if its label is judged to overflow, and thatjudgement is made against the wrong element and the wrong font.
setValueText()measured the label before the tag markup existed:$valueTextis.vscomp-value, so the container passed in is.vscomp-toggle-button:.vscomp-toggle-buttonclientWidth4px 22px 0 10px), the tag's border + padding + margin (~17px) and the 24px clear button — roughly 73px less14pxfrom.vscomp-wrapper.vscomp-value-tagrenders at12px, so the measurement runs ~17% wideThe two errors push in opposite directions and do not cancel, so tags that visibly truncate can end
up without a tooltip and tags that fit can end up with one.
What is the new behavior?
data-tooltip-ellipsis-only,so
tooltip-pluginevaluatesscrollWidth > offsetWidthon the rendered box at hover time.No overflow measurement happens at render at all.
.vscomp-value-tag, because the tag isinline-flexand the spancarries
width: calc(100% - 24px)— so the span clips while the tag reports no overflow of itsown. Measured on a genuinely clipped tag: tag
scrollWidth 260 / offsetWidth 262(not clipped),content
259 / 225(clipped). Putting the check on the tag would suppress every tooltip.Utils.willTextOverflow(),getTextMeasurer(),removeTextMeasurer()and the shared off-screenmeasurer node are removed; nothing replaces them.
This matches how the non-tag value text has always worked —
getToggleButtonHtml()already emitsgetTooltipAttrText(this.placeholder, /* ellipsisOnly */ true, true)and lets the plugin decide.Tags were the odd one out.
Deferring to hover fixes two cases a render-time measurement cannot handle at all:
display: nonereadsoffsetWidth === 0,so an eager check concludes "nothing is clipped" and nothing recomputes on reveal);
It also removes the forced synchronous layout the old code paid per tag.
Does this introduce a breaking change?
No prop, method or event changed shape, and nothing was removed from the public API
(
Utilsis internal — it is not attached towindow).Three DOM-level consequences are visible, though, and are worth calling out for reviewers:
.vscomp-value-tag.vscomp-value-tag-contentdata-tooltip-ellipsis-only="true"So
data-tooltipis no longer a proxy for "this tag overflows" — assert the tooltip appearing onhover instead. Hovering a tag's clear button no longer shows the tooltip; hovering the label
does. And more tags will show tooltips than before, which is the corrected behaviour rather than a
regression, but it is user-visible.
Other information
Tests —
cypress/e2e/tag-tooltip-overflow.cy.ts(6 cases). Written first and verified against abundle built from
master's ownsrc/, so the red run reflects the originally reported bug:4 of 6 red, every failure being
.tooltip-compnever appearing on hover over a clipped tag.masterselectedLabelRendereroutput.vscomp-text-measurernode existsmaster, green now (regression guard)+ n morecounter tag carries no tooltipThe cases assert what the user experiences — whether a tooltip appears on hover — rather than
the presence of an attribute, so they stay valid whichever side computes the overflow. Two traps are
handled explicitly and documented in the spec: the 200ms
tooltipEnterDelay(so "no tooltip" mustoutlast it rather than passing instantly), and the fact that hiding is
display: nonerather thanremoval (so absence is asserted on visibility, not existence).
perf-text-measurer.cy.tsis deleted — its 4 cases asserted the existence and teardown of theshared measurer node, which no longer exists.
tag-tooltip-overflow.cy.tsreplaces it and stillpins that no measurer is created.
Two follow-on test changes:
security-quote-escaping.cy.tsreadsdata-tooltipoff the content span now. The securityguarantee is unchanged — escaping still happens in
DomUtils.getAttributesText(), and the[data-pwned]breakout assertion passed throughout — only the selector moved.mountVs()gained an optionalhostStyleapplied beforeinit()..vscomp-eleshipsmax-width: 250px, which silently capped a test'swidth: 300px; the helper's JSDoc now recordsthe cap.
Verification: full suite 414/414 across 26 specs;
npm run validate(tsc + ESLint +Stylelint) clean. No build output is committed —
dist/,dist-archive/anddocs/assets/staypinned to
master, per the PR rule in.github/README.md.