Page MenuHomePhabricator

CSS sanitizer should support using CSS variables (not setting/creating them) for use in color values in TemplateStyles
Closed, ResolvedPublic5 Estimated Story Points

Description

Background

The CSS sanitizer doesn't allow CSS variables, which means that editors cannot follow our guidance on https://www.mediawiki.org/wiki/Recommendations_for_night_mode_compatibility_on_Wikimedia_wikis#Use_CSS_variables_or_CSS_design_tokens_with_fallback_for_background_and_text_where_possible

There is a broader request for CSS variables in T320322 but this is the minimum level of support we need to support night mode.

User story

As a template editor I want to use Codex design tokens for my CSS rules, so that they adapt to the night theme experience.

Requirements

"

/* {{pp|small=y}} */
.tracked {
	float: right;
	clear: right;
	margin: 0 0 1em 1em;
	width: 12em;
	border: 1px solid var( --border-color-interactive );
	border-radius: 2px;
	background-color: var( --background-color-neutral );
	font-size: 85%;
	text-align: center;
	padding: 0.5em;
}
  • (for now) I should not be able to reassign or define CSS variables. e.g. the following should trigger an error:
:root {
  --color-base: red;
}
html.skin-theme-clientpref-os #shortcut 
	--color-base: red;
}

Sign off step

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson set the point value for this task to 5.Mar 21 2024, 5:57 PM
Jdlrobson added a subscriber: Mabualruz.

We'd like to use this opportunity to knowledge share as @Mabualruz is the only member of the team comfortable with this code and this type of change.

Jdlrobson renamed this task from CSS sanitizer should support using CSS variables (not setting/creating them) to CSS sanitizer should support using CSS variables (not setting/creating them) for use in TemplateStyles.Mar 21 2024, 8:50 PM

I talked to @Catrope and we agreed that it is okay to fix this bug, but that there is a broader problem here at T340477 that needs to be solved before calling CSS design tokens "stable".

