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.

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

Reset styling for embedded languages in grammars? #33120

Closed
mjbvz opened this issue Aug 24, 2017 · 6 comments · Fixed by #33866
Closed

Reset styling for embedded languages in grammars? #33120

mjbvz opened this issue Aug 24, 2017 · 6 comments · Fixed by #33866
Assignees
Labels
grammar Syntax highlighting grammar themes Color theme issues under-discussion Issue is under discussion for relevance, priority, approach

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Aug 24, 2017

Problem

For grammars with embedded languages, the outside scope's colorization can leak into the embedded language:

screen shot 2017-08-24 at 4 26 07 pm 1

The above example shows embedding the html grammar in a js template string. Notice that text is orange. No specific html grammar scope applies to it so it picks up the foreground color of string.template.js

Instead, in most cases I feel it would make more sense that text be colored the same at it would be in a normal html file, which in this case would be the default foreground color of the theme

screen shot 2017-08-24 at 4 38 32 pm

Possible Approach

I've already fixed this issue for ${expressions} in normal js template strings. Manually handling each language and embedded language combination will not scale however.

One idea is to make themes treat the meta.embedded scope as a styling reset point. Themes would just use the same style for meta.embedded as for the default:

	"tokenColors": [
		{
			"settings": {
				"foreground": "#D4D4D4",
				"background": "#1E1E1E"
			}
		},
		{
			"scope": "meta.embedded",
			"settings": {
				"foreground": "#D4D4D4",
				"background": "#1E1E1E"
			}
		},
      ...

This works in my initial testing but may have unintended side effects. It also requires updating every theme

@mjbvz mjbvz added grammar Syntax highlighting grammar themes Color theme issues under-discussion Issue is under discussion for relevance, priority, approach labels Aug 24, 2017
@mjbvz mjbvz self-assigned this Aug 24, 2017
@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 24, 2017

@Tyriar or @aeschli, any thoughts on this?

@mjbvz mjbvz changed the title Reset styling for grammars with embedded languages? Reset styling for embedded languages in grammars? Aug 24, 2017
@Tyriar
Copy link
Member

Tyriar commented Aug 24, 2017

You essentially want to clear the scopes for the embedded language block right?

@alexandrudima is quite familiar with the textmate scopes.

@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 24, 2017

Yes, I looked through some textmate grammars but didn't see any examples of this

@aeschli
Copy link
Contributor

aeschli commented Aug 25, 2017

I'd argue that having red as default isn't wrong, it's still a string, after all. But themes are already free to define such rules such as setting the default background color for 'source' or 'meta.embedded'

I'm against any tricks and customizations of our textmate coloring engine, we should stay compatible. But if you want to add something to our default rules, no problem.

@alexdima
Copy link
Member

👍

I'm against any tricks and customizations of our textmate coloring engine, we should stay compatible. But if you want to add something to our default rules, no problem.

@mjbvz mjbvz added this to the September 2017 milestone Aug 28, 2017
@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 28, 2017

Ok, sounds like proposed theme changes is the way to go. I'll look into trying this next iteration

mjbvz added a commit to mjbvz/vscode that referenced this issue Sep 6, 2017
Fixes microsoft#33120

Adds rules to vs dark+light to reset style for meta embedded scopes. This is a proposed approach to support colorization of nested languages better
mjbvz added a commit that referenced this issue Sep 6, 2017
Fixes #33120

Adds rules to vs dark+light to reset style for meta embedded scopes. This is a proposed approach to support colorization of nested languages better
mjbvz added a commit to mjbvz/TypeScript-TmLanguage that referenced this issue Sep 6, 2017
**Problem**
VS Code themes currently hardcode a solution for reseting expression colorization inside of template string substitution expressions. This is not scalable since it js/ts specific

With microsoft/vscode#33120 I've proposed that VS Code themes reset styling inside of meta.embedded scopes to support this across multiple languages

**Fix**
Marks template substitution expressions with the additional `meta.embedded.block.ts` scope
mjbvz added a commit to mjbvz/TypeScript-TmLanguage that referenced this issue Sep 6, 2017
**Problem**
VS Code themes currently hardcode a solution for reseting expression colorization inside of template string substitution expressions. This is not scalable since it js/ts specific

With microsoft/vscode#33120 I've proposed that VS Code themes reset styling inside of meta.embedded scopes to support this across multiple languages

**Fix**
Marks template substitution expressions with the additional `meta.embedded.line.ts` scope
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
grammar Syntax highlighting grammar themes Color theme issues under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants