From 559026bbf3ce2a74cbb7682663596895219c79d7 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 29 Dec 2023 03:40:44 -0800 Subject: [PATCH 1/3] Implement and default to text metrics measure strategy Fixes #3449 --- src/browser/services/CharSizeService.ts | 63 ++++++++++++++++++------- 1 file changed, 45 insertions(+), 18 deletions(-) diff --git a/src/browser/services/CharSizeService.ts b/src/browser/services/CharSizeService.ts index 614b9b3005..ef0b1ab4e6 100644 --- a/src/browser/services/CharSizeService.ts +++ b/src/browser/services/CharSizeService.ts @@ -8,12 +8,6 @@ import { EventEmitter } from 'common/EventEmitter'; import { ICharSizeService } from 'browser/services/Services'; import { Disposable } from 'common/Lifecycle'; - -const enum MeasureSettings { - REPEAT = 32 -} - - export class CharSizeService extends Disposable implements ICharSizeService { public serviceBrand: undefined; @@ -32,7 +26,11 @@ export class CharSizeService extends Disposable implements ICharSizeService { @IOptionsService private readonly _optionsService: IOptionsService ) { super(); - this._measureStrategy = new DomMeasureStrategy(document, parentElement, this._optionsService); + try { + this._measureStrategy = new TextMetricsMeasureStrategy(this._optionsService); + } catch { + this._measureStrategy = new DomMeasureStrategy(document, parentElement, this._optionsService); + } this.register(this._optionsService.onMultipleOptionChange(['fontFamily', 'fontSize'], () => this.measure())); } @@ -47,12 +45,7 @@ export class CharSizeService extends Disposable implements ICharSizeService { } interface IMeasureStrategy { - measure(): IReadonlyMeasureResult; -} - -interface IReadonlyMeasureResult { - readonly width: number; - readonly height: number; + measure(): Readonly; } interface IMeasureResult { @@ -60,8 +53,10 @@ interface IMeasureResult { height: number; } -// TODO: For supporting browsers we should also provide a CanvasCharDimensionsProvider that uses -// ctx.measureText +const enum DomMeasureStrategyConstants { + REPEAT = 32 +} + class DomMeasureStrategy implements IMeasureStrategy { private _result: IMeasureResult = { width: 0, height: 0 }; private _measureElement: HTMLElement; @@ -73,14 +68,14 @@ class DomMeasureStrategy implements IMeasureStrategy { ) { this._measureElement = this._document.createElement('span'); this._measureElement.classList.add('xterm-char-measure-element'); - this._measureElement.textContent = 'W'.repeat(MeasureSettings.REPEAT); + this._measureElement.textContent = 'W'.repeat(DomMeasureStrategyConstants.REPEAT); this._measureElement.setAttribute('aria-hidden', 'true'); this._measureElement.style.whiteSpace = 'pre'; this._measureElement.style.fontKerning = 'none'; this._parentElement.appendChild(this._measureElement); } - public measure(): IReadonlyMeasureResult { + public measure(): Readonly { this._measureElement.style.fontFamily = this._optionsService.rawOptions.fontFamily; this._measureElement.style.fontSize = `${this._optionsService.rawOptions.fontSize}px`; @@ -93,10 +88,42 @@ class DomMeasureStrategy implements IMeasureStrategy { // If values are 0 then the element is likely currently display:none, in which case we should // retain the previous value. if (geometry.width !== 0 && geometry.height !== 0) { - this._result.width = geometry.width / MeasureSettings.REPEAT; + this._result.width = geometry.width / DomMeasureStrategyConstants.REPEAT; this._result.height = Math.ceil(geometry.height); } return this._result; } } + +class TextMetricsMeasureStrategy implements IMeasureStrategy { + private _result: IMeasureResult = { width: 0, height: 0 }; + private _canvas: OffscreenCanvas; + private _ctx: OffscreenCanvasRenderingContext2D; + + constructor( + private _optionsService: IOptionsService + ) { + // This will throw if any required API is not supported + this._canvas = new OffscreenCanvas(100, 100); + this._ctx = this._canvas.getContext('2d')!; + const a = this._ctx.measureText('W'); + if (!('width' in a && 'fontBoundingBoxAscent' in a && 'fontBoundingBoxDescent' in a)) { + throw new Error('Required font metrics not supported'); + } + } + + public measure(): Readonly { + this._ctx.font = `${this._optionsService.rawOptions.fontSize}px ${this._optionsService.rawOptions.fontFamily}`; + + const metrics = this._ctx.measureText('W'); + + // Sanity check that the values are not 0 + if (metrics.width !== 0 && metrics.fontBoundingBoxAscent !== 0) { + this._result.width = metrics.width; + this._result.height = Math.ceil(metrics.fontBoundingBoxAscent + metrics.fontBoundingBoxDescent); + } + + return this._result; + } +} From 379c382fe7465ff91398dfbf84d7b87be8b5eb05 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 29 Dec 2023 03:47:44 -0800 Subject: [PATCH 2/3] Pull common measure parts into base class --- src/browser/services/CharSizeService.ts | 48 ++++++++++++------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/src/browser/services/CharSizeService.ts b/src/browser/services/CharSizeService.ts index ef0b1ab4e6..da14b67ddf 100644 --- a/src/browser/services/CharSizeService.ts +++ b/src/browser/services/CharSizeService.ts @@ -27,9 +27,9 @@ export class CharSizeService extends Disposable implements ICharSizeService { ) { super(); try { - this._measureStrategy = new TextMetricsMeasureStrategy(this._optionsService); + this._measureStrategy = this.register(new TextMetricsMeasureStrategy(this._optionsService)); } catch { - this._measureStrategy = new DomMeasureStrategy(document, parentElement, this._optionsService); + this._measureStrategy = this.register(new DomMeasureStrategy(document, parentElement, this._optionsService)); } this.register(this._optionsService.onMultipleOptionChange(['fontFamily', 'fontSize'], () => this.measure())); } @@ -57,8 +57,22 @@ const enum DomMeasureStrategyConstants { REPEAT = 32 } -class DomMeasureStrategy implements IMeasureStrategy { - private _result: IMeasureResult = { width: 0, height: 0 }; +abstract class BaseMeasureStategy extends Disposable implements IMeasureStrategy { + protected _result: IMeasureResult = { width: 0, height: 0 }; + + protected _validateAndSet(width: number | undefined, height: number | undefined): void { + // If values are 0 then the element is likely currently display:none, in which case we should + // retain the previous value. + if (width !== undefined && width > 0 && height !== undefined && height > 0) { + this._result.width = width; + this._result.height = height; + } + } + + public abstract measure(): Readonly; +} + +class DomMeasureStrategy extends BaseMeasureStategy { private _measureElement: HTMLElement; constructor( @@ -66,6 +80,7 @@ class DomMeasureStrategy implements IMeasureStrategy { private _parentElement: HTMLElement, private _optionsService: IOptionsService ) { + super(); this._measureElement = this._document.createElement('span'); this._measureElement.classList.add('xterm-char-measure-element'); this._measureElement.textContent = 'W'.repeat(DomMeasureStrategyConstants.REPEAT); @@ -80,30 +95,20 @@ class DomMeasureStrategy implements IMeasureStrategy { this._measureElement.style.fontSize = `${this._optionsService.rawOptions.fontSize}px`; // Note that this triggers a synchronous layout - const geometry = { - height: Number(this._measureElement.offsetHeight), - width: Number(this._measureElement.offsetWidth) - }; - - // If values are 0 then the element is likely currently display:none, in which case we should - // retain the previous value. - if (geometry.width !== 0 && geometry.height !== 0) { - this._result.width = geometry.width / DomMeasureStrategyConstants.REPEAT; - this._result.height = Math.ceil(geometry.height); - } + this._validateAndSet(Number(this._measureElement.offsetWidth) / DomMeasureStrategyConstants.REPEAT, Number(this._measureElement.offsetHeight)); return this._result; } } -class TextMetricsMeasureStrategy implements IMeasureStrategy { - private _result: IMeasureResult = { width: 0, height: 0 }; +class TextMetricsMeasureStrategy extends BaseMeasureStategy { private _canvas: OffscreenCanvas; private _ctx: OffscreenCanvasRenderingContext2D; constructor( private _optionsService: IOptionsService ) { + super(); // This will throw if any required API is not supported this._canvas = new OffscreenCanvas(100, 100); this._ctx = this._canvas.getContext('2d')!; @@ -115,15 +120,8 @@ class TextMetricsMeasureStrategy implements IMeasureStrategy { public measure(): Readonly { this._ctx.font = `${this._optionsService.rawOptions.fontSize}px ${this._optionsService.rawOptions.fontFamily}`; - const metrics = this._ctx.measureText('W'); - - // Sanity check that the values are not 0 - if (metrics.width !== 0 && metrics.fontBoundingBoxAscent !== 0) { - this._result.width = metrics.width; - this._result.height = Math.ceil(metrics.fontBoundingBoxAscent + metrics.fontBoundingBoxDescent); - } - + this._validateAndSet(metrics.width, metrics.fontBoundingBoxAscent + metrics.fontBoundingBoxDescent); return this._result; } } From 82d50a4f89d9699deef35fbfa0ac214f83992186 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 29 Dec 2023 03:57:37 -0800 Subject: [PATCH 3/3] Allow fit addon test to work when hidden --- addons/addon-fit/test/FitAddon.api.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/addons/addon-fit/test/FitAddon.api.ts b/addons/addon-fit/test/FitAddon.api.ts index 5611972ee0..24641fc6c4 100644 --- a/addons/addon-fit/test/FitAddon.api.ts +++ b/addons/addon-fit/test/FitAddon.api.ts @@ -4,7 +4,7 @@ */ import { assert } from 'chai'; -import { openTerminal, launchBrowser } from '../../../out-test/api/TestUtils'; +import { openTerminal, launchBrowser, timeout } from '../../../out-test/api/TestUtils'; import { Browser, Page } from '@playwright/test'; const APP = 'http://127.0.0.1:3001/test'; @@ -75,7 +75,15 @@ describe('FitAddon', () => { await page.evaluate(`window.term = new Terminal()`); await page.evaluate(`window.term.open(document.querySelector('#terminal-container'))`); await loadFit(); - assert.equal(await page.evaluate(`window.fit.proposeDimensions()`), undefined); + const dimensions: { cols: number, rows: number } | undefined = await page.evaluate(`window.fit.proposeDimensions()`); + // The value of dims will be undefined if the char measure strategy falls back to the DOM + // method, so only assert if it's not undefined. + if (dimensions) { + assert.isAbove(dimensions.cols, 85); + assert.isBelow(dimensions.cols, 88); + assert.isAbove(dimensions.rows, 24); + assert.isBelow(dimensions.rows, 29); + } }); });