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}