Btw, keep in mind that CSS variables can have a fallback like var( --color-base, #000 ), maybe that is a good thing to advise in the variable use so that dropping a certain token doesn’t cause too many problems.

Change #1014653 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[css-sanitizer@master] [WIP] Implement CustomPropertyMatcher for CSS Variable Handling

https://gerrit.wikimedia.org/r/1014653

Blocked until I get a pairing partner to share knowledge and discuss next steps

Remaining Steps:

- Review and merge the css-sanitizer patch

- Update the version of css-sanitizer

How to update the version of css-sanitizer involves a process that typically follows these steps:
  • CSS Sanitizer Updates: When updates or enhancements are made to the CSS Sanitizer, these changes are committed and then tagged in the repository. This tagging is crucial for version management and indicates that a new stable version of the sanitizer is available.
  • Publishing to Packagist: The new version of the CSS Sanitizer needs to be published on Packagist, which is a repository for PHP packages. This makes the updated version available for use by other projects, including TemplateStyles.
  • Updating TemplateStyles: The TemplateStyles extension, which uses the CSS Sanitizer to ensure the CSS applied to MediaWiki templates is secure, must then update its composer.json file to require the new version of the CSS Sanitizer. This ensures that the extension benefits from the latest security and functionality improvements.
  • Testing and Deployment: After updating the dependency in TemplateStyles, thorough testing is recommended to ensure compatibility and that new features or security enhancements work as expected within the MediaWiki environment.

To locally test the TemplateStyles extension with changes from the CSS Sanitizer, follow this guide:

  • Place both css-sanitizer and TemplateStyles directories side by side in the same parent directory.
  • Open the composer.json file in the TemplateStyles directory.
  • Add the following to the composer.json to specify the local path of the CSS Sanitizer and set the version requirement to use the local development version:
"repositories": [
    {
        "type": "path",
        "url": "../css-sanitizer"
    }
],
"require": {
    "wikimedia/css-sanitizer": "@dev"
}
  • Run composer update in the TemplateStyles directory to apply the local CSS Sanitizer.

This setup will enable local development and testing with the CSS Sanitizer.

Change #1015447 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/extensions/TemplateStyles@master] Bump Template Styles Extension to 5.2.0

https://gerrit.wikimedia.org/r/1015447

Last step for deployment we need to update the mediawiki/vendor repository with the new version of the CSS Sanitizer after successful local tests:

- Visit the mediawiki-vendor repository page and check the README.md file for specific commands and procedures.
  1. Typically, this involves submitting a patch with the updated composer.json and composer.lock files that reference the new version of the CSS Sanitizer, and updated vendor files to reference the new version/package.
  2. Follow the repository's contribution guidelines for how to submit these changes for integration.

This process ensures that the new version is available for the broader MediaWiki ecosystem

Jdlrobson added a subscriber: cscott.

Hey @cscott would you be able to code review another css sanitizer patch? If so thanks in advance, if not let us know how much time you need.

Jdlrobson lowered the priority of this task from High to Medium.Apr 3 2024, 11:00 AM

Reviewed https://gerrit.wikimedia.org/r/c/css-sanitizer/+/1014653 ; once that is updated and merges I can help release a new version of css-santizier -- that library really needs a HISTORY.md file to document its release history. The dependency in TemplateStyles is ~5.0.0 so it should probably get its composer.json updated to something like "wikimedia/css-sanitizer": "^5.1.0 || ^5.2.0" in preparation for a 5.2.0 release of css-sanitizer.

Change #1014653 merged by jenkins-bot:

[css-sanitizer@master] Handle custom properties in color attributes

https://gerrit.wikimedia.org/r/1014653

Change #1016381 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/vendor@master] Update CSS Sanitizer to 5.2.0

https://gerrit.wikimedia.org/r/1016381

Change #1016381 merged by jenkins-bot:

[mediawiki/vendor@master] Update CSS Sanitizer to 5.2.0

https://gerrit.wikimedia.org/r/1016381

This doesn't appear to be working just yet. What did we miss?

Screenshot 2024-04-05 at 2.12.21 PM.png (1×2 px, 342 KB)

Change #1017080 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[css-sanitizer@master] feat(css-sanitizer): Improve color parsing var func fallback values

https://gerrit.wikimedia.org/r/1017080

Due to a misunderstanding in requirements, we've added support for:

color: var( --color-base );

but not:

color: var( --color-base, #222; )

Community members can workaround this by using body.skin-minerva or wait until T361934 is completed. We are not going to get this fixed in a day so I've created T361934. For the community members subscribed to this ticket, very sorry about the delay here.

Jdlrobson updated the task description. (Show Details)

I just had a discussion on slack with @cscott, about the new sub task and follow up work in summary:

  • I proposed adding fallback values for the CSS var() function and introduced a patch on T361934.
  • C. Scot raised questions about the necessity of fallback values, given that styles should be predefined. He expressed concerns regarding the security of using var() and preferred to keep the implementation simple.
  • I explained that certain styles require fallback values. And suggested postponing the security discussion. I assured that fallbacks would only include simple color codes and names.
  • Both discussed the effectiveness of using var() compared to direct color values.
  • C. Scot was not concerned about the code implementation, more for the necessity of the change.
  • C. Scot proposed using the following examples to simulate fallback values/defaults:
background-color: #fff;
background-color: var( --skin-specific );

and
border: 1px solid #72777d;
border: 1px solid var( --border-color-interactive );

@Mabualruz unless you hard-code something to switch it to var( --skin-specific, #fff ), this example would lead to override of #fff to an empty value in all the browsers supporting var() in case --skin-specific is undefined.

@stjn can we move the discussion to T361934, I wanted to summarise the movement of the discussion there for everyone's benefit. Thanks

Change #1017080 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[css-sanitizer@master] feat(css-sanitizer): Improve color parsing var func fallback values

https://gerrit.wikimedia.org/r/1017080

Not really done, it's only supported for color-related properties.

A digression for the specific scope of this task, but mentioning it here in case it can inform direction / the discussion:

I'm in the middle of a cleanup of default Gadgets etc. on enWS, and am quietly muttering impolite words to myself because of formerly-Common.css-now-in-default-Gadget global CSS classes that have been used in a myriad of templates (and ad hoc in page content) with no particular rhyme or reason.

BUT… the one good use case in all the mess is that these global classes are used to provide a standardised ("house style for enWS") set of backgrounds and borders ("etc.") for what would otherwise be a whole little forest of interrelated templates needing to be kept in sync manually. I.e. a classic use for a style sheet.

For our particular use case it would be useful for the project to be able to define certain global variables that were available to people maintaining the TemplateStyles for individual templates. Mostly colors, lengths, sizes, border radii, and similar. It would be perfectly fine for this ability to be restricted to Interface Admins as the desirable management model for this use case coincides well with the IA model. We might even be able to make do with this being a Site-request rather than IA-limited, but I think that probably isn't a good idea from a sustainability perspective.

It can kinda sorta be done with the default gadgets, but there's a subtle difference in control delineation between making classes (selectors) global and providing re-usable variables. The need also overlaps with what would be covered by supporting internal (local-only) @import within TemplateStyles CSS, but that's a slightly different use case.

A digression here, of course, but might be useful for thinking about the issue.

PS. Note also that the use case I'm describing above, and that this task is currently trying to solve, is very different from the one in T200632. The latter is about end-user users of a template being able to pass an argument to the template that the template then passes into TemplateStyles in order to vary colors, widths, etc. per-invocation of the template. The security profile and barriers to implementation (e.g. T200632#4623300) are quite different.

@Tgr T320322 covers the general use case.

Jdlrobson renamed this task from CSS sanitizer should support using CSS variables (not setting/creating them) for use in TemplateStyles to CSS sanitizer should support using CSS variables (not setting/creating them) for use in color values in TemplateStyles.Apr 7 2024, 1:29 PM

@Xover what about just using Codex design tokens? See T355244.

Change #1015447 abandoned by Mabualruz:

[mediawiki/extensions/TemplateStyles@master] chore(extensionDependencies): Update css-sanitizer from 5.1.0 to 5.2.0

Reason:

Was done in 1021564

https://gerrit.wikimedia.org/r/1015447