Page MenuHomePhabricator

Support defining CSS variables in TemplateStyles
Open, MediumPublicFeature

Description

Attempting to use CSS variables as the following (which are supported by all major browsers)

:root {
  --color-main: #06c;
}

#foo h1 {
  color: var( --color-main );
}

fails with some very unfriendly error messages:

  • Unrecognized or unsupported property at line 2 character 3.
  • Invalid or unsupported value for property color at line 6 character 10.

Looking at the source code I discovered this comment which says:

This intentionally doesn't support cascading variables since that seems impossible to securely sanitize.

The comment unfortunately does not elaborate further ... I don't know what exactly about CSS variables was deemed impossible to sanitize. I might be ignorant but I don't see the problem.

NOTE: Once T360562 is done, it will be possible to use (but not set) CSS variables in TemplateStyles.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

While I can see a potential security problem by allowing defining variables, using the var keyword doesn't have any potential impact that I'm aware of, and can be very useful for wikis that may define variables in each skin CSS file, or even core skins can define variables for themes, dark mode, etc.

Validation should happen on the fallback value. It may be useful to enforce providing a fallback value when using var(). It should be noted that the fallback value can be another var()

As there are plans for making a darkskin this will be important for templates. It will not be feasible for the WMF staff to create a skin supporting each and every template.

Here is just one of probably many practical use cases:
https://pl.wikipedia.org/wiki/Szablon:Tree_chart/style.css
Both of this colors should be taken from variables:

html.enable-dark-skin .tree-chart .tbox {
	background: #303030;
	color: rgb(216, 216, 216);
}

cc: @sgrabarczuk

Another use case is making code more readable and maintainable:
https://pl.wikipedia.org/w/index.php?title=Szablon:Nux/test_teambracket/8tpl-lines.css&oldid=70248259

I can easily read and adjust this one:

.pair::after {
	--width: calc(var(--line-width) + .5 * var(--team-col-gap) + var(--line-thick));
	left: calc(100% + var(--width));
	width: var(--width);
}

This would be a nightmare long term:

.pair::after {
	left: calc(100% + calc(5px + .5 * calc(5px * 2) + 2px));
	width: calc(5px + .5 * calc(5px * 2) + 2px);
}

I will probably maintain Less off-wiki in that case. But as you can imagine that will be less then ideal 😉

Well, examples like these, @Nux, are more of an argument against allowing this (although I am definitely still in favour). Nested CSS calculations should be avoided, especially when they are unnecessary, such as here. 5px is not used anywhere in the template without * 2, why even repeated calculations of it in the first place? Since 5px + .5 * calc(5px * 2) + 2px is just 5px * 2 + 2px written in a weird way.

@stjn you can see full CSS here, that will probably make more sense:
https://github.com/Eccenux/wiki-team-brackets/blob/master/html-redesign/lines.css

Not that it matters here. The point was that things depend on each other and numbers can be made readable by adding names and meaning.

I don't think there are any potential security issues by using variables and calculations. I think it should be allowed. Also, sometimes I may need to add styles for root elements, or at least the .mw-parser-output itself.

Besides, why some properties such as margin-block-start are not supported?

I don't think there are any potential security issues by using variables and calculations. I think it should be allowed. Also, sometimes I may need to add styles for root elements, or at least the .mw-parser-output itself.

Besides, why some properties such as margin-block-start are not supported?

Please don’t mix up all issues; see:

Tacsipacsi renamed this task from Support CSS variables to Support CSS variables in TemplateStyles.Jan 18 2024, 5:21 PM

Tagging DST as if this task was done it could have repercussions on the use of design tokens in articles.

I think at very least we should support using CSS variables in template styles. Defining CSS variables seems like it requires a little more thought (and probably should be encouraged on the MediaWiki:Common.css level if anywhere)

As long as defining CSS variables is scoped to .mw-parser-output children as everything else, is there a problem with it if we would allow to use them? (other than sanitisation, but that one seems potentially solvable by code reuse)

As long as defining CSS variables is scoped to .mw-parser-output children as everything else, is there a problem with it if we would allow to use them? (other than sanitization, but that one seems potentially solvable by code reuse)

I think this just needs a bit more thought than I can provide right now as it creates quite a few social challenges. For example, let's say MediaWiki:Common.css defines @wikipedia-color-infobox-bg if a template can define its own CSS variables, that means it can override the value for anything inside the template (CSS variables are inherited). While in some cases this might be desired, in others I can imagine that creating on-wiki issues. Add to this, situations where templates use the same class, it could create complications in how our scoped CSS works.

