Page MenuHomePhabricator

Allow grid-template-columns: subgrid;
Open, Needs TriagePublicFeature

Description

Allow grid-template-columns: subgrid; in sanitized styles.

It seems that adding new KeyWordMatcher( 'subgrid' ) to src/Sanitizer/StylePropertySanitizer.php is enough.

Event Timeline

Change 986638 had a related patch set uploaded (by Alex Mashin; author: Alexander Mashin):

[css-sanitizer@master] Add 'subgrid' to grid-template-(columns/rows)

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

Subgrid is currently at editor's draft, which is not usually sufficiently stable to add to CSS sanitizer.

Subgrid is currently at editor's draft, which is not usually sufficiently stable to add to CSS sanitizer.

Already supported by browsers.

Subgrid is currently at editor's draft, which is not usually sufficiently stable to add to CSS sanitizer.

Already supported by browsers.

That's true, but not the standard we use in this code base. Previous standard has been at least candidate recommendation from memory.

It's in the candidate recommendation draft (and even in the candidate recommendation although that pretty aggressively recommends using the candidate recommendation draft instead).

I'd rather not have the library support a mix of grid 1 and grid 2, but then it's still better than being stuck on grid 1 for five more years. Let's wait a few weeks and see if the extension gets picked up by the new(ish) MediaWiki teams, if not I'll merge the patch.

Nice catch!

(and even in the candidate recommendation although that pretty aggressively recommends using the candidate recommendation draft instead).

A CR Draft is a type of CR. (The other kind of CR is a CR Snapshot.) A Draft is supposed to be an intermediate document between each Snapshot, which I assume are the only kinds of CR that can be promoted to Recommendation (maturity stages).

I'd rather not have the library support a mix of grid 1 and grid 2,

I think we've merged mixed level support before. Probably makes it difficult for assessing what still needs to be done to support a module, but specific calls for specific CSS should probably be supported as they appear after they approach an appropriate level of support by the W3C. Probably worth an adjustment to this patch to call this addition out, since I recall somewhere in the code base some modest and likely incomplete documentation of what level of support is provided.

A CR Draft is a type of CR. (The other kind of CR is a CR Snapshot.) A Draft is supposed to be an intermediate document between each Snapshot, which I assume are the only kinds of CR that can be promoted to Recommendation (maturity stages).

Thanks, I didn't realize those are at the same level.

I think we've merged mixed level support before.

Mixed in the sense of different levels for different modules yes, mixed within a module I don't think so.

I recall somewhere in the code base some modest and likely incomplete documentation of what level of support is provided.

It's in the README and AFAIK pretty complete. There's also per-method documentation in MatcherFactory and PropertySanitizer.

but specific calls for specific CSS should probably be supported as they appear after they approach an appropriate level of support by the W3C.

The appropriate level of support is that it's published in a CR, and then that affects the entire module, no?
I'm not necessarily against including things on some different basis (there was a discussion about vendor prefixes for example), I'm just saying it's not ideal. (According to the docs we only did it once for touch-action.) A full module update is a lot of work though, and the extension is unmaintained so I don't think it's a good reason for blocking patches, I just want to see if it gets a maintainer as the MediaWiki Engineering teams are scoping out their work.

but specific calls for specific CSS should probably be supported as they appear after they approach an appropriate level of support by the W3C.

The appropriate level of support is that it's published in a CR,

Or whatever level, yes. c.f. task I filed today (I had been thinking about that separately but this task finally provided enough motivation for it).

and then that affects the entire module, no?

Not sure what you mean by this comment. I think it's that you're concerned some systems are supposed to parse things a certain way and those ways can change between module levels (CSS Colors as a particular example). Yes, of course. This particular task's addition doesn't strike me as being problematic in that way, but that is something worth evaluating for each change in this code base which isn't a systematic upgrade of modules and addition of new modules (as with the reedy patch this past year or two).

I'm not necessarily against including things on some different basis [snip]

It looks like I mostly/totally agree with you here?

@Tgr, could you please approve the change, or reject it, or send it to someone else for reviewing?