Merge pull request #4673 from Tyriar/4097

DOM: Render selection color instead of cell bg

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;
 }

src/browser/renderer/dom/DomRendererRowFactory.test.ts

@@ -309,15 +309,15 @@ 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),
-          '<span>a</span><span class="xterm-decoration-top">b</span>'
+          '<span>a</span><span style="background-color:#ff0000;" class="xterm-decoration-top">b</span>'
         );
       });
       it('should force whitespace cells to be rendered above the background', () => {
         lineData.setCell(1, CellData.fromCharData([DEFAULT_ATTR, 'a', 1, 'a'.charCodeAt(0)]));
         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),
-          '<span class="xterm-decoration-top"> </span><span class="xterm-decoration-top">a</span>'
+          '<span style="background-color:#ff0000;" class="xterm-decoration-top"> a</span>'
         );
       });
     });

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;