From 1cef30cca0171eaae7e2de231587360431af2ee1 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sun, 13 Aug 2023 11:27:57 -0700 Subject: [PATCH 1/2] Enforce half minimum contrast ratio for dim text Provided MCR is not too high, this ensures dim text differs from normal text and has a reasonable difference in contrast. Fixes #4262 --- .../test/WebglRenderer.api.ts | 73 ++++++++++++++++++- src/browser/Types.d.ts | 3 + .../renderer/dom/DomRendererRowFactory.ts | 18 ++++- src/browser/renderer/shared/CharAtlasUtils.ts | 3 +- src/browser/renderer/shared/TextureAtlas.ts | 24 ++++-- src/browser/services/ThemeService.ts | 8 +- 6 files changed, 113 insertions(+), 16 deletions(-) diff --git a/addons/xterm-addon-webgl/test/WebglRenderer.api.ts b/addons/xterm-addon-webgl/test/WebglRenderer.api.ts index cca37e0a..8d84b200 100644 --- a/addons/xterm-addon-webgl/test/WebglRenderer.api.ts +++ b/addons/xterm-addon-webgl/test/WebglRenderer.api.ts @@ -789,7 +789,7 @@ describe('WebGL Renderer Integration Tests', async () => { await pollFor(page, () => getCellColor(6, 2), [0xad, 0x7f, 0xa8, 255]); await pollFor(page, () => getCellColor(7, 2), [0x34, 0xe2, 0xe2, 255]); await pollFor(page, () => getCellColor(8, 2), [0xee, 0xee, 0xec, 255]); - // Setting and check for minimum contrast values, note that these are note + // Setting and check for minimum contrast values, note that these are not // exact to the contrast ratio, if the increase luminance algorithm // changes then these will probably fail await page.evaluate(`window.term.options.minimumContrastRatio = 10;`); @@ -858,7 +858,7 @@ describe('WebGL Renderer Integration Tests', async () => { await pollFor(page, () => getCellColor(6, 2), [0xad, 0x7f, 0xa8, 255]); await pollFor(page, () => getCellColor(7, 2), [0x34, 0xe2, 0xe2, 255]); await pollFor(page, () => getCellColor(8, 2), [0xee, 0xee, 0xec, 255]); - // Setting and check for minimum contrast values, note that these are note + // Setting and check for minimum contrast values, note that these are not // exact to the contrast ratio, if the increase luminance algorithm // changes then these will probably fail await page.evaluate(`window.term.options.minimumContrastRatio = 10;`); @@ -879,6 +879,75 @@ describe('WebGL Renderer Integration Tests', async () => { await pollFor(page, () => getCellColor(7, 2), [13, 67, 67, 255]); await pollFor(page, () => getCellColor(8, 2), [64, 64, 64, 255]); }); + + itWebgl('should enforce half the contrast for dim cells', async () => { + const theme: ITheme = { + background: '#ffffff', + black: '#2e3436', + red: '#cc0000', + green: '#4e9a06', + yellow: '#c4a000', + blue: '#3465a4', + magenta: '#75507b', + cyan: '#06989a', + white: '#d3d7cf', + brightBlack: '#555753', + brightRed: '#ef2929', + brightGreen: '#8ae234', + brightYellow: '#fce94f', + brightBlue: '#729fcf', + brightMagenta: '#ad7fa8', + brightCyan: '#34e2e2', + brightWhite: '#eeeeec' + }; + await page.evaluate(` + window.term.options.theme = ${JSON.stringify(theme)}; + window.term.options.minimumContrastRatio = 1; + `); + // Block characters ignore block elements so a different char is used here + await writeSync(page, + '\\x1b[2m' + + `\\x1b[30m■\\x1b[31m■\\x1b[32m■\\x1b[33m■\\x1b[34m■\\x1b[35m■\\x1b[36m■\\x1b[37m■\\r\\n` + + `\\x1b[90m■\\x1b[91m■\\x1b[92m■\\x1b[93m■\\x1b[94m■\\x1b[95m■\\x1b[96m■\\x1b[97m■` + ); + // Validate before minimumContrastRatio is applied + await pollFor(page, () => getCellColor(1, 1), [Math.floor((255 + 0x2e) / 2), Math.floor((255 + 0x34) / 2), Math.floor((255 + 0x36) / 2), 255]); + await pollFor(page, () => getCellColor(2, 1), [Math.floor((255 + 0xcc) / 2), Math.floor((255 + 0x00) / 2), Math.floor((255 + 0x00) / 2), 255]); + await pollFor(page, () => getCellColor(3, 1), [Math.floor((255 + 0x4e) / 2), Math.floor((255 + 0x9a) / 2), Math.floor((255 + 0x06) / 2), 255]); + await pollFor(page, () => getCellColor(4, 1), [Math.floor((255 + 0xc4) / 2), Math.floor((255 + 0xa0) / 2), Math.floor((255 + 0x00) / 2), 255]); + await pollFor(page, () => getCellColor(5, 1), [Math.floor((255 + 0x34) / 2), Math.floor((255 + 0x65) / 2), Math.floor((255 + 0xa4) / 2), 255]); + await pollFor(page, () => getCellColor(6, 1), [Math.floor((255 + 0x75) / 2), Math.floor((255 + 0x50) / 2), Math.floor((255 + 0x7b) / 2), 255]); + await pollFor(page, () => getCellColor(7, 1), [Math.floor((255 + 0x06) / 2), Math.floor((255 + 0x98) / 2), Math.floor((255 + 0x9a) / 2), 255]); + await pollFor(page, () => getCellColor(8, 1), [Math.floor((255 + 0xd3) / 2), Math.floor((255 + 0xd7) / 2), Math.floor((255 + 0xcf) / 2), 255]); + await pollFor(page, () => getCellColor(1, 2), [Math.floor((255 + 0x55) / 2), Math.floor((255 + 0x57) / 2), Math.floor((255 + 0x53) / 2), 255]); + await pollFor(page, () => getCellColor(2, 2), [Math.floor((255 + 0xef) / 2), Math.floor((255 + 0x29) / 2), Math.floor((255 + 0x29) / 2), 255]); + await pollFor(page, () => getCellColor(3, 2), [Math.floor((255 + 0x8a) / 2), Math.floor((255 + 0xe2) / 2), Math.floor((255 + 0x34) / 2), 255]); + await pollFor(page, () => getCellColor(4, 2), [Math.floor((255 + 0xfc) / 2), Math.floor((255 + 0xe9) / 2), Math.floor((255 + 0x4f) / 2), 255]); + await pollFor(page, () => getCellColor(5, 2), [Math.floor((255 + 0x72) / 2), Math.floor((255 + 0x9f) / 2), Math.floor((255 + 0xcf) / 2), 255]); + await pollFor(page, () => getCellColor(6, 2), [Math.floor((255 + 0xad) / 2), Math.floor((255 + 0x7f) / 2), Math.floor((255 + 0xa8) / 2), 255]); + await pollFor(page, () => getCellColor(7, 2), [Math.floor((255 + 0x34) / 2), Math.floor((255 + 0xe2) / 2), Math.floor((255 + 0xe2) / 2), 255]); + await pollFor(page, () => getCellColor(8, 2), [Math.floor((255 + 0xee) / 2), Math.floor((255 + 0xee) / 2), Math.floor((255 + 0xec) / 2), 255]); + // Setting and check for minimum contrast values, note that these are not + // exact to the contrast ratio, if the increase luminance algorithm + // changes then these will probably fail + await page.evaluate(`window.term.options.minimumContrastRatio = 10;`); + await pollFor(page, () => getCellColor(1, 1), [150, 153, 154, 255]); + await pollFor(page, () => getCellColor(2, 1), [229, 127, 127, 255]); + await pollFor(page, () => getCellColor(3, 1), [63, 124, 4, 255]); + await pollFor(page, () => getCellColor(4, 1), [127, 104, 0, 255]); + await pollFor(page, () => getCellColor(5, 1), [153, 178, 209, 255]); + await pollFor(page, () => getCellColor(6, 1), [186, 167, 189, 255]); + await pollFor(page, () => getCellColor(7, 1), [4, 122, 124, 255]); + await pollFor(page, () => getCellColor(8, 1), [110, 112, 108, 255]); + await pollFor(page, () => getCellColor(1, 2), [170, 171, 169, 255]); + await pollFor(page, () => getCellColor(2, 2), [215, 36, 36, 255]); + await pollFor(page, () => getCellColor(3, 2), [72, 117, 25, 255]); + await pollFor(page, () => getCellColor(4, 2), [117, 109, 36, 255]); + await pollFor(page, () => getCellColor(5, 2), [72, 103, 135, 255]); + await pollFor(page, () => getCellColor(6, 2), [125, 91, 121, 255]); + await pollFor(page, () => getCellColor(7, 2), [25, 117, 117, 255]); + await pollFor(page, () => getCellColor(8, 2), [111, 111, 110, 255]); + }); }); describe('selectionBackground', async () => { diff --git a/src/browser/Types.d.ts b/src/browser/Types.d.ts index 5337a169..d32b6099 100644 --- a/src/browser/Types.d.ts +++ b/src/browser/Types.d.ts @@ -136,7 +136,10 @@ export interface IColorSet { selectionInactiveBackgroundTransparent: IColor; selectionInactiveBackgroundOpaque: IColor; ansi: IColor[]; + /** Maps original colors to colors that respect minimum contrast ratio. */ contrastCache: IColorContrastCache; + /** Maps original colors to colors that respect _half_ of the minimum contrast ratio. */ + halfContrastCache: IColorContrastCache; } export type ReadonlyColorSet = Readonly> & { ansi: Readonly['ansi']> }; diff --git a/src/browser/renderer/dom/DomRendererRowFactory.ts b/src/browser/renderer/dom/DomRendererRowFactory.ts index 8a428c96..8ecaad42 100644 --- a/src/browser/renderer/dom/DomRendererRowFactory.ts +++ b/src/browser/renderer/dom/DomRendererRowFactory.ts @@ -14,6 +14,7 @@ import { JoinedCellData } from 'browser/services/CharacterJoinerService'; import { excludeFromContrastRatioDemands } from 'browser/renderer/shared/RendererUtils'; import { AttributeData } from 'common/buffer/AttributeData'; import { WidthCache } from 'browser/renderer/dom/WidthCache'; +import { IColorContrastCache } from 'browser/Types'; export const enum RowCss { @@ -444,15 +445,19 @@ export class DomRendererRowFactory { } // Try get from cache first, only use the cache when there are no decoration overrides + const cache = this._getContrastCache(cell); let adjustedColor: IColor | undefined | null = undefined; if (!bgOverride && !fgOverride) { - adjustedColor = this._themeService.colors.contrastCache.getColor(bg.rgba, fg.rgba); + adjustedColor = cache.getColor(bg.rgba, fg.rgba); } // Calculate and store in cache if (adjustedColor === undefined) { - adjustedColor = color.ensureContrastRatio(bgOverride || bg, fgOverride || fg, this._optionsService.rawOptions.minimumContrastRatio); - this._themeService.colors.contrastCache.setColor((bgOverride || bg).rgba, (fgOverride || fg).rgba, adjustedColor ?? null); + // Dim cells only require half the contrast, otherwise they wouldn't be distinguishable from + // non-dim cells + const ratio = this._optionsService.rawOptions.minimumContrastRatio / (cell.isDim() ? 2 : 1); + adjustedColor = color.ensureContrastRatio(bgOverride || bg, fgOverride || fg, ratio); + cache.setColor((bgOverride || bg).rgba, (fgOverride || fg).rgba, adjustedColor ?? null); } if (adjustedColor) { @@ -463,6 +468,13 @@ export class DomRendererRowFactory { return false; } + private _getContrastCache(cell: ICellData): IColorContrastCache { + if (cell.isDim()) { + return this._themeService.colors.halfContrastCache; + } + return this._themeService.colors.contrastCache; + } + private _addStyle(element: HTMLElement, style: string): void { element.setAttribute('style', `${element.getAttribute('style') || ''}${style};`); } diff --git a/src/browser/renderer/shared/CharAtlasUtils.ts b/src/browser/renderer/shared/CharAtlasUtils.ts index 89b21dbc..955bd4e6 100644 --- a/src/browser/renderer/shared/CharAtlasUtils.ts +++ b/src/browser/renderer/shared/CharAtlasUtils.ts @@ -24,7 +24,8 @@ export function generateConfig(deviceCellWidth: number, deviceCellHeight: number // For the static char atlas, we only use the first 16 colors, but we need all 256 for the // dynamic character atlas. ansi: colors.ansi.slice(), - contrastCache: colors.contrastCache + contrastCache: colors.contrastCache, + halfContrastCache: colors.halfContrastCache }; return { customGlyphs: options.customGlyphs, diff --git a/src/browser/renderer/shared/TextureAtlas.ts b/src/browser/renderer/shared/TextureAtlas.ts index d73d4ba6..6a572ce0 100644 --- a/src/browser/renderer/shared/TextureAtlas.ts +++ b/src/browser/renderer/shared/TextureAtlas.ts @@ -15,6 +15,7 @@ import { FourKeyMap } from 'common/MultiKeyMap'; import { IdleTaskQueue } from 'common/TaskQueue'; import { IBoundingBox, ICharAtlasConfig, IRasterizedGlyph, IRequestRedrawEvent, ITextureAtlas } from 'browser/renderer/shared/Types'; import { EventEmitter } from 'common/EventEmitter'; +import { IColorContrastCache } from 'browser/Types'; /** * A shared object which is used to draw nothing for a particular cell. @@ -309,8 +310,7 @@ export class TextureAtlas implements ITextureAtlas { } private _getForegroundColor(bg: number, bgColorMode: number, bgColor: number, fg: number, fgColorMode: number, fgColor: number, inverse: boolean, dim: boolean, bold: boolean, excludeFromContrastRatioDemands: boolean): IColor { - // TODO: Pass dim along to get min contrast? - const minimumContrastColor = this._getMinimumContrastColor(bg, bgColorMode, bgColor, fg, fgColorMode, fgColor, false, bold, excludeFromContrastRatioDemands); + const minimumContrastColor = this._getMinimumContrastColor(bg, bgColorMode, bgColor, fg, fgColorMode, fgColor, false, bold, dim, excludeFromContrastRatioDemands); if (minimumContrastColor) { return minimumContrastColor; } @@ -385,23 +385,26 @@ export class TextureAtlas implements ITextureAtlas { } } - private _getMinimumContrastColor(bg: number, bgColorMode: number, bgColor: number, fg: number, fgColorMode: number, fgColor: number, inverse: boolean, bold: boolean, excludeFromContrastRatioDemands: boolean): IColor | undefined { + private _getMinimumContrastColor(bg: number, bgColorMode: number, bgColor: number, fg: number, fgColorMode: number, fgColor: number, inverse: boolean, bold: boolean, dim: boolean, excludeFromContrastRatioDemands: boolean): IColor | undefined { if (this._config.minimumContrastRatio === 1 || excludeFromContrastRatioDemands) { return undefined; } // Try get from cache first - const adjustedColor = this._config.colors.contrastCache.getColor(bg, fg); + const cache = this._getContrastCache(dim); + const adjustedColor = cache.getColor(bg, fg); if (adjustedColor !== undefined) { return adjustedColor || undefined; } const bgRgba = this._resolveBackgroundRgba(bgColorMode, bgColor, inverse); const fgRgba = this._resolveForegroundRgba(fgColorMode, fgColor, inverse, bold); - const result = rgba.ensureContrastRatio(bgRgba, fgRgba, this._config.minimumContrastRatio); + // Dim cells only require half the contrast, otherwise they wouldn't be distinguishable from + // non-dim cells + const result = rgba.ensureContrastRatio(bgRgba, fgRgba, this._config.minimumContrastRatio / (dim ? 2 : 1)); if (!result) { - this._config.colors.contrastCache.setColor(bg, fg, null); + cache.setColor(bg, fg, null); return undefined; } @@ -410,11 +413,18 @@ export class TextureAtlas implements ITextureAtlas { (result >> 16) & 0xFF, (result >> 8) & 0xFF ); - this._config.colors.contrastCache.setColor(bg, fg, color); + cache.setColor(bg, fg, color); return color; } + private _getContrastCache(dim: boolean): IColorContrastCache { + if (dim) { + return this._config.colors.halfContrastCache; + } + return this._config.colors.contrastCache; + } + private _drawToCache(codeOrChars: number | string, bg: number, fg: number, ext: number, restrictToCellHeight: boolean = false): IRasterizedGlyph { const chars = typeof codeOrChars === 'number' ? String.fromCharCode(codeOrChars) : codeOrChars; diff --git a/src/browser/services/ThemeService.ts b/src/browser/services/ThemeService.ts index 2411ce85..58c9f354 100644 --- a/src/browser/services/ThemeService.ts +++ b/src/browser/services/ThemeService.ts @@ -81,7 +81,8 @@ export class ThemeService extends Disposable implements IThemeService { public serviceBrand: undefined; private _colors: IColorSet; - private _contrastCache: IColorContrastCache; + private _contrastCache: IColorContrastCache = new ColorContrastCache(); + private _halfContrastCache: IColorContrastCache = new ColorContrastCache(); private _restoreColors!: IRestoreColorSet; public get colors(): ReadonlyColorSet { return this._colors; } @@ -94,7 +95,6 @@ export class ThemeService extends Disposable implements IThemeService { ) { super(); - this._contrastCache = new ColorContrastCache(); this._colors = { foreground: DEFAULT_FOREGROUND, background: DEFAULT_BACKGROUND, @@ -106,7 +106,8 @@ export class ThemeService extends Disposable implements IThemeService { selectionInactiveBackgroundTransparent: DEFAULT_SELECTION, selectionInactiveBackgroundOpaque: color.blend(DEFAULT_BACKGROUND, DEFAULT_SELECTION), ansi: DEFAULT_ANSI_COLORS.slice(), - contrastCache: this._contrastCache + contrastCache: this._contrastCache, + halfContrastCache: this._halfContrastCache }; this._updateRestoreColors(); this._setTheme(this._optionsService.rawOptions.theme); @@ -172,6 +173,7 @@ export class ThemeService extends Disposable implements IThemeService { } // Clear our the cache this._contrastCache.clear(); + this._halfContrastCache.clear(); this._updateRestoreColors(); this._onChangeColors.fire(this.colors); } From 28c4732abcab95637a92f1be27658deaf15e4f73 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sun, 13 Aug 2023 13:04:40 -0700 Subject: [PATCH 2/2] Fix ThemeService.ctor test --- src/browser/services/ThemeService.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/browser/services/ThemeService.test.ts b/src/browser/services/ThemeService.test.ts index f2b2def3..cdfa87df 100644 --- a/src/browser/services/ThemeService.test.ts +++ b/src/browser/services/ThemeService.test.ts @@ -35,7 +35,12 @@ describe('ThemeService', () => { describe('constructor', () => { it('should fill all colors with values', () => { for (const key of Object.keys(themeService.colors)) { - if (key !== 'ansi' && key !== 'contrastCache' && key !== 'selectionForeground') { + if (![ + 'ansi', + 'contrastCache', + 'halfContrastCache', + 'selectionForeground' + ].includes(key)) { // A #rrggbb or rgba(...) assert.ok((themeService.colors as any)[key].css.length >= 7); }