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 8ecaad4287..998a11ebcf 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 2015d04637..5981a6b51f 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 609a5b4a57..36923dbbce 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'
);
});
});