Debt - use CSS variables over `registerThemingParticipant` #165169

jrieken · 2022-11-01T16:35:06Z

When running VS Code (or the Monaco editor) all theme colors are exposed as CSS variables. That means components don’t need to use JavaScript to read and set colors but can simply rely on CSS. When a theme changes, the variable values automatically update. Also, with CSS variables you have less code and everything in one place.

The (long) list below is all users of registerThemingParticipant. Those that only set/update colors are candidates for adopting CSS variables The names of the CSS variables can be "derived" with the following logic

prefix --vscode-
take the theme color id and replace dots with slashes

For instance the color id editorSuggestWidget.background is exposed as CSS variables --vscode-editorSuggestWidget-background. Find a sample adoption in this commit: 3a787a8.

The text was updated successfully, but these errors were encountered:

Fixes microsoft#165169

Fixes #165169

Tyriar · 2023-02-27T20:13:46Z

I looked over mine again and they are all there still intentionally:

vscode/src/vs/workbench/contrib/terminal/browser/widgets/terminalHoverWidget.ts

Lines 135 to 143 in bceaaf8

    
           registerThemingParticipant((theme, collector) => { 
        
           	let editorHoverHighlightColor = theme.getColor(editorHoverHighlight); 
        
           	if (editorHoverHighlightColor) { 
        
           		if (editorHoverHighlightColor.isOpaque()) { 
        
           			editorHoverHighlightColor = editorHoverHighlightColor.transparent(0.5); 
        
           		} 
        
           		collector.addRule(`.integrated-terminal .hoverHighlight { background-color: ${editorHoverHighlightColor}; }`); 
        
           	} 
        
           });

I don't think we can get the same effect of .transparent(0.5) in CSS using the variable.

https://github.com/microsoft/vscode/blob/bceaaf84a27c3a95a0cdfc79287e3215b56b951c/src/vs/workbench/services/hover/browser/hoverService.ts#L179-L186

Same as above.

vscode/src/vs/workbench/contrib/terminal/browser/xterm/decorationAddon.ts

Lines 479 to 486 in bceaaf8

    
           let successColor: string | Color | undefined; 
        
           let errorColor: string | Color | undefined; 
        
           let defaultColor: string | Color | undefined; 
        
           registerThemingParticipant((theme: IColorTheme) => { 
        
           	successColor = theme.getColor(TERMINAL_COMMAND_DECORATION_SUCCESS_BACKGROUND_COLOR); 
        
           	errorColor = theme.getColor(TERMINAL_COMMAND_DECORATION_ERROR_BACKGROUND_COLOR); 
        
           	defaultColor = theme.getColor(TERMINAL_COMMAND_DECORATION_DEFAULT_BACKGROUND_COLOR); 
        
           });

This is needed as the color is handed off to xterm.js.

jrieken · 2023-02-27T20:16:59Z

@Tyriar please check those occurrences anyways so that we can remove them from the todo list

andreamah · 2023-02-27T20:19:34Z

For searchEditor.ts, I couldn't find references to registerThemingParticipant.

For searchView.ts, we use the RGB values from the foreground var and we use a custom alpha. Since this only applies to the text, I don't think that there's a CSS-only equivalent. We could create a new var for this or just leave this as is; I'm not sure what's best in this case.

vscode/src/vs/workbench/contrib/search/browser/searchView.ts

Lines 2037 to 2045 in b58d49d

    
           registerThemingParticipant((theme: IColorTheme, collector: ICssStyleCollector) => { 
        
           	if (theme.type === 'dark') { 
        
           		const foregroundColor = theme.getColor(foreground); 
        
           		if (foregroundColor) { 
        
           			const fgWithOpacity = new Color(new RGBA(foregroundColor.rgba.r, foregroundColor.rgba.g, foregroundColor.rgba.b, 0.65)); 
        
           			collector.addRule(`.search-view .message { color: ${fgWithOpacity}; }`); 
        
           		} 
        
           	} 
        
           });

Tyriar · 2023-02-27T20:24:27Z

@andreamah you're setting color there so you can probably use a plain opacity: 0.65 style, unless the element also has a background color as that would also apply there.

andreamah · 2023-02-27T20:41:04Z

@andreamah you're setting color there so you can probably use a plain opacity: 0.65 style, unless the element also has a background color as that would also apply there.

The element also has links, which have a normal alpha, so then the link opacity is affected.

Tyriar · 2023-02-27T20:44:08Z

@andreamah you could have another rule with higher specificity that re-applies the link color?

aeschli · 2023-02-28T09:16:04Z

@Tyriar For

vscode/src/vs/workbench/contrib/terminal/browser/widgets/terminalHoverWidget.ts

Lines 135 to 143 in bceaaf8

    
           registerThemingParticipant((theme, collector) => { 
        
           	let editorHoverHighlightColor = theme.getColor(editorHoverHighlight); 
        
           	if (editorHoverHighlightColor) { 
        
           		if (editorHoverHighlightColor.isOpaque()) { 
        
           			editorHoverHighlightColor = editorHoverHighlightColor.transparent(0.5); 
        
           		} 
        
           		collector.addRule(`.integrated-terminal .hoverHighlight { background-color: ${editorHoverHighlightColor}; }`); 
        
           	} 
        
           });

you could define a new (terminal hover specific) color. The default value of the color can express that it is a function of editorHoverHighlightColor

vscode/src/vs/workbench/contrib/terminal/browser/xterm/decorationAddon.ts

Lines 479 to 486 in bceaaf8

    
           let successColor: string | Color | undefined; 
        
           let errorColor: string | Color | undefined; 
        
           let defaultColor: string | Color | undefined; 
        
           registerThemingParticipant((theme: IColorTheme) => { 
        
           	successColor = theme.getColor(TERMINAL_COMMAND_DECORATION_SUCCESS_BACKGROUND_COLOR); 
        
           	errorColor = theme.getColor(TERMINAL_COMMAND_DECORATION_ERROR_BACKGROUND_COLOR); 
        
           	defaultColor = theme.getColor(TERMINAL_COMMAND_DECORATION_DEFAULT_BACKGROUND_COLOR); 
        
           });

To access a color value use IThemeService.getColorTheme().getColor(...)

bpasero · 2023-02-28T10:03:42Z

As discussed in standup, tabs have quite an involved CSS ruling that involves not only colors but also other properties. I would like to keep it this way via registerThemingParticipant and not invent various class names to get a similar effect to make maintenance easier there.

Part of #165169

jrieken · 2023-05-09T06:32:35Z

Closing this as I believe we are done here

jrieken added debt Code quality issues perf-bloat labels Nov 1, 2022

jrieken assigned alexdima, hediet, jrieken, Tyriar, lramos15, meganrogge, roblourens, rebornix and bpasero and unassigned hediet and alexdima Nov 1, 2022

jrieken added this to the November 2022 milestone Nov 1, 2022

hediet removed their assignment Nov 1, 2022

lramos15 mentioned this issue Nov 1, 2022

Switch to using CSS #165185

Merged

mjbvz added a commit to mjbvz/vscode that referenced this issue Nov 1, 2022

Use css vars for lightbulb

1e56fd9

Fixes microsoft#165169

mjbvz mentioned this issue Nov 1, 2022

Use css vars for lightbulb #165186

Merged

mjbvz closed this as completed in #165186 Nov 1, 2022

mjbvz added a commit that referenced this issue Nov 1, 2022

Use css vars for lightbulb (#165186)

25c6529

Fixes #165169

vscodenpa added the unreleased Patch has not yet been released in VS Code Insiders label Nov 1, 2022

lramos15 reopened this Nov 1, 2022

vscodenpa removed the unreleased Patch has not yet been released in VS Code Insiders label Nov 1, 2022

rebornix mentioned this issue Nov 2, 2022

Reduce theme participant for notebook #165214

Merged

lramos15 removed their assignment Nov 2, 2022

rebornix removed their assignment Nov 3, 2022

jrieken mentioned this issue Nov 3, 2022

joh/external marlin #165336

Merged

rzhao271 mentioned this issue Nov 3, 2022

Remove redundant CSS rules #165401

Merged

jrieken assigned Tyriar, andreamah and lramos15 Feb 27, 2023

lramos15 assigned bhavyaus and unassigned lramos15 Feb 27, 2023

Tyriar removed their assignment Feb 27, 2023

bpasero removed their assignment Feb 28, 2023

Tyriar added a commit that referenced this issue Feb 28, 2023

Use decoration colors directly

f164298

Part of #165169

Tyriar added a commit that referenced this issue Feb 28, 2023

Register a new color instead of theme participant

4b98001

Part of #165169

Tyriar mentioned this issue Feb 28, 2023

Remove 2 terminal theme participants #175647

Merged

This was referenced Feb 28, 2023

Remove welcomeOverlay and its references. #175686

Merged

Remove theme participants in walkthroughs #175711

Merged

bhavyaus removed their assignment Mar 1, 2023

andreamah mentioned this issue Mar 1, 2023

create colour token for search foreground #175782

Merged

andreamah removed their assignment Mar 1, 2023

jrieken modified the milestones: March 2023, April 2023 Mar 20, 2023

jrieken modified the milestones: April 2023, May 2023 Apr 24, 2023

jrieken closed this as completed May 9, 2023

github-actions bot locked and limited conversation to collaborators Jun 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debt - use CSS variables over `registerThemingParticipant` #165169

Debt - use CSS variables over `registerThemingParticipant` #165169

jrieken commented Nov 1, 2022 •

edited by andreamah

Loading

Tyriar commented Feb 27, 2023

jrieken commented Feb 27, 2023

andreamah commented Feb 27, 2023

Tyriar commented Feb 27, 2023

andreamah commented Feb 27, 2023

Tyriar commented Feb 27, 2023

aeschli commented Feb 28, 2023

bpasero commented Feb 28, 2023

jrieken commented May 9, 2023

Debt - use CSS variables over registerThemingParticipant #165169

Debt - use CSS variables over registerThemingParticipant #165169

Comments

jrieken commented Nov 1, 2022 • edited by andreamah Loading

Tyriar commented Feb 27, 2023

jrieken commented Feb 27, 2023

andreamah commented Feb 27, 2023

Tyriar commented Feb 27, 2023

andreamah commented Feb 27, 2023

Tyriar commented Feb 27, 2023

aeschli commented Feb 28, 2023

bpasero commented Feb 28, 2023

jrieken commented May 9, 2023

Debt - use CSS variables over `registerThemingParticipant` #165169

Debt - use CSS variables over `registerThemingParticipant` #165169

jrieken commented Nov 1, 2022 •

edited by andreamah

Loading