From 73a77cda3d4352e88ee4684edbe530938bcbac30 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sun, 13 Aug 2023 13:47:50 -0700 Subject: [PATCH 1/2] DOM: Render selection color instead of cell bg Summary: - Selection cell reuse has been added to the DOM renderer - Selection bg override is now correctly set correctly - Selection elements 'pretend' to be a decoration in order to render bg Fixes #4097 --- .../renderer/dom/DomRendererRowFactory.ts | 44 ++++++++++++------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/browser/renderer/dom/DomRendererRowFactory.ts b/src/browser/renderer/dom/DomRendererRowFactory.ts index 8ecaad42..998a11eb 100644 --- a/src/browser/renderer/dom/DomRendererRowFactory.ts +++ b/src/browser/renderer/dom/DomRendererRowFactory.ts @@ -89,6 +89,7 @@ export class DomRendererRowFactory { let oldExt = 0; let oldLinkHover: number | boolean = false; let oldSpacing = 0; + let oldIsInSelection: boolean = false; let spacing = 0; const classes: string[] = []; @@ -154,16 +155,24 @@ export class DomRendererRowFactory { /** * chars can only be merged on existing span if: * - existing span only contains mergeable chars (cellAmount != 0) - * - fg/bg/ul did not change - * - char not part of a selection + * - bg did not change (or both are in selection) + * - fg did not change (or both are in selection and selection fg is set) + * - ext did not change * - underline from hover state did not change * - cell content renders to same letter-spacing * - cell is not cursor */ if ( cellAmount - && cell.bg === oldBg && cell.fg === oldFg && cell.extended.ext === oldExt - && !isInSelection + && ( + (isInSelection && oldIsInSelection) + || (!isInSelection && cell.bg === oldBg) + ) + && ( + (isInSelection && oldIsInSelection && colors.selectionForeground) + || (!(isInSelection && oldIsInSelection && colors.selectionForeground) && cell.fg === oldFg) + ) + && cell.extended.ext === oldExt && isLinkHover === oldLinkHover && spacing === oldSpacing && !isCursorCell @@ -194,6 +203,7 @@ export class DomRendererRowFactory { oldExt = cell.extended.ext; oldLinkHover = isLinkHover; oldSpacing = spacing; + oldIsInSelection = isInSelection; if (isJoined) { // The DOM renderer colors the background of the cursor but for ligatures all cells are @@ -328,22 +338,26 @@ export class DomRendererRowFactory { isTop = d.options.layer === 'top'; }); - // Apply selection foreground if applicable - if (!isTop) { - if (colors.selectionForeground && isInSelection) { + // Apply selection + if (!isTop && isInSelection) { + // If in the selection, force the element to be above the selection to improve contrast and + // support opaque selections. The applies background is not actually needed here as + // selection is drawn in a seperate container, the main purpose of this to ensuring minimum + // contrast ratio + bgOverride = this._coreBrowserService.isFocused ? colors.selectionBackgroundOpaque : colors.selectionInactiveBackgroundOpaque; + bg = bgOverride.rgba >> 8 & 0xFFFFFF; + bgColorMode = Attributes.CM_RGB; + // Since an opaque selection is being rendered, the selection pretends to be a decoration to + // ensure text is drawn above the selection. + isTop = true; + // Apply selection foreground if applicable + if (colors.selectionForeground) { fgColorMode = Attributes.CM_RGB; fg = colors.selectionForeground.rgba >> 8 & 0xFFFFFF; fgOverride = colors.selectionForeground; } } - // If in the selection, force the element to be above the selection to improve contrast and - // support opaque selections - if (isInSelection) { - bgOverride = this._coreBrowserService.isFocused ? colors.selectionBackgroundOpaque : colors.selectionInactiveBackgroundOpaque; - isTop = true; - } - // If it's a top decoration, render above the selection if (isTop) { classes.push('xterm-decoration-top'); @@ -417,7 +431,7 @@ export class DomRendererRowFactory { } // exclude conditions for cell merging - never merge these - if (!isCursorCell && !isInSelection && !isJoined && !isDecorated) { + if (!isCursorCell && !isJoined && !isDecorated && isInSelection === oldIsInSelection) { cellAmount++; } else { charElement.textContent = text; From 39ae0f45fdee2195382e36db0f32cbeb7c747679 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 14 Aug 2023 04:43:55 -0700 Subject: [PATCH 2/2] Fix selectionForeground dom tests --- src/browser/TestUtils.test.ts | 4 +++- src/browser/renderer/dom/DomRendererRowFactory.test.ts | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/browser/TestUtils.test.ts b/src/browser/TestUtils.test.ts index 2015d046..5981a6b5 100644 --- a/src/browser/TestUtils.test.ts +++ b/src/browser/TestUtils.test.ts @@ -529,6 +529,8 @@ export class MockThemeService implements IThemeService{ css.toColor('#ad7fa8'), css.toColor('#34e2e2'), css.toColor('#eeeeec') - ] + ], + selectionBackgroundOpaque: css.toColor('#ff0000'), + selectionInactiveBackgroundOpaque: css.toColor('#00ff00') } as any; } diff --git a/src/browser/renderer/dom/DomRendererRowFactory.test.ts b/src/browser/renderer/dom/DomRendererRowFactory.test.ts index 609a5b4a..36923dbb 100644 --- a/src/browser/renderer/dom/DomRendererRowFactory.test.ts +++ b/src/browser/renderer/dom/DomRendererRowFactory.test.ts @@ -309,7 +309,7 @@ describe('DomRendererRowFactory', () => { rowFactory.handleSelectionChanged([1, 0], [2, 0], false); const spans = rowFactory.createRow(lineData, 0, false, undefined, undefined, 0, false, 5, EMPTY_WIDTH, -1, -1); assert.equal(extractHtml(spans), - 'ab' + 'ab' ); }); it('should force whitespace cells to be rendered above the background', () => { @@ -317,7 +317,7 @@ describe('DomRendererRowFactory', () => { rowFactory.handleSelectionChanged([0, 0], [2, 0], false); const spans = rowFactory.createRow(lineData, 0, false, undefined, undefined, 0, false, 5, EMPTY_WIDTH, -1, -1); assert.equal(extractHtml(spans), - ' a' + ' a' ); }); });