1268858 - CSS Custom properties don't seem to work with animations

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Kingston [:jkt] he/him]

There appears to be a regression between 48 and 49 nightly in that I can't make transitions that contain a custom property.

http://codepen.io/anon/pen/aNRayE

In nightly this just uses the final state without any transition where as chrome and all other firefox versions do the transition animation.



Specifically this is the code that no longer works.
:root {
  --panel-move-to: -600px;
}
.container:hover {
  animation: movement 0.5s linear;
}
@keyframes movement {
  from {
    transform: translate(0, 0);
  }
  to {
    transform: translate(var(--panel-move-to), 0);
  }
}


Is this expected?

Comment 1

[Cameron McCormack (:heycam)]

I don't think that's expected.

Comment 2

[Alice0775 White]

regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3baed5c339e31188949641cb7bc7178a71070718&tochange=52c0128aff10f4c50ae3ab34c64cef8844497942

regressed by: Bug 1260655

Comment 3

[Jonathan Kingston [:jkt] he/him]

Confirm 48 has the issue also.

Comment 4

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

Wow, I didn't realize we supported that (we don't have any tests for it). I'll look into it next week or so.

Feel free to steal it from me though!

Comment 5

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

I had a quick look at this but I won't finish it today and we have public holidays for the next three days so for my own notes, we're tripping up on this test:

  https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/dom/animation/KeyframeUtils.cpp#479

I think what we need to do is:

1. Create a reftest (or a scripted test that uses getComputedStyle) for this for
   CSS animations and script-generated animations -- maybe CSS transitions too?
2. Create a script-based test that checks that we get the right thing back from
   getFrames() for this case (for *both* CSS animations and script-generated
   animations) -- we should probably update the code to do a trim() on the
   string too so that the results we get back here are more sensible --
   currently we'll get back " translate(var(--panel-move-to), 0)" as the string.
3. Create a browser chrome test that calls the privileged getProperties() test
   that checks that we correctly resolves variable references
4. Get the tests in (3) to pass -- it might be as simple as removing the check
   linked-to above, but doing that might break other tests.

Comment 6

[Jonathan Kingston [:jkt] he/him]

Attachment: MozReview Request: Bug 1268858 - Adding reftest for animation variables

Review commit: https://reviewboard.mozilla.org/r/50019/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50019/

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5896e929871b

Comment 8

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

I just pushed the whitespace changes patch so it doesn't bitrot.

The rest of the changes (minus the reftest) are running through try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb21b0017bb8

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5896e929871b

Comment 10

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

[Tracking Requested - why for this release]: web-facing regression

Comment 11

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

Attachment: part 1 - Move keyframe sequence test data to appropriate place in keyframe-utils.js

MozReview-Commit-ID: 8laNGaElWz6

Comment 12

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

Attachment: part 2 - Distinguish between valid and invalid token stream values in keyframe value handling

In order to support CSS variable references in animation properties we need to
handle token stream values. However, we already use token stream values for two
other purposes:

* To store shorthand property values
* To store invalid longhand property values

The shorthand property value case is already handled correctly, however for
longhand values we need to distinguish between valid token streams (e.g.
values that contain variable references) and invalid values stored as token
streams.

This patch makes us do that by looking at the mPropertyID member of the
nsCSSValueTokenStream object which will be eCSSProperty_UNKNOWN for an invalid
value but will be set to something sensible for a valid token stream.


MozReview-Commit-ID: AXUaO5dopBC

Comment 13

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

Attachment: part 4 - Add tests that getFrames() returns the correct result for CSS variables

MozReview-Commit-ID: BZrd3FAhrrf

Comment 14

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

Attachment: part 5 - Add tests that getProperties() returns the correct result for CSS variables

MozReview-Commit-ID: D0t1CFaa7DZ

Comment 15

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

Attachment: part 3 - Rework serialization of keyframe values so that CSS variable values are trimmed

This is so that when we write:

  @keyframes move {
    to { left: var(--left-offset); }
  }

And we later call getFrames() to inspect the keyframe values we get back
"var(--left-offset)" instead of " var(--left-offset)".

To do this we move the serialization work into KeyframeUtils which is where
it really belongs since KeyframeUtils is where we encapsulate the knowledge
of how different kinds of property values (invalid values, shorthand values
etc.) are represented.

