1320474 - CSS animation-name should accept <string> as well as <custom-ident>

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Kew [:jfkthame]]

According to [1], the animation-name property and @keyframes at-rule are supposed to allow either <custom-ident> or <string> values for the <keyframes-name>.

Gecko currently doesn't accept <string>s here. (Neither does Blink, AFAICT, whereas Webkit does.)

This came to my attention because I ran into a Webkit example[2] where the animation-name property used a <string>, and was having trouble getting Gecko to accept it.

For compatibility with content authored for Webkit, we should probably add support for <string> values.

[1] https://drafts.csswg.org/css-animations/#typedef-keyframes-name
[2] https://jsfiddle.net/k6pkvj66/1/

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Allow <string> in addition to <custom-ident> for css-animation keyframe names

Is this a change we want, or should we push back against it in the spec (and assert that webkit is wrong to support it)?

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

This changed was actually proposed by me when I looked into a web compatibility issue :) It was supported by tabatkins, and the CSSWG resolved to accept this change. Birtles also seems to agree with that. [1]

[1] https://github.com/w3c/csswg-drafts/issues/118

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #0)
> Gecko currently doesn't accept <string>s here. (Neither does Blink, AFAICT,
> whereas Webkit does.)

FWIW, Blink accepts <string> for @-webkit-keyframes and probably also -webkit-animation-name.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8814593 [details] [diff] [review]
Allow <string> in addition to <custom-ident> for css-animation keyframe names

Review of attachment 8814593 [details] [diff] [review]:
-----------------------------------------------------------------

These changes seem fine, but do we correctly allow "initial" and "none" as strings while disallowing them as identifiers as per the example in [1]?

r=me with tests added for that, assuming this passes said tests.

Currently, most CSS animation tests of this sort live in:

  layout/style/test/test_animations.html

It's a real mess though. Once the CSSWG moves its tests to web-platform-tests we can work on splitting that mammoth test up into smaller (cross-browser) tests.

[1] https://drafts.csswg.org/css-animations/#typedef-keyframes-name

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Specifically exclude CSS-wide keywords (and <none>) from use as the <custom-ident> in an @keyframes rule

Good point, it turns out that @keyframes happily accepts the CSS-wide keywords at present, although they're not accessible because the animation-name parsing will treat them specially. But once animation-name accepts a string, it becomes possible to use "@keyframes initial { ... }", which shouldn't be allowed. So we need to make sure we reject those identifiers in the @keyframes parsing, I think. (Do we have a better way to do that than explicitly checking like this?)

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: tests - Add tests for the use of <string> as keyframes-name

Here are some simple tests for <string> support; with both the above patches, they all pass (whereas there are a couple of failures without patch 2, because we don't ignore the illegal <custom-ident> versions of the rules).

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8814910 [details] [diff] [review]
tests - Add tests for the use of <string> as keyframes-name

Review of attachment 8814910 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_animations.html
@@ +2088,5 @@
> +div.style.animation = "unset";
> +div.style.animation = "initial 1s linear";
> +is(cs.getPropertyValue("left"), "0px", "animation name 'initial' as identifier is ignored");
> +div.style.animation = "unset";
> +div.style.animation = "\'initial' 1s linear";

Oops, just noticed the inconsistent escaping/quoting here - harmless, but untidy. I've fixed it locally.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: tests - Add tests for the use of <string> as keyframes-name

Updated to include a version of the tests in test_animations_omta.html, as requested by comments in test_animations.html; sorry, I overlooked this first time around.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8814908 [details] [diff] [review]
patch 2 - Specifically exclude CSS-wide keywords (and <none>) from use as the <custom-ident> in an @keyframes rule

Review of attachment 8814908 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that one nit fixed

::: layout/style/nsCSSParser.cpp
@@ +4407,5 @@
> +  if (mToken.mType == eCSSToken_Ident) {
> +    // check for keywords that are not allowed as custom-ident
> +    nsCSSKeyword kw = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +    switch (kw) {
> +    case eCSSKeyword_inherit:

Nit: case labels should be indented according to our coding style.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8815078 [details] [diff] [review]
tests - Add tests for the use of <string> as keyframes-name

Review of attachment 8815078 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for writing these.

I actually don't think the test_animations_omta.html tests are needed since this purely effects animation setup which is common to both main-thread and compositor animations.

I know there's that comment at the top of test_animations.html / test_animations_omta.html about keeping them in sync, but I actually think it would be fine and even preferable to just include a comment saying that we deliberately haven't included them here since the code under test does not have any effect on compositor animations. We already have a comment like that for animation-direction tests (see the comment starting, "We don't need to include all the animation-direction related tests found in test_animations.html").

r=me with that

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8814908 [details] [diff] [review]
patch 2 - Specifically exclude CSS-wide keywords (and <none>) from use as the <custom-ident> in an @keyframes rule

Could this be restructured to use the ParseCustomIdent method rather than adding yet another list of the CSS-wide keywords?

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Specifically exclude CSS-wide keywords (and <none>) from use as the <custom-ident> in an @keyframes rule

Modified to use ParseCustomIdent instead of explicitly listing the CSS-wide keywords; carrying over r=birtles, as this doesn't affect behavior. I'll fold the two code patches together for landing, to avoid an intermediate state that incorrectly accepts CSS-wide identifiers here.

Comment 14

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bed1ca42b7b4a29ef1638bee5156ba27ff3c4c7d
Bug 1320474 - Allow <string> in addition to <custom-ident> (excluding css-wide keywords and 'none') for css-animation keyframe names. r=birtles

https://hg.mozilla.org/integration/mozilla-inbound/rev/f41a419197de988085637d058a9d86f1a9cc4014
Bug 1320474 - Add tests for the use of <string> as keyframes-name. r=birtles

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bed1ca42b7b4
https://hg.mozilla.org/mozilla-central/rev/f41a419197de