Expose data-vscode-theme-id attribute in webviews, fix #149661 by pingren · Pull Request #154635 · microsoft/vscode

pingren · 2022-07-09T15:32:19Z

This PR fixes #149661

Context

data-vscode-theme-name is exposed on webviews for theme extensions to write theme specific CSS.
theme label will change when change display language change, which unwanted in Webview CSS: data-vscode-theme-name attribute changes when the language changes #149661.
themes have a label not a name. The term should be consistent with theme extension.

Changes

deprecate data-vscode-theme-name, rename it to data-vscode-theme-label
add data-vscode-theme-id

Original PR

Adds a data attribute to webviews with the themeId. Extension could write theme specific CSS for webviews. Unlike themeName, themeId will not change if changing the display language. themeName is preserved to prevent breaking changes.

Before (the display language changed to zh-cn)

After

ghost · 2022-07-09T15:32:32Z

All CLA requirements met.

mjbvz

Instead of adding a new class, how about always using the default english name for data-vscode-theme-name. That way existing users don't have to adopt anything

pingren · 2022-07-10T03:52:10Z

@mjbvz I agree it's better not let existing users adopt anything. However, after some diggings around the code, I realized that

theme label is defined in

vscode/extensions/theme-red/package.json

Line 15 in 042e505

"label": "%themeLabel%",

and could be overwritten by i18n extension in

https://github.com/microsoft/vscode-loc/blob/fb987ff21ae002c918fc0e7039642ae345f279b6/i18n/vscode-language-pack-zh-hans/translations/extensions/theme-red.i18n.json#L14

When ExtensionsRegistry loading theme extensions, themeLabel will be localized right away, and ThemeService is not aware of its original English string.

Keeping and passing a copy of English themeLabel is overkill. Therefore, adding a data-vscode-theme-id should be the way to address the issue with minimal efforts.

mjbvz · 2022-07-12T01:00:06Z

Introducing this new attribute both leaves the current broken behavior in place and introduces a new thing for users/extension authors to adopt. Not a fan

@aeschli Any ideas on reading the default English value of a theme name?

aeschli · 2022-07-12T09:56:48Z

Yes, I don't think we don't know the original English label in the ThemeService as the translation happens in a layer below it.
It would be good if we can deprecate data-vscode-theme-name. Themes actually have a label not a name.
Let's search how many extensions use it. We can then also see how it is used: either for label (for presentation) vs the id (to set it or for theme specific colors)

pingren · 2022-07-12T10:08:06Z

I found themes using data-vscode-theme-name from https://github.com/search?q=data-vscode-theme-name&type=code.
Most of them are used to for theme specific colors or styles. I don't know if there are better ways to get exact usage stats.

aeschli · 2022-07-12T15:25:01Z

Searching the top 6000 extensions on the Marketplace so far only rangav.vscode-thunder-client uses data-vscode-theme-name

aeschli · 2022-07-19T09:55:59Z

After searching 37'800 extensions, I found data-vscode-theme-name at the following spaces

blueplanet-ciena.ciena-blueplanet/resource-type-graph/main.91fb18098d9d6cca2a82.js
cweijan.vscode-database-client2/out/webview/js/app.js
cweijan.vscode-database-client2/out/webview/js/query.js
cweijan.vscode-dm-client/out/webview/js/app.js
cweijan.vscode-es-client2/out/webview/js/app.js
cweijan.vscode-es-client2/out/webview/js/query.js
cweijan.vscode-mysql-client2/out/webview/js/app.js
cweijan.vscode-mysql-client2/out/webview/js/query.js
cweijan.vscode-postgresql-client2/out/webview/js/app.js
cweijan.vscode-postgresql-client2/out/webview/js/query.js
cweijan.vscode-redis-client/out/webview/js/app.js
cweijan.vscode-redis-client/out/webview/js/query.js
cweijan.vscode-ssh/out/webview/js/app.js
cweijan.vscode-ssh/out/webview/js/query.js
eliostruyf.vscode-theming-devtools/dist/webview.js
LigaAI.liga/out/web-dist/assets/i18n.js
Lightrun.lightrunplugin-saas/webpack/build/configViewer.js
Lightrun.lightrunplugin-saas/webpack/build/logs.js
rangav.vscode-thunder-client/dist/extension.js
rebornix.vscode-code-renderer/out/client/index.js
YuTengjing.scripting-listener/dist/web/webview.js

My suggestion would be to deprecate data-vscode-theme-name (remove it from documentation, but leave it as is for the next half year), add data-vscode-theme-id and data-vscode-theme-label and inform the extensions above.

pingren · 2022-07-21T11:01:49Z

Thank you for your research!
I will change the PR to deprecate data-vscode-theme-name and add data-vscode-theme-id and data-vscode-theme-label.
I could submit another PR to https://github.com/microsoft/vscode-docs about these changes if you want.

And I am just curious. How did you search for source code of extensions on the Marketplace?

aeschli · 2022-07-21T11:42:04Z

@pingren An update to the docs would be fantastic.
We have an internal tool that downloads a set of extensions from the marketplace so we can search them

