Skip to content

fix(#487): decide tag tooltips from the rendered tag, not an off-screen measurement - #489

Open
gnbm wants to merge 1 commit into
masterfrom
gm/issue-487
Open

fix(#487): decide tag tooltips from the rendered tag, not an off-screen measurement#489
gnbm wants to merge 1 commit into
masterfrom
gm/issue-487

Conversation

@gnbm

@gnbm gnbm commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

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 that
judgement is made against the wrong element and the wrong font.

setValueText() measured the label before the tag markup existed:

const valueTooltipForTags = Utils.willTextOverflow($valueText.parentElement, label) ? ... : '';

$valueText is .vscomp-value, so the container passed in is .vscomp-toggle-button:

Compared against What the tag text really has
Width .vscomp-toggle-button clientWidth minus the button's padding (4px 22px 0 10px), the tag's border + padding + margin (~17px) and the 24px clear button — roughly 73px less
Font size inherited 14px from .vscomp-wrapper .vscomp-value-tag renders at 12px, so the measurement runs ~17% wide

The 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?

  • The tooltip attributes are emitted on the tag's content span with data-tooltip-ellipsis-only,
    so tooltip-plugin evaluates scrollWidth > offsetWidth on the rendered box at hover time.
    No overflow measurement happens at render at all.
  • The content span rather than .vscomp-value-tag, because 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 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-screen
    measurer node are removed; nothing replaces them.

This matches how the non-tag value text has always worked — getToggleButtonHtml() already emits
getTooltipAttrText(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:

  • a control first rendered inside a hidden container (display: none reads offsetWidth === 0,
    so an eager check concludes "nothing is clipped" and nothing recomputes on reveal);
  • after a resize, where tags reflow but a render-time answer goes stale.

It also removes the forced synchronous layout the old code paid per tag.

Does this introduce a breaking change?

  • Yes
  • No

No prop, method or event changed shape, and nothing was removed from the public API
(Utils is internal — it is not attached to window).

Three DOM-level consequences are visible, though, and are worth calling out for reviewers:

Before After
Element carrying the tooltip attributes .vscomp-value-tag .vscomp-value-tag-content
Which tags carry them only those judged to overflow all of them, with data-tooltip-ellipsis-only="true"
When overflow is evaluated at render, off-screen at hover, on the rendered element

So data-tooltip is no longer a proxy for "this tag overflows" — assert the tooltip appearing on
hover 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 a
bundle built from master's own src/, so the red run reflects the originally reported bug:
4 of 6 red, every failure being .tooltip-comp never appearing on hover over a clipped tag.

Case vs master
a tooltip appears on every clipped tag and on no tag that fits — 24 graded label lengths, asserting both outcomes occur so it cannot pass vacuously red
clipping introduced by consumer CSS on the tag itself red
tooltip text is the selectedLabelRenderer output red
no .vscomp-text-measurer node exists red
control first rendered inside a hidden container, then revealed green on master, green now (regression guard)
+ n more counter tag carries no tooltip green (guard)

The 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" must
outlast it rather than passing instantly), and the fact that hiding is display: none rather than
removal (so absence is asserted on visibility, not existence).

perf-text-measurer.cy.ts is deleted — its 4 cases asserted the existence and teardown of the
shared measurer node, which no longer exists. tag-tooltip-overflow.cy.ts replaces it and still
pins that no measurer is created.

Two follow-on test changes:

  • security-quote-escaping.cy.ts reads data-tooltip off the content span now. The security
    guarantee is unchanged — escaping still happens in DomUtils.getAttributesText(), and the
    [data-pwned] breakout assertion passed throughout — only the selector moved.
  • mountVs() gained an optional hostStyle applied before init(). .vscomp-ele ships
    max-width: 250px, which silently capped a test's width: 300px; the helper's JSDoc now records
    the cap.

Verification: full suite 414/414 across 26 specs; npm run validate (tsc + ESLint +
Stylelint) clean. No build output is committed — dist/, dist-archive/ and docs/assets/ stay
pinned to master, per the PR rule in .github/README.md.

…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.
@gnbm gnbm added the bug Something isn't working label Aug 8, 2026
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

PR Test Results — ✅ all checks passed

Check Result Time
Typecheck 3s
ESLint 2s
Stylelint 1s
CI Scripts 1s
Build 3s
a11y-aria-label.cy.ts ✅ 10/10 6s
a11y-close-clears-highlight.cy.ts ✅ 7/7 5s
a11y-escape-close.cy.ts ✅ 6/6 2s
a11y-listbox-multiselectable.cy.ts ✅ 3/3 1s
a11y-live-region.cy.ts ✅ 18/18 6s
a11y-reduced-motion.cy.ts ✅ 3/3 1s
a11y-required-error.cy.ts ✅ 20/20 4s
a11y-search-arrow-navigation.cy.ts ✅ 13/13 7s
a11y-select-all.cy.ts ✅ 7/7 3s
a11y-server-search-announcements.cy.ts ✅ 6/6 4s
a11y-target-size.cy.ts ✅ 5/5 2s
examples.cy.ts ✅ 219/219 1m41s
observer-listener-lifecycle.cy.ts ✅ 7/7 1s
perf-resize-throttle.cy.ts ✅ 3/3 1s
perf-scroll-aria.cy.ts ✅ 6/6 3s
secure-text-warning.cy.ts ✅ 4/4 1s
security-ampersand-storage.cy.ts ✅ 16/16 3s
security-chrome-label-props.cy.ts ✅ 10/10 1s
security-classnames-xss.cy.ts ✅ 2/2 1s
security-customdata-xss.cy.ts ✅ 2/2 1s
security-global-defaults.cy.ts ✅ 11/11 1s
security-hidden-input-name.cy.ts ✅ 7/7 1s
security-proto-value.cy.ts ✅ 9/9 1s
security-quote-escaping.cy.ts ✅ 12/12 2s
tag-tooltip-overflow.cy.ts ✅ 6/6 11s
timer-cleanup.cy.ts ✅ 2/2 1s

Tested commit: 0c8fa2e · Run #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tag tooltips: the overflow check measures the wrong box, so tooltips are both missed and added unnecessarily

1 participant