We make the |aResult| parameter an nsString value since we want to call
Trim() which is not available on nsAString (and alternatives such as using
nsContentUtils::TrimWhitespace and returning a dependent substring are messy
in this case).

MozReview-Commit-ID: IKWLSexsu5o

Comment 16

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

https://reviewboard.mozilla.org/r/50019/#review47963

::: layout/reftests/css-variables/variables-animation.html:2
(Diff revision 1)
> +<style>
> +  @keyframes a {
> +    from { background-color: var(--my-prop) }
> +    to { background-color: var(--my-prop);}
> +  }
> +  :root {
> +    --my-prop: red;
> +  }
> +  p { --my-prop: purple; animation-duration: 1s; animation-fill-mode: both; animation-name: a }
> +  p#myid { --my-prop: blue }
> +</style>
> +<p>This should be purple, and might cache background in the rule tree.</p>
> +<p id="myid">This should be blue, and can't used that cached struct,
> +which really shouldn't be cached anyway.</p>

I think this test is more complex than it needs to be (and the text doesn't really make sense).

I'm not sure if we even need a reftest after all since I think the tests in part 4~5 might be enough. But if we decide we do need a reftest, I wonder if we could do something like the following would be better:

div {
  width: 100px;
  height: 100px;
  background-color: red;
  animation: a 1s both;
}
@keyframes a {
  from { background-color: var(--green) }
  to { background-color: var(--green) }
}
:root {
  --green: green;
}

<div></div>

Comment 17

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

I realised I forgot to testing variables in shorthands.

Also, with the trimming behavior, we can't distinguish between invalid and valid shorthand values at the point where we go to serialize them so I guess we will end up trimming invalid shorthands--maybe that's ok though, or perhaps we should only ever serialize values set from CSS keyframes like I was doing originally: https://hg.mozilla.org/try/rev/732fc2886f3f337397026b9b5e711205740fd84b.

Comment 18

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

Comment on attachment 8750125 [details] [diff] [review]
part 2 - Distinguish between valid and invalid token stream values in keyframe value handling

Cancelling review request for all except the first patch in this series since I think the variable-in-shorthand case might need some further thought.

Comment 19

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

I discovered that when we have something like:

  @keyframes a {
    to { margin: var(--var-100px) }
  }

inside CSSAnimationBuilder::GetKeyframePropertyValues, aKeyframeRule->Declaration()->HasNonImportantValueFor(prop) will return true for each of the four margin-* properties. When we call aDataBlock->ValueFor(prop) for each, however, we'll get back token stream nsCSSValue containing " var(--var-100px) " and listing "margin" as the shorthand property.

getFrames() in that case will return an empty string for each of the properties since when nsCSSValue::AppendToString seems a token stream value with a valid shorthand property it will append nothing to the string. That's easy to fix but I wonder what getFrames() *should* be returning though?

getFrames() was originally intended to preserve the input as best as possible, i.e. preserve shorthands, em units etc. so authors get back a predicatable result (and so that when authors specify animation values in % or em units we can recompute their values when the context changes).

However, when we came to integrate CSS animation building into this, it proved too difficult to maintain shorthands in light of all the complex cascading that takes place when resolving overlapping keyframes so we just decided to always expand to longhand properties for that case.

So now, given input like the following:

  :root {
    --two-values: 10em 20em;
  }
  body {
    font-size: 10px;
  }
  @keyframes a {
    to {
      margin: var(--two-values);
      marginRight: 30em;
    }
  }

we *could* return from getFrames() a number of different things including:

A. [ { marginBottom: '0px',
       marginLeft: '0px',
       marginRight: '0px',
       marginTop: '0px' },
     { marginBottom: '100px',
       marginLeft: '200px',
       marginRight: '300px',
       marginTop: '100px' } ]

i.e. expand all values out to their computed values and never return 'var()'
values. This is somewhat unfortunate since the intention behind the current
architecture is that we store 'em' units in the animation keyframes, and when
the font-size changes we only need to re-compute the animation properties from
the animation keyframes.

B. [ { marginBottom: '0px',
       marginLeft: '0px',
       marginRight: '0px',
       marginTop: '0px' },
     { margin: 'var(--two-values)',
       marginRight: '30em' } ]

i.e. in this one special case we actually return a shorthand. We can probably
achieve this because when we go to serialize we can recognize a tokenstream
value that has a valid shorthand property and just serialize that. It's a bit
odd to only return shorthands in this one case though.

C. [ { marginBottom: '0px',
       marginLeft: '0px',
       marginRight: '0px',
       marginTop: '0px' },
     { marginBottom: 'var(--two-values)',
       marginLeft: 'var(--two-values)',
       marginRight: '300px',
       marginTop: 'var(--two-values)' } ]

i.e. pretty much just expose exactly what we'd store in this case. From
a debugging point of view, this is most helpful but from an author's point of
view it's the least helpful.


I suspect we should do (A) although that's probably the most involved of all
three.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fae93a432b73
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4d7509c27e

Comment 21

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

Attachment: part 2 - Distinguish between valid and invalid token stream values in keyframe value handling

In order to support CSS variable references in animation properties we need to
handle token stream values. However, we already use token stream values for two
other purposes:

* To store shorthand property values
* To store invalid longhand property values

The shorthand property value case is already handled correctly, however for
longhand values we need to distinguish between valid token streams (e.g.
values that contain variable references) and invalid values stored as token
streams.

This patch makes us do that by looking at the mPropertyID member of the
nsCSSValueTokenStream object which will be eCSSProperty_UNKNOWN for an invalid
value but will be set to something sensible for a valid token stream.


MozReview-Commit-ID: AXUaO5dopBC

Comment 22

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

Attachment: part 3 - Make CSS Animations store computed values

This allows us to represent values specified using CSS variable references in
a predictable fashion.

MozReview-Commit-ID: D9KUUhCxPW4

Comment 23

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

Attachment: part 4 - Add tests that getFrames() returns the correct result when CSS variables are used

MozReview-Commit-ID: BZrd3FAhrrf

Comment 24

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

Attachment: part 5 - Add tests that getProperties() returns the correct result for CSS variables

MozReview-Commit-ID: D0t1CFaa7DZ

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fae93a432b73
https://hg.mozilla.org/mozilla-central/rev/1d4d7509c27e

Comment 26

[Cameron McCormack (:heycam)]

Comment on attachment 8750598 [details] [diff] [review]
part 2 - Distinguish between valid and invalid token stream values in keyframe value handling

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

::: dom/animation/KeyframeUtils.cpp
@@ +840,5 @@
>      // In either case, store the string value as a token stream.
>      nsCSSValueTokenStream* tokenStream = new nsCSSValueTokenStream;
>      tokenStream->mTokenStream = aStringValue;
> +
> +    // We are about to converted a null value to a token stream value but

"convert"

Comment 27

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

Previous try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffa7ecd9e52f

After rebasing (Linux only):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=536ab144ea38

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24c217726c36
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed73d118cd2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc88578182bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/70e971effcd1

Comment 29

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

I'll request approval for landing on Aurora some time next week.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24c217726c36
https://hg.mozilla.org/mozilla-central/rev/ed73d118cd2e
https://hg.mozilla.org/mozilla-central/rev/dc88578182bb
https://hg.mozilla.org/mozilla-central/rev/70e971effcd1

Comment 31

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

Attachment: Roll-up patch for landing on Aurora

I'll request aurora approval once I verify that I didn't break anything while rebasing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e47ef31830

Comment 32

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

Comment on attachment 8753133 [details] [diff] [review]
Roll-up patch for landing on Aurora

Approval Request Comment
[Feature/regressing bug #]: bug 1260655
[User impact if declined]: CSS Animations with @keyframes rules that use CSS variables will not run (for the properties that use those rules)
[Describe test coverage new/current, TreeHerder]: Includes tests which have been running on m-c for about 3 days
[Risks and why]: No obvious risks -- just tightens up some error handling and calculates computed values earlier in the pipeline (which is what the previous implementation did prior to the regressing bug)
[String/UUID change made/needed]: n/a

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8753133 [details] [diff] [review]
Roll-up patch for landing on Aurora

New web compat regression, we will have the beta cycle to fix potential fallout from this patch, have plenty of new tests, taking it

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9fe9c798d44f