From 0c8fa2e8534b08a497606981affe5def5f22147c Mon Sep 17 00:00:00 2001 From: gnbm Date: Fri, 7 Aug 2026 19:53:25 +0100 Subject: [PATCH] fix(#487): decide tag tooltips from the rendered tag, not an off-screen 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. --- cypress/e2e/perf-text-measurer.cy.ts | 107 ---------- cypress/e2e/security-quote-escaping.cy.ts | 14 +- cypress/e2e/tag-tooltip-overflow.cy.ts | 245 ++++++++++++++++++++++ cypress/support/mount.ts | 16 +- src/utils/utils.js | 75 ------- src/virtual-select.js | 29 ++- 6 files changed, 292 insertions(+), 194 deletions(-) delete mode 100644 cypress/e2e/perf-text-measurer.cy.ts create mode 100644 cypress/e2e/tag-tooltip-overflow.cy.ts diff --git a/cypress/e2e/perf-text-measurer.cy.ts b/cypress/e2e/perf-text-measurer.cy.ts deleted file mode 100644 index 119d5cd..0000000 --- a/cypress/e2e/perf-text-measurer.cy.ts +++ /dev/null @@ -1,107 +0,0 @@ -/** cSpell:ignore vscomp */ - -/** - * Measuring whether a tag's text overflows must not thrash layout. - * - * The check runs once per selected tag. It used to create a div, read getComputedStyle twice, - * append it to , read clientWidth and remove it again - and each DOM mutation - * invalidates layout for the read that follows, so rendering many tags produced a burst of - * forced synchronous layouts. - */ - -import { makeOptions, mountVs, unmountVs } from '../support/mount'; - -describe('Perf: one shared text measurer', { testIsolation: true }, () => { - const mountId = 'vs-measurer'; - - const mount = () => { - cy.viewport(1280, 800); - cy.visit('get-started'); - cy.window().then((win) => - mountVs(win, mountId, { options: makeOptions(5), multiple: true, showValueAsTags: true }), - ); - cy.get(`#${mountId}`).find('.vscomp-toggle-button').click(); - }; - - it('reuses a single off-screen node instead of one per tag', () => { - mount(); - - // Several tags, i.e. several willTextOverflow() calls per render. - ['o1', 'o2', 'o3', 'o4'].forEach((v) => { - cy.get(`#${mountId}`).find(`.vscomp-option[data-value="${v}"]`).click(); - }); - - cy.get('.vscomp-text-measurer').should('have.length', 1); - }); - - it('keeps the measurer out of the accessibility tree and out of flow', () => { - mount(); - cy.get(`#${mountId}`).find('.vscomp-option[data-value="o1"]').click(); - - cy.get('.vscomp-text-measurer').should('have.attr', 'aria-hidden', 'true'); - cy.get('.vscomp-text-measurer').should('have.css', 'position', 'absolute'); - }); - - it('still detects overflow, so tag tooltips are unaffected', () => { - cy.viewport(1280, 800); - cy.visit('get-started'); - cy.window().then((win) => - mountVs(win, mountId, { - options: [ - { label: 'A label far too long to fit inside a narrow tag without being clipped', value: 'long' }, - { label: 'Ok', value: 'short' }, - ], - multiple: true, - showValueAsTags: true, - }), - ); - cy.get(`#${mountId}`).find('.vscomp-toggle-button').click(); - - cy.get(`#${mountId}`).find('.vscomp-option[data-value="long"]').click(); - // Overflowing text still gets a tooltip, i.e. the measurement still works. - cy.get(`#${mountId}`).find('.vscomp-value-tag').first().should('have.attr', 'data-tooltip'); - - cy.window().then((win) => unmountVs(win, mountId)); - }); - - /** - * The one case that needs to be the *only* instance on the page, so it mounts on a docs page - * that hosts no demos of its own. - * - * `get-started` keeps two instances alive, which is why this case used to be written as - * `if (remaining === 0) { ...assert... }` — a condition that is false on every run, so its only - * assertion was skipped every time: it reported green while verifying nothing, and - * `Utils.removeTextMeasurer()` was never exercised at all. - * - * Destroying the page's own instances to force the condition works in a plain browser but hangs - * the Cypress runner, so this takes the other route: `properties` renders real content, loads - * the same bundle and starts with zero instances (measured), which makes our mount genuinely the - * last one. Nothing outside this test is torn down, and the precondition is asserted rather than - * assumed — if that page ever gains a demo, this fails loudly instead of going quiet again. - */ - it('removes the measurer once the last instance is destroyed', () => { - cy.viewport(1280, 800); - cy.visit('properties'); - - cy.window().should((win) => { - // @ts-expect-error - bundle global - expect(win.VirtualSelect.activeInstances.size, 'this page hosts no instances of its own').to.equal(0); - }); - - cy.window().then((win) => - mountVs(win, mountId, { options: makeOptions(5), multiple: true, showValueAsTags: true }), - ); - cy.get(`#${mountId}`).find('.vscomp-toggle-button').click(); - cy.get(`#${mountId}`).find('.vscomp-option[data-value="o1"]').click(); - cy.get('.vscomp-text-measurer').should('exist'); - - cy.window().then((win) => unmountVs(win, mountId)); - - cy.window().should((win) => { - // @ts-expect-error - bundle global - expect(win.VirtualSelect.activeInstances.size, 'no instances left').to.equal(0); - }); - - cy.get('.vscomp-text-measurer').should('not.exist'); - }); -}); diff --git a/cypress/e2e/security-quote-escaping.cy.ts b/cypress/e2e/security-quote-escaping.cy.ts index ecfd251..e35b67e 100644 --- a/cypress/e2e/security-quote-escaping.cy.ts +++ b/cypress/e2e/security-quote-escaping.cy.ts @@ -16,7 +16,8 @@ * 1. `data-value="${d.value}"` in `renderOptions()`, always; * 2. `data-tooltip="${label}"` via `getTooltipAttrText()` in the value-tag path, whose own * escaping was conditional on `containsHTML(label)` - so a payload with no tag at all - * went in raw and put a live attribute on the tag element. + * went in raw and put a live attribute on the tag element. (#487 later moved these + * attributes onto `.vscomp-value-tag-content`; the sink and its escaping are the same.) * * Sink 2 was a real breakout whenever escaping was off, and sink 1 whether it was on or off. * Pre-escaping the stored text only ever masked sink 2, and only in the escaping-on case. @@ -154,9 +155,14 @@ describe('Security: quotes are escaped at the attribute, not in the stored text' }); cy.get(`#${mountId}`).find('[data-pwned]').should('not.exist'); - cy.get(`#${mountId}`).find('.vscomp-value-tag').should(($tag) => { - expect($tag.attr('data-tooltip'), 'tooltip round-trip').to.equal(longAttrPayload); - }); + /** the attributes moved from `.vscomp-value-tag` to its content span in #487, so that + * tooltip-plugin evaluates ellipsis on the box that actually clips; the escaping path + * (`getAttributesText()`) and therefore this guarantee are unchanged */ + cy.get(`#${mountId}`) + .find('.vscomp-value-tag .vscomp-value-tag-content') + .should(($content) => { + expect($content.attr('data-tooltip'), 'tooltip round-trip').to.equal(longAttrPayload); + }); }); }); diff --git a/cypress/e2e/tag-tooltip-overflow.cy.ts b/cypress/e2e/tag-tooltip-overflow.cy.ts new file mode 100644 index 0000000..fa693f1 --- /dev/null +++ b/cypress/e2e/tag-tooltip-overflow.cy.ts @@ -0,0 +1,245 @@ +/** cSpell:ignore vscomp */ + +/** + * Issue #487 — with `showValueAsTags: true`, a tag should show a tooltip exactly when its + * rendered text is clipped. + * + * The old check measured the label off-screen against `.vscomp-toggle-button` (wrong box: ~73px + * wider than the space the text really gets) using that element's font (wrong size: 14px vs the + * tag's 12px), before the tag existed. Tags that visibly truncate could miss their tooltip and + * tags that fit could get one. + * + * The tooltip now sits on `.vscomp-value-tag-content` with `data-tooltip-ellipsis-only`, so + * tooltip-plugin runs `scrollWidth > offsetWidth` on the real box at hover time. + * + * These cases assert what the user experiences — whether a tooltip *appears on hover* — rather + * than the presence of an attribute, so they hold regardless of which side computes the overflow. + */ + +import { makeOptions, mountVs, unmountVs } from '../support/mount'; + +const mountId = 'vs-tag-tooltip'; + +/** `.vscomp-ele` caps at 250px, so a wider host needs maxWidth lifted too. */ +const host = (width: string) => ({ width, maxWidth: width }); + +const tagContent = () => cy.get(`#${mountId} .vscomp-value-tag[data-index] .vscomp-value-tag-content`); + +/** tooltip-plugin listens for delegated `mouseover`, so a real bubbling event is what shows it. */ +const hover = ($el: JQuery) => cy.wrap($el).trigger('mouseover', { force: true }); +const unhover = ($el: JQuery) => cy.wrap($el).trigger('mouseout', { force: true }); + +const assertTooltipShows = (text: string) => + cy.get('.tooltip-comp').should('be.visible').and('contain.text', text); + +/** + * Two traps here, both learned from watching this fail: + * + * - `tooltipEnterDelay` is 200ms, so "no tooltip" has to outlast it — asserting straight after the + * hover would pass before the tooltip had any chance to appear. + * - hiding is `display: none`, not removal (tooltip-plugin only removes the node when the *next* + * tooltip is built), so once any tooltip has been shown the node stays in the DOM. Absence has + * to be asserted on visibility, not existence. + */ +const TOOLTIP_ENTER_DELAY = 200; +const assertNoTooltipShows = () => { + cy.wait(TOOLTIP_ENTER_DELAY * 2); + cy.get('body').should(($body) => { + expect($body.find('.tooltip-comp:visible'), 'no visible tooltip').to.have.length(0); + }); +}; + +const isClipped = ($el: HTMLElement) => $el.scrollWidth > $el.offsetWidth; + +describe('Tag tooltips reflect real overflow (#487)', { testIsolation: true }, () => { + beforeEach(() => { + cy.viewport(1280, 800); + cy.visit('properties'); + }); + + afterEach(() => { + cy.window().then((win) => { + unmountVs(win, mountId); + win.document.getElementById(`${mountId}-css`)?.remove(); + }); + }); + + /** + * A graded range of label lengths, so the invariant cannot be satisfied by accident at one + * particular width: whichever labels the 250px host happens to clip must be exactly the ones + * that get a tooltip. + */ + it('shows a tooltip on every clipped tag and on no tag that fits', () => { + const labels = Array.from({ length: 24 }, (_, i) => `Tag ${'ab'.repeat(i)}`); + + cy.window().then((win) => + mountVs( + win, + mountId, + { + options: labels.map((label, i) => ({ label, value: `o${i}` })), + multiple: true, + showValueAsTags: true, + selectedValue: labels.map((_l, i) => `o${i}`), + }, + host('250px'), + ), + ); + + tagContent().should('have.length', labels.length); + + // Both outcomes must occur, otherwise the case would pass vacuously. + tagContent().then(($contents) => { + const clipped = $contents.toArray().filter(isClipped).length; + + expect(clipped, 'some tags are clipped').to.be.greaterThan(0); + expect(clipped, 'some tags are not clipped').to.be.lessThan($contents.length); + }); + + tagContent().each(($content) => { + const clipped = isClipped($content[0]); + + hover($content); + + if (clipped) { + assertTooltipShows($content.text().trim()); + } else { + assertNoTooltipShows(); + } + + unhover($content); + cy.get('body').should(($body) => { + expect($body.find('.tooltip-comp:visible'), 'tooltip hides again').to.have.length(0); + }); + }); + }); + + /** + * Consumer CSS narrows the tag while the toggle button stays wide — the split the old + * measurement was blind to, since it only ever looked at the button. + */ + it('sees clipping introduced by consumer CSS on the tag itself', () => { + cy.window().then((win) => { + const $style = win.document.createElement('style'); + $style.id = `${mountId}-css`; + $style.textContent = `#${mountId} .vscomp-value-tag { max-width: 120px; }`; + win.document.head.appendChild($style); + + mountVs( + win, + mountId, + { + options: [{ label: 'A label the tag cannot show in full', value: 'o1' }], + multiple: true, + showValueAsTags: true, + selectedValue: ['o1'], + }, + host('600px'), + ); + }); + + tagContent().should(($content) => { + expect(isClipped($content[0]), 'precondition: the tag really is clipped').to.equal(true); + }); + + tagContent().then(hover); + assertTooltipShows('A label the tag cannot show in full'); + }); + + /** + * Regression guard for the review finding on the first attempt at this fix: computing overflow + * at render time yields 0/0 inside a hidden container, so every tag silently lost its tooltip + * and nothing recomputed when it became visible. Deferring to hover cannot have that failure. + */ + it('still shows tooltips for a control first rendered inside a hidden container', () => { + cy.window().then((win) => { + const $host = mountVs( + win, + mountId, + { + options: [{ label: 'A label the tag cannot show in full at this width', value: 'o1' }], + multiple: true, + showValueAsTags: true, + selectedValue: ['o1'], + }, + { ...host('250px'), display: 'none' }, + ); + + // Rendered while hidden, then revealed — no re-render in between. + $host.style.display = ''; + }); + + tagContent().should(($content) => { + expect(isClipped($content[0]), 'precondition: the tag is clipped once visible').to.equal(true); + }); + + tagContent().then(hover); + assertTooltipShows('A label the tag cannot show in full at this width'); + }); + + it('uses the selectedLabelRenderer output as the tooltip text', () => { + cy.window().then((win) => { + const $style = win.document.createElement('style'); + $style.id = `${mountId}-css`; + $style.textContent = `#${mountId} .vscomp-value-tag { max-width: 120px; }`; + win.document.head.appendChild($style); + + mountVs( + win, + mountId, + { + options: [{ label: 'A label the tag cannot show in full', value: 'o1' }], + multiple: true, + showValueAsTags: true, + selectedValue: ['o1'], + selectedLabelRenderer: (option: { label: string }) => `${option.label} (rendered)`, + }, + host('600px'), + ); + }); + + tagContent().then(hover); + assertTooltipShows('A label the tag cannot show in full (rendered)'); + }); + + it('leaves the "+ n more" counter tag without a tooltip', () => { + cy.window().then((win) => + mountVs( + win, + mountId, + { + options: makeOptions(6), + multiple: true, + showValueAsTags: true, + noOfDisplayValues: 3, + selectedValue: ['o1', 'o2', 'o3', 'o4', 'o5', 'o6'], + }, + host('250px'), + ), + ); + + cy.get(`#${mountId} .vscomp-value-tag.more-value-count`) + .should('exist') + .should('not.have.attr', 'data-tooltip'); + }); + + /** The rendered box is the measurement now, so no off-screen measurer node may exist. */ + it('creates no shared off-screen measurer node', () => { + cy.window().then((win) => + mountVs( + win, + mountId, + { + options: makeOptions(5), + multiple: true, + showValueAsTags: true, + selectedValue: ['o1', 'o2', 'o3'], + }, + host('250px'), + ), + ); + + tagContent().should('have.length', 3); + cy.get('.vscomp-text-measurer').should('not.exist'); + }); +}); diff --git a/cypress/support/mount.ts b/cypress/support/mount.ts index e848029..02bc979 100644 --- a/cypress/support/mount.ts +++ b/cypress/support/mount.ts @@ -27,13 +27,27 @@ export function unmountVs(win: Window, mountId: string): void { /** * Create a fresh host element and initialise a VirtualSelect on it. * + * `hostStyle` is applied to the host **before** `init()`, so the instance is built at the + * geometry the test intends. Note `.vscomp-ele` ships `max-width: 250px`, so a test that wants a + * wider host has to set `maxWidth` as well as `width` — setting `width` alone is silently capped. + * * @returns the host element the instance was mounted on */ -export function mountVs(win: Window, mountId: string, options: VsOptions): HTMLElement { +export function mountVs( + win: Window, + mountId: string, + options: VsOptions, + hostStyle?: Partial, +): HTMLElement { unmountVs(win, mountId); const $ele = win.document.createElement('div'); $ele.id = mountId; + + if (hostStyle) { + Object.assign($ele.style, hostStyle); + } + win.document.body.appendChild($ele); // @ts-expect-error - VirtualSelect is attached to window by the bundle diff --git a/src/utils/utils.js b/src/utils/utils.js index 485d437..7662e66 100644 --- a/src/utils/utils.js +++ b/src/utils/utils.js @@ -187,40 +187,6 @@ export class Utils { return text.normalize('NFD').replace(NON_WORD_CHARS_REGEX, ''); } - /** - * @static - * @param {*} container - * @param {string} text - * @return {boolean} - * @memberof Utils - */ - static willTextOverflow(container, text) { - /** - * Called once per selected tag to decide whether that tag needs a tooltip. - * - * It used to create a div, read two separate getComputedStyle results, append it to - * , read clientWidth and remove it again - so every tag paid an element creation - * plus two DOM mutations, and each mutation invalidates layout for the read that - * follows. Rendering many tags therefore meant a burst of forced synchronous layouts. - * - * One reusable off-screen node instead, and one getComputedStyle read for every property. - * The node stays out of flow and is aria-hidden, so it cannot affect layout or be - * announced, and it is removed once the last instance is destroyed. - */ - const $measurer = Utils.getTextMeasurer(); - const { fontSize, fontFamily, fontWeight, letterSpacing } = window.getComputedStyle(container); - - $measurer.style.fontSize = fontSize; - $measurer.style.fontFamily = fontFamily; - /** weight and tracking change advance width too, so ignoring them under-reported - * overflow and could drop a tooltip that was actually needed */ - $measurer.style.fontWeight = fontWeight; - $measurer.style.letterSpacing = letterSpacing; - $measurer.textContent = text; - - return $measurer.clientWidth > container.clientWidth; - } - /** * Whether the user has asked the operating system to reduce motion. * @@ -234,41 +200,6 @@ export class Utils { return typeof window.matchMedia === 'function' && window.matchMedia('(prefers-reduced-motion: reduce)').matches; } - /** - * The shared, lazily created off-screen node used to measure text width. - * - * @static - * @returns {HTMLElement} - */ - static getTextMeasurer() { - if (!Utils.$textMeasurer || !Utils.$textMeasurer.isConnected) { - const $measurer = document.createElement('div'); - - $measurer.className = 'vscomp-text-measurer'; - $measurer.setAttribute('aria-hidden', 'true'); - $measurer.style.cssText = - 'position:absolute;top:0;left:-9999px;visibility:hidden;white-space:nowrap;pointer-events:none;'; - - document.body.appendChild($measurer); - Utils.$textMeasurer = $measurer; - } - - return Utils.$textMeasurer; - } - - /** - * Drop the shared measuring node, so nothing of ours is left in the document once the last - * instance has gone. - * - * @static - */ - static removeTextMeasurer() { - if (Utils.$textMeasurer) { - Utils.$textMeasurer.remove(); - Utils.$textMeasurer = null; - } - } - /** * @static * @param {string} text @@ -474,9 +405,3 @@ export class Utils { } } -/** - * Shared off-screen node used to measure text width, created on first use and removed when - * the last VirtualSelect instance is destroyed. - * @type {HTMLElement | null} - */ -Utils.$textMeasurer = null; diff --git a/src/virtual-select.js b/src/virtual-select.js index 69124fe..2b6a02c 100644 --- a/src/virtual-select.js +++ b/src/virtual-select.js @@ -1035,8 +1035,6 @@ export class VirtualSelect { if (VirtualSelect.activeInstances.size === 0) { VirtualSelect.removeGlobalListeners(); VirtualSelect.disconnectDomObserver(); - /** the shared text measurer is the last page-level node we own */ - Utils.removeTextMeasurer(); } } @@ -2157,9 +2155,26 @@ export class VirtualSelect { selectedValuesCount += 1; if (showValueAsTags) { - // Will cause text overflow in runtime and if so,the tooltip information is prepared - const valueTooltipForTags = Utils.willTextOverflow($valueText.parentElement, label) - ? this.getTooltipAttrText(label, false, true) : ''; + /** + * The tooltip is attached to the tag's *content* span, with ellipsisOnly, so the + * tooltip plugin runs `scrollWidth > offsetWidth` on the real box at hover time. + * + * This used to be decided here, before the tag existed, by measuring the label + * off-screen against `.vscomp-toggle-button` - an element ~73px wider than the space + * the tag text actually gets, at that element's 14px rather than the tag's 12px. Both + * errors are gone by construction once the rendered box is the measurement (#487). + * + * The content span rather than `.vscomp-value-tag`: the tag is `inline-flex` and its + * content span carries `width: calc(100% - 24px)`, so the span clips while the tag + * never reports an overflow of its own - measured, a clipped tag reads + * scrollWidth 260 / offsetWidth 262. + * + * Deferring to hover also means no layout work at render, and a correct answer after + * a resize or when the control is first rendered inside a hidden container - none of + * which a render-time measurement can give. It matches how the non-tag value text has + * always worked (see getToggleButtonHtml). + */ + const valueTagTooltip = this.getTooltipAttrText(label, true, true); /** markup in the label would otherwise land in the accessible name; a double * quote in it would break out of the attribute entirely */ @@ -2170,8 +2185,8 @@ export class VirtualSelect { ariaLabelClearBtnTxt = `aria-label="${stripHtmlLabel}, ${clearButtonText}"`; } - const valueTagHtml = ` - ${label} + const valueTagHtml = ` + ${label}