Related PR: microsoft/vscode#154635

pingren · 2022-09-05T04:58:20Z

@aeschli PR Updated. Please take a look when you have a moment.

aeschli · 2022-09-05T14:24:24Z

I'd rather not introduce the id to platform/common/IColorTheme.

Instead you can do the following:
In the WebviewThemeDataProvider use IWorkbenchThemeService instead of IThemeService

Then the theme you get with this._themeService.getColorTheme() is of type IWorkbenchColorTheme and you can access the property id

…deprecate data-vscode-theme-name. Fixes microsoft#149661

pingren · 2022-09-13T02:59:17Z

Updated: use IWorkbenchThemeService instead of IThemeService

aeschli · 2022-09-13T09:41:46Z

Thanks @pingren. Can you also remove the change in src/vs/workbench/contrib/terminal/test/common/terminalColorRegistry.test.ts?

pingren · 2022-09-15T04:40:50Z

Thank you! Sorry for that.

Updated: Remove the change in src/vs/workbench/contrib/terminal/test/common/terminalColorRegistry.test.ts

mjbvz · 2022-09-22T23:55:36Z

src/vs/workbench/contrib/webview/browser/themeing.ts


 			const activeTheme = ApiThemeClassName.fromTheme(theme);
-			this._cachedWebViewThemeData = { styles, activeTheme, themeLabel: theme.label, };
+			this._cachedWebViewThemeData = { styles, activeTheme, themeLabel: theme.label, themeId: theme.settingsId };


@aeschli Can you please confirm that settingsId is a good value to use here? I switched to use this since theme.id is not friendly to consume. Here's some example values for Dark+:

theme.id — vs-dark vscode-theme-defaults-themes-dark_plus-json

theme.settingsId — Default Dark+

Yes, sorry for not realizing this. settingsId is what we want (it's unique and it's what can be used in the settings)

vscodenpa assigned mjbvz Jul 9, 2022

mjbvz requested changes Jul 9, 2022

View reviewed changes

pingren force-pushed the fix/expose-theme-id-webview branch from c6ffba7 to b7560ee Compare July 24, 2022 11:08

pingren added a commit to pingren/vscode-docs that referenced this pull request Jul 24, 2022

Update docs for theming webview content

d54e20c

Related PR: microsoft/vscode#154635

pingren mentioned this pull request Jul 24, 2022

Update docs for theming webview content microsoft/vscode-docs#5491

Closed

pingren force-pushed the fix/expose-theme-id-webview branch from b7560ee to 7ab7b48 Compare September 5, 2022 04:55

pingren requested a review from mjbvz September 5, 2022 04:56

Expose data-vscode-theme-id and data-vscode-theme-label in webviews, …

29fbfb5

…deprecate data-vscode-theme-name. Fixes microsoft#149661

pingren force-pushed the fix/expose-theme-id-webview branch from 7ab7b48 to 29fbfb5 Compare September 13, 2022 02:57

fix: remove useless id in test

70cc324

aeschli previously approved these changes Sep 15, 2022

View reviewed changes

mjbvz added this to the September 2022 milestone Sep 22, 2022

mjbvz added 2 commits September 22, 2022 16:26

Merge branch 'main' into pr/pingren/154635-1

cbec21e

Use settingId and remove theme-label

0f461f9

mjbvz dismissed aeschli’s stale review via 0f461f9 September 22, 2022 23:51

mjbvz reviewed Sep 22, 2022

View reviewed changes

mjbvz enabled auto-merge (squash) September 23, 2022 17:59

mjbvz approved these changes Sep 23, 2022

View reviewed changes

mjbvz merged commit e42b88d into microsoft:main Sep 23, 2022

github-actions bot locked and limited conversation to collaborators Nov 7, 2022

Conversation

pingren commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes

Original PR

Before (the display language changed to zh-cn)

After

Uh oh!

ghost commented Jul 9, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjbvz left a comment

Choose a reason for hiding this comment

Uh oh!

pingren commented Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjbvz commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeschli commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pingren commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeschli commented Jul 12, 2022

Uh oh!

aeschli commented Jul 19, 2022

Uh oh!

pingren commented Jul 21, 2022

Uh oh!

aeschli commented Jul 21, 2022

Uh oh!

pingren commented Sep 5, 2022

Uh oh!

aeschli commented Sep 5, 2022

Uh oh!

pingren commented Sep 13, 2022

Uh oh!

aeschli commented Sep 13, 2022

Uh oh!

pingren commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjbvz Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeschli Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pingren commented Jul 9, 2022 •

edited

Loading

ghost commented Jul 9, 2022 •

edited by ghost

Loading

pingren commented Jul 10, 2022 •

edited

Loading

mjbvz commented Jul 12, 2022 •

edited

Loading

aeschli commented Jul 12, 2022 •

edited

Loading

pingren commented Jul 12, 2022 •

edited

Loading

pingren commented Sep 15, 2022 •

edited

Loading

mjbvz Sep 22, 2022 •

edited

Loading