To be more precise, if you have var( --wikipedia-color-infobox-bg ) and it gets overridden on template level to some other value, that could affect things outside the template scope, yes. But so can just putting .infobox { background: #dcc } in TemplateStyles, no? That’s not really fundamentally different things. And the nesting behaviour you describe is already possible via .template .infobox { background: #dcc } without CSS variable use. So TS already has those kinds of scoping complications.

stjn added a subscriber: CCiufo-WMF.

@CCiufo-WMF this task’s scope is different from that task’s scope.

@CCiufo-WMF this task’s scope is different from that task’s scope.

Right, T360562 is a subset of this task because while it makes using CSS variables possible, you wouldn't be able to set them (which is an ask of this task). In that case I'm going to remove this as a subtask of T355244, because that is just about using Codex design tokens, not altering them.

Encountered another instance where setting a variable on the class level would be beneficial to simpler CSS writing:
https://ru.wikipedia.org/?diff=136900868

I wanted to override the default styles that Minerva sets via its hacks where amboxes absolutely have to be the single colour, but then, unless I would be able to set CSS variables, I need to reimplement the background-image that is present on .client-js .ambox-learn-more entirely. Whereas just being able to set a different CSS variable value at the local level (not at :root) would provide a simple opportunity to change the colour.

Jdlrobson renamed this task from Support CSS variables in TemplateStyles to Support defining CSS variables in TemplateStyles.May 21 2024, 5:44 PM

There are modules where some lengths need to be hardcoded and kept in sync with styles.css. Using css variables would help with this.
Yes, they could overwrite any system-wide variables, but you could enforce some naming scheme, the simplest of which migh be --ts-something-something. Disallow :root{} variables if you want, too.

@Volker_E I’m not sure if --color-main is a good name: it’s likely to clash with software-defined variables. I’d treat standard prefixes like --color-, --background- etc. as reserved, which should not be used in on-wiki definitions (usage is of course fine).

@Volker_E I’m not sure if --color-main is a good name: it’s likely to clash with software-defined variables. I’d treat standard prefixes like --color-, --background- etc. as reserved, which should not be used in on-wiki definitions (usage is of course fine).

Yeah, maybe there should be a semi-standard prefix for gadgets/user scripts and for templates? Something short preferably. E.g. --t- for templates and --u- for user scripts.

For classes that I target using TemplateStyles, I tend to use tpl-, so I’d use --tpl- for CSS variables. However, I don’t think a prefix is necessary, as long as clashes are not realistically possible: if someone uses --hintegreundfarbe5, that probably won’t ever clash with names used by software. (And, let’s be honest, a standard prefix would not become universal anyway, since TemplateStyles is written by lots of editors without much coordination.)

TS could add any random prefix to all defined variables in its output. But I think the task description was never meant to display the required specification in regards to variable safety (and it’s unclear whether this will be implemented at all, unfortunately).

TS could add any random prefix to all defined variables in its output.

That’d be much more confusing than helpful: would other TemplateStyles pages, which consume the value, get the same prefix? (How would the extension decide when to prefix usage and when not? If a clash does actually happen, it’s impossible to programmatically determine what the author meant.) And what about consuming from user scripts or inline CSS?

But I think the task description was never meant to display the required specification in regards to variable safety

I agree that the original task description was not meant to do any such hints, but I think the edit of @Volker_E in T320322#11164642 was actually meant to show the current best practice, and as such a kind of specification for what to use.

Copying what i wrote on T200632#11221909 as its really for this ticket

My proposal for this would be something like:

  • have a naming convention like --templatestyle-[type]-varname. So you might have a css variable named --templatestyle-color-accenttext or --templatestyle-length-foo
  • templatestyles would enfoce that such variables have an @property block with appropriate syntax
  • templatestyles would enforce that if you set the variable, what you are setting it to is valud according to the middle part of its name.
  • potentially templatestyles could enforce that you only use the variable in places where values of the type in the middle part of the name are expected.

I think this would accomplish the security requirements:

  • they are implicitly namespaced. You can't effect stuff outside of the article because you can only set variables named with the --templatestyles- prefix which other css wouldn't use.
  • @property would protect from inlinestyles or other css setting it to something it shouldnt be (this assumes the attacker cannot make their own @property blocks. If your assuming the attacker can make @rules then all hope is lost as they can just @import)
  • templatestyles can know what type of stuff the variable contains by its name and enforce it is only used in appropriate contexts.

I think that would allow variables to be defined and used while still maintaining security properties we want.

I think the second security requirement is not really necessary if the variable name would enforce what values it might have and TemplateStyles would only accept CSS variables of a particular type. As you yourself say, it’s not a particularly strong protection anyway, so unless it would be required by TS to validate the variable values, it doesn’t seem useful to have.

I think the second security requirement is not really necessary if the variable name would enforce what values it might have and TemplateStyles would only accept CSS variables of a particular type. As you yourself say, it’s not a particularly strong protection anyway, so unless it would be required by TS to validate the variable values, it doesn’t seem useful to have.

I was basing that part on anomie's comment at T200632#4623300 that template styles should be secure in the presence of a malicious style attribute (in the event mw's style attribute sanitizer fails)

I can see the argument for that being over paranoid, but the templatestyles threat model could probably be described as over paranoid in general.