Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DOM: Render selection color instead of cell bg #4673

Merged
merged 2 commits into from
Aug 14, 2023
Merged

DOM: Render selection color instead of cell bg #4673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73a77cd
DOM: Render selection color instead of cell bg
Tyriar Aug 13, 2023
39ae0f4
Fix selectionForeground dom tests
Tyriar Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/browser/TestUtils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
4 changes: 2 additions & 2 deletions src/browser/renderer/dom/DomRendererRowFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>'
);
});
});
Expand Down
44 changes: 29 additions & 15 deletions src/browser/renderer/dom/DomRendererRowFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];

Expand Down Expand Up @@ -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)
)
Comment on lines +167 to +174
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this looks quite complicated - I guess you have tested it, so I assume it good as it is (though I wonder if it could be simplified). Which selection state tried you to cover here, which was not by the old conditions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we would just never merge cells if it was a selection which is easy but that means select all would all be the old slow rendering.

Outside of pulling things into named variables or expanding the comment I'm not sure how to simplify this, these cases are described in the comment above:

- bg did not change (or both are in selection)
- fg did not change (or both are in selection and selection fg is set)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... but that means select all would all be the old slow rendering.

Yes very true. Btw the same applies to isDecorated. Yeah, I kinda skipped dealing with inner works of selections and decorations, as it is more on the rare side of things. But indeed, you can actually feel the difference in responsiveness when selecting the full viewport and try to scroll then - so its good you address that.

&& cell.extended.ext === oldExt
&& isLinkHover === oldLinkHover
&& spacing === oldSpacing
&& !isCursorCell
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, if isInSelection === oldIsInSelection makes much sense here - conditions here are late evals for the first span, and whether it qualifies for merging with follow-up chars.

I lazily hacked !isInSelection in here, which is technically too strict. Normally selections can also merge, if the edge conditions are all met, which I was not sure about, thus the shortcut.

Now your isInSelection === oldIsInSelection matches the same value with itself, because of line:206?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right this does nothing 🙈

We may just be able to remove that condition completely then as this seemed to work well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4676

cellAmount++;
} else {
charElement.textContent = text;
Expand Down