Closed Bug 1268858 Opened 4 years ago Closed 4 years ago

CSS Custom properties don't seem to work with animations

Categories

(Core :: CSS Parsing and Computation, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: jkt, Assigned: birtles)

References

Details

(Keywords: regression)

Attachments

(7 files, 4 obsolete files)

58 bytes, text/x-review-board-request
Details
13.28 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.84 KB, patch
heycam
: review+
Details | Diff | Splinter Review
14.76 KB, patch
heycam
: review+
Details | Diff | Splinter Review
9.20 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.87 KB, patch
heycam
: review+
Details | Diff | Splinter Review
35.72 KB, patch
Details | Diff | Splinter Review
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?
I don't think that's expected.
Confirm 48 has the issue also.
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!
Assignee: nobody → bbirtles
Flags: needinfo?(bbirtles)
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.
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
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
Keywords: leave-open
[Tracking Requested - why for this release]: web-facing regression
MozReview-Commit-ID: 8laNGaElWz6
Attachment #8750124 - Flags: review?(cam)
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
Attachment #8750125 - Flags: review?(cam)
MozReview-Commit-ID: BZrd3FAhrrf
Attachment #8750127 - Flags: review?(cam)
MozReview-Commit-ID: D0t1CFaa7DZ
Attachment #8750128 - Flags: review?(cam)
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
Attachment #8750129 - Flags: review?(cam)
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>
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 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.
Attachment #8750125 - Flags: review?(cam)
Attachment #8750129 - Flags: review?(cam)
Attachment #8750127 - Flags: review?(cam)
Attachment #8750128 - Flags: review?(cam)
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.
Attachment #8750124 - Flags: review?(cam) → review+
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
Attachment #8750598 - Flags: review?(cam)
Attachment #8750125 - Attachment is obsolete: true
This allows us to represent values specified using CSS variable references in
a predictable fashion.

MozReview-Commit-ID: D9KUUhCxPW4
Attachment #8750599 - Flags: review?(cam)
Attachment #8750127 - Attachment is obsolete: true
Attachment #8750128 - Attachment is obsolete: true
Attachment #8750129 - Attachment is obsolete: true
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"
Attachment #8750598 - Flags: review?(cam) → review+
Attachment #8750599 - Flags: review?(cam) → review+
Attachment #8750600 - Flags: review?(cam) → review+
Attachment #8750601 - Flags: review?(cam) → review+
I'll request approval for landing on Aurora some time next week.
Keywords: leave-open
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 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
Attachment #8753133 - Flags: approval-mozilla-aurora?
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
Attachment #8753133 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1326131
You need to log in before you can comment on or make changes to this bug.