microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
165.18k stars 29.62k forks source link

Theme colour applied to SVG icons #194710

Closed pouyakary closed 1 year ago

pouyakary commented 1 year ago

Type: Bug

The Flutter extension (as well as many other extensions) put extra icons in the debug controls:

Now that the controls can be placed on the command center (#192592) it seems that some of them break:

I don't know if this is something that the author should fix or it is due to the changes in the vscode (Perhaps @DanTup should have a look as well) but I think its a breaking change that effects many other extensions as well.

VS Code version: Code - Insiders 1.83.0-insider (4268e464763087044d0c1b5bdd37ebbe683cadfa, 2023-09-29T20:44:05.310Z) OS version: Darwin arm64 23.0.0 Modes:

System Info |Item|Value| |---|---| |CPUs|Apple M1 (8 x 24)| |GPU Status|2d_canvas: enabled
canvas_oop_rasterization: enabled_on
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: enabled_on
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
video_decode: enabled
video_encode: enabled
vulkan: disabled_off
webgl: enabled
webgl2: enabled
webgpu: enabled| |Load (avg)|3, 3, 3| |Memory (System)|8.00GB (0.09GB free)| |Process Argv|| |Screen Reader|no| |VM|0%|
Extensions (13) Extension|Author (truncated)|Version ---|---|--- Bookmarks|ale|13.4.1 dart-code|Dar|3.75.20231002 flutter|Dar|3.74.0 copilot|Git|1.121.461 copilot-chat|Git|0.8.2023092902 todo-tree|Gru|0.0.226 comment|kar|23.3.0 justifier|kar|0.0.5 moves|kar|1.0.1 theme-karyfoundation-themes|kar|34.3.0 virtuous-vscode|kar|0.0.7 code-spell-checker|str|3.0.1 tldraw-vscode|tld|2.0.11
jrieken commented 1 year ago

@DanTup does your extension use an icon font or SVGs?

DanTup commented 1 year ago

It's an SVG:

https://github.com/Dart-Code/Dart-Code/blob/97cd316e830d9265768c578f0246255a1fab06d9/package.json#L534-L537

https://github.com/Dart-Code/Dart-Code/blob/master/media/commands/hot-reload.svg

DanTup commented 1 year ago

This came up at https://github.com/dart-lang/sdk/issues/53690, although this just looks like an undocked toolbar and not specific to the command center?

jrieken commented 1 year ago

Yes - it unrelated to CC but a consequence (regression) from https://github.com/microsoft/vscode/issues/190679 which applies theming colors to SVG based icons too.

jrieken commented 1 year ago

/cc @pflannery

jrieken commented 1 year ago

So, we did this change on purpose so that theme colors (for icons) can be applied uniformly across the different icon types. Tho, I do acknowledge

DanTup commented 1 year ago

so that theme colors (for icons) can be applied to uniformly across the different icon types

What's the correct way to colour icons with specific colours in this case? (for example the stop button is still red)?

It also seems like the icon has been stretched in some way - do you know what's causing that?

jrieken commented 1 year ago

What's the correct way to colour icons with specific colours in this case? (for example the stop button is still red)?

/cc @aeschli for that

It also seems like the icon has been stretched in some way - do you know what's causing that?

I believe there is some transparent border around the flash which is now visible/coloured

pflannery commented 1 year ago

@jrieken I've been looking at this today and I've discovered that when loading svg's using -webkit-mask the svg colours are interpreted as an alpha value. I.E. where black is invisible and white is fully visible.

It also seems that this change has made the light and dark options under contributes\commands in the package.json obsolete? as now there isn't really anything we can do with those options.

I've found a way to make svg icons to appear enabled or not. This involves using the svg mask element to create a semi transparent background.

image

image

image

enabled svg sample

disabled svg sample

I'm not able to resolve the red error icon that versionlens previously used so I've had to change the icon to look like a warning.

image

imo I think it would be better that an extension author has ownership over their icons. Users can create requests with those authors regarding theme compatibility. Perhaps vscode could even allow extension icons to be overridden by theme authors or users similar to how Product Icons are?

DanTup commented 1 year ago

imo I think it would be better that an extension author has ownership over their icons

I agree. Some of the built-in icons already have their own colours, so it seems clear that making them all the same isn't a good option.

If this must be supported, perhaps it could be done with a flag that says whether an icon should inherit a theme colour or not?

DanTup commented 1 year ago

@jrieken is there any workaround we could apply until this is resolved? We have multiple icons that show up in different contexts and they all look pretty bad now:

image

nvuillam commented 1 year ago

Similar issue wwith VsCode SFDX Hardis.

Before: image

After: image

jrieken commented 1 year ago

I agree. Some of the built-in icons already have their own colours, so it seems clear that making them all the same isn't a good option.

This wasn't about making them all the same but about making them configurable. The idea being that that "all yellow theme" can tweak the dart-debug-flash to a better fit. That's what motivated and justified this change. Tho, I do see the technical limitations of the mask-approach and therefore we should revisit this.

The gist is that

jrieken commented 1 year ago

Also note that we have the codicon-font from which extensions can use icons, e.g the dart flash and magnifier could be zap and search. Check them out here https://microsoft.github.io/vscode-codicons/dist/codicon.html

DanTup commented 1 year ago

we invite extension authors to adopt our CSS variables for theming SVG

Does this mean in SVG icons we will be able to choose between our own hard-coded colours, or using variables that would be inherited from a theme? If so, that sounds great.

Some of the icons used in Dart were picked to match the icons that appear in DevTools (an external tool we embed) which come from the Material icons. Are custom icon fonts supported? And if we do use an icon font, will we still be able to override the colour where it makes sense, or will this be restricted to SVGs?

aeschli commented 1 year ago

Verified in 1.83.1 candidate