[css-values-3] Restrict none/auto/normal from <custom-ident>

[fantasai]

We often use these keywords in grammars, so @tabatkins suggested we blanket-restrict them the same way we do all the global keyboards (initial/inherit/etc.)

[dbaron]

I gathered some compatibility data on this using Cluster Telemetry, specifically with trace counters using this patch. I also included default as well because there's an existing Chromium bug on fixing Chromium to include default.

I ran through a pageset of 10K pages (generally the homepages of sites, and cached rather than live pages) with the custom ident parsing function in Chromium's CSS parser instrumented. This detected the following pages as being affected by changes to the set of allowed values in <custom-ident>, for the set of features supported by Chromium's CSS parser:

normal:
no pages found

none:

1.0,http://www.inc.com (#1243)
2.0,http://www.ebscohost.com (#5605)
1.0,http://www.lopezdoriga.com (#9061)
1.0,http://www.buzzfeednews.com (#9439)
4.0,http://www.bonappetit.com (#9508)

auto:

1.0,http://www.madhyamam.com (#2013)
3.0,http://www.freebitco.in (#2386)
1.0,http://www.chrono24.com (#3635)
3.0,http://www.coin-farm.net (#3768)
1.0,http://www.reactnative.dev (#4956)
3.0,http://www.hcaptcha.com (#5387)
3.0,http://www.dhlexpress.fr (#6459)
3.0,http://www.bestchange.com (#7807)
3.0,http://www.coin-birds.com (#8023)
3.0,http://www.mobox.io (#8435)
3.0,http://yad2.co.il (#8959)

default:

1.0,http://www.elster.de (#7992)

I have not attempted to look at any of the pages. (I could also do a re-run against live pages if it turns out the cached ones are meaningfully out-of-date.)

[dbaron]

Some further details on the compatibility data above, gathered by running Chrome locally with this patch.

These four are all grid lines named none:

* 1.0,http://www.inc.com (#1243)
* 2.0,http://www.ebscohost.com (#5605)
* 1.0,http://www.lopezdoriga.com (#9061)
* 1.0,http://www.buzzfeednews.com (#9439)

For the final none datapoint I couldn't detect any failures on the live site:

* 4.0,http://www.bonappetit.com (#9508)

This auto failure is transition-property: auto:

* 1.0,http://www.madhyamam.com (#2013)

These auto failures are animation-name: auto (with no corresponding @keyframes auto rule!):

* 3.0,http://www.freebitco.in (#2386)
* 3.0,http://www.coin-farm.net (#3768)
* 3.0,http://www.hcaptcha.com (#5387)
* 3.0,http://www.dhlexpress.fr (#6459)
* 3.0,http://www.bestchange.com (#7807)
* 3.0,http://www.coin-birds.com (#8023)
* 3.0,http://www.mobox.io (#8435)

This auto failure is a CSS counter:

* 1.0,http://www.chrono24.com (#3635)

This auto failure is color-scheme: auto:

* 1.0,http://www.reactnative.dev (#4956)

This auto failure was not observed on the live site:

* 3.0,http://yad2.co.il (#8959)

Finally, the case on http://www.elster.de where default is used: it is a use in the animation-name property (possibly via the shorthand). There is a corresponding @keyframes rule. (I tested default because it's not reserved in Chromium, but is in Gecko and WebKit.) [Edited to add: My post to blink-dev has more details on the elster.de case, which is not really relevant to this issue.]

[tabatkins]

Okay, at least for most of these the CSS is already invalid and/or useless. In particular, the CSS counter auto appears to not do anything (it looks like they were intending it to be none, maybe?). I can't find the usage of a none grid line on the first page listed, so it must be buried in one of the many, many additional requests. (And haven't looked in the others.) Without seeing it, tho, it really seems weird to have a line named none, and I suspect it's an attempt to say [] - no grid line names.

So, uh, seems like we're pretty reasonably compatible?

[tabatkins]

Agenda+ to discuss this change

[css-meeting-bot]

The CSS Working Group just discussed [css-values-3] Restrict none/auto/normal from <custom-ident>.

The full IRC log of that discussion

<fantasai> Topic: [css-values-3] Restrict none/auto/normal from <custom-ident>
<fantasai> github: [css-values-3] Restrict none/auto/normal from <custom-ident>
<fantasai> github: https://github.com//issues/7431#issuecomment-1178237576
<fantasai> TabAtkins: In the definition for <custom-ident> in Values and Units
<fantasai> TabAtkins: can be any keyword, except CSS-wide keywords
<fantasai> TabAtkins: can't name an animation "initial"
<fantasai> TabAtkins: any other keywords that need to be xcluded for grammatical ambiguity reasons, you need to specify that explicitly
<fantasai> TabAtkins: however, there's a handful of keywords that are used so often in properties, and seem so not-very-useful for naming things
<fantasai> TabAtkins: that it might make sense to restrict them as well
<fantasai> TabAtkins: specifically: auto, none, normal
<fantasai> TabAtkins: these are used in a lot of properties, and are not good names for animations/counter-styles/etc.
<fantasai> TabAtkins: First question is, are sites using these ?
<fantasai> TabAtkins: dbaron did an initial analysis, and while they are used in a few places
<fantasai> TabAtkins: they appear to be invalid in those places anywhere
<fantasai> TabAtkins: e.g. 'none' as a line name in Grid, but [missed example]
<fantasai> TabAtkins: so makes sense to treat as invalid anyway
<fantasai> TabAtkins: I believe compat impact will be minimal to none
<fantasai> TabAtkins: all uses we saw were already invalid
<plinss> q+
<fantasai> TabAtkins: and this should make custom idents safer to use in general, without us having to remember to explicitly cut out these common keywords
<andreubotella> s/[missed example]/no 'none' line
<fantasai> TabAtkins: so that's the proposal -- restrict none/auto/normal from <custom-ident>
<fantasai> plinss: If using 'none' as a line name now, would that invalidate the line name or the whole declaration
<fantasai> TabAtkins: invalidate the whole declaration
<fantasai> plinss: That's more breakage than making valid but matching nothing
<fantasai> plinss: That said, I'm not too concerned
<dbaron> One other example I've seen is the use of `auto` as a named toggle state in https://toggles.oddbird.net/#named-modes
<fantasai> plinss: probably too late to do anything, allowing custom-idents without prefixing was risky in the first place
<Rossen_> ack plinss
<fantasai> TabAtkins: definitely it's a problem we should avoid
<fantasai> plinss: Should we require that, and not repeat this mistake that allows potentially conflicting <custom-ident>
<fantasai> TabAtkins: Could do that, make these legacy
<fantasai> fantasai: I would prefer to stay consistent with existing practice
<fantasai> fantasai: Anyway that's outside the scope of the issue
<fantasai> dbaron: There are probably some uses of <custom-ident> that could be <ident> because don't conflict with other stuff
<fantasai> dbaron: not sure if that's one of them
<fantasai> TabAtkins: names, toggle-state, and line names won't
<fantasai> TabAtkins: problems are things like animation-name, which we screwed up on, and don't want to repeat that
<fantasai> plinss: original sin here was unquoted font names, all kinds of problems from day 1
<fantasai> fantasai: Back to original issue, do we exclude these keywords? I propose to accept
<fantasai> TabAtkins: I'm now less convinced that we should
<fantasai> TabAtkins: maybe revisit grammar, and require only using in unambiguous contexts like line names
<fantasai> TabAtkins: Let's not screw it up again
<fantasai> fantasai: I still think it's worth excluding these keywords
<fantasai> TabAtkins: I say take it back to issue

[tabatkins]

After the discussion today, I no longer think this is needed. The breakage potential seems higher than I initially thought, and the use-cases less important - generally, new APIs should use dashed-ident rather than custom-ident anyway. (And if it's safe to use custom-ident because it wont' mix with other keywords, like grid line names, then it's safe to name them none/auto/normal as well.)

[css-meeting-bot]

The CSS Working Group just discussed [css-values-3] Restrict none/auto/normal from <custom-ident>, and agreed to the following:

• RESOLVED: Close no change

The full IRC log of that discussion

<fantasai> Topic: [css-values-3] Restrict none/auto/normal from <custom-ident>
<fantasai> github: [css-values-3] Restrict none/auto/normal from <custom-ident>
<fantasai> github: https://github.com//issues/7431#issuecomment-1178237576
<fantasai> TabAtkins: I filed the issue, happy to withdraw
<vmpstr> fantasai: would've been nice to make it excluded, but i understand there's compat risk
<fantasai> astearns: Proposed resolution is to close no change
<TabAtkins> I didn't file the issue actually, Elika did. (But it happened as a result of us discussing it together.)
<fantasai> RESOLVED: Close no change