Closed Bug 1402219 Opened 7 years ago Closed 7 years ago

stylo: custom properties in keyframes are not used

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: xidorn, Assigned: hiro)

References

Details

Attachments

(7 files, 2 obsolete files)

testcase
229 bytes, text/html
Details
slightly changed version from hiro's testcase
1.05 KB, text/html
Details
Bug 1402219 - Check the length of returned keyframes of getKeyframes().
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1402219 - Factor custom properties in keyframes into compute values in each keyframe.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1402219 - Compute css variables with custom properties in keyframes for getKeyframes().
59 bytes, text/x-review-board-request
birtles
: review+
Details
Servo PR
41 bytes, text/x-github-pull-request
Details | Review
Servo side changes for beta
21.08 KB, patch
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file testcase — 
See the testcase.

The declaration of custom property "--endColor" in the keyframes rule is ignored for the animation.

This doesn't match either Gecko or Blink, and it can actually cause rendering difference, so I would say this is probably a bit more important than others.

There are three web-platform tests affected by this:
* css/css-variables-1/variable-animation-substitute-within-keyframe.html
* css/css-variables-1/variable-animation-substitute-within-keyframe-fallback.html
* css/css-variables-1/variable-animation-substitute-within-keyframe-multiple.html
OK. I will take a look in the next week.
I was on a plane after looking at this, and I didn't end up fixing it because I ran out of battery, but I ended up writing https://github.com/servo/servo/pull/18604.

Not sure if it's better to write on top of it. If that is somehow a step in the wrong direction, we can also close it.
Attached file Revelant test cases (obsolete) — 
This file adds two more test cases.

  @keyframes bar {
    from { color: blue; }
    to { color: var(--endColor); }
    to { --endColor: green; }
  }
  @keyframes baz {
    from { color: blue; }
    50% { --endColor: yellow; }
    to { color: var(--endColor); }
  }

I think the former should be animated from 'blue' to 'green', but Gecko doesn't (chrome does)

The latter is not clear to me what the spec says.  Anyway, Gecko does animate from blue to red (which is specified in #id selector), chromium does animate from blue -> yellow -> red.

Another thing I noticed is that Gecko ignores !important in #id selector, Chrome also ignores.

I am not sure Emilio's PR is necessary to fix this bug yet, as far as I can tell we should do

1) Don't filter out custom properties in Servo_StyleSet_GetKeyframesForName() [1]
2) Store the custom properties in PropertyDeclarationBlock in the keyframe (If we do behave as Chrome in the latter test case above, we should store the custom properties in an independent keyframe, I guess)
3) Pass the stored custom properties to AnimationValue::from_declaration()
4) Compound the stored custom properties with context's custom properties in from_declaration()

I guess this way solves the !important problem and the former case above.

[1] https://hg.mozilla.org/mozilla-central/file/b44e80e0590e/servo/ports/geckolib/glue.rs#l3619
Forgot to mention about getKeyframes().  For getKeyframes() I guess we also need to use the stored custom_properties in single_value_to_css().
Chrome's behavior is quite weird, though. For the case in comment 0 and the first case in comment 8, it animates from blue to red+blue, then animates from black+green to green. And for the second case in comment 8, it animates from blue to yellow, then jump to red.

I don't think that's what we want...

It feels to me that Gecko's behavior (for comment 0 case) is probably more straightforward (that it just computes the property value at the given keyframe and use the computed value to animate). IIRC animations are based on computed value, and variable references are removed in computed value, so sounds like Gecko's behavior for the second case in comment 8 makes sense as well.

It may be worth raising an issue to CSSWG, though.
(In reply to Hiroyuki Ikezoe (:hiro) PTO on 26 and 28 Sep. from comment #3)
> I am not sure Emilio's PR is necessary to fix this bug yet, as far as I can
> tell we should do

Yeah, I'm not saying that's required. That PR doesn't fix the bug indeed. Was just pointing out that since it's a somewhat big cleanup, it may be worth accounting for it so it doesn't need to be rebased, or duplicate any work that would need to happen in this bug.
This testcase changes the animation durations to 10s, and use background-color instead of color, so that it can be seen more clearly.
I did write code what I described in comment 3.  Three tests in comment 0 passed locally. I thought the former case (the custom property in a different keyframe block but the same offset) in comment 3 will be fixed by this approach, but actually is not. The !important case was not fixed either unfortunately. But anyway, seems to match gecko's behavior now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63c90d9b32f44d0f140a2f571a6c86b569a1744b
Attachment #8911614 - Attachment is obsolete: true
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
What Brian told me on IRC was right, the modification for getKeyframe() is not necessary. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbb82df82857d1e3b4c8f423d580468c34d3dd2b
(In reply to Hiroyuki Ikezoe (:hiro) PTO on 26 and 28 Sep. from comment #10)
> What Brian told me on IRC was right, the modification for getKeyframe() is
> not necessary. 
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=cbb82df82857d1e3b4c8f423d580468c34d3dd2b

Gah, no, I was wrong. We definitely need the modification. What I was wrong is that the test case did not check the length of returned value of getKeyframes().
I did miss that we pass computed values only for CSS animation in KeyframeEffectReadOnly::GetKeyframes(). So we need to check whether computed values is not None instead of checking custom properties is None in PropertyDeclarationBlock::single_value_to_css().
https://treeherder.mozilla.org/#/jobs?repo=try&revision=744cc62e6177ad0aa964b156e5cbff56f727154d
I am going to drop the second patch [1] here since it's not essentially related to this and also it conflicts Emilio's PR in comment 2.

[1] https://hg.mozilla.org/try/rev/9d7a462742db01cbaf188e0c0442ea43802cdf9f
The try does not finish yet, but I think I am on the right track now.
Comment on attachment 8911695 [details]
Bug 1402219 - Store custom properties in keyframes into servo's PropertyDeclarationBlock.

https://reviewboard.mozilla.org/r/183094/#review188302

Looks ok, though I'd say if possible, it'd be nice to find a solution that doesn't involve duplicating work for each animatable property in a keyframe.

::: servo/components/style/properties/properties.mako.rs:1520
(Diff revision 1)
>      }
>  
> +    /// Returns true if this property is a custom property, false
> +    /// otherwise.
> +    pub fn is_custom(&self) -> bool {
> +        match *self {

nit: You can use `matches!` here if you want.

::: servo/ports/geckolib/glue.rs:3622
(Diff revision 1)
>                  let animatable =
>                      guard.normal_declaration_iter()
>                           .filter(|declaration| declaration.is_animatable());
>  
> +                // XXXX: Use SmallVec instead?
> +                let custom_properties: Vec<&PropertyDeclaration> =

Yeah, let's use `SmallVec`.

::: servo/ports/geckolib/glue.rs:3643
(Diff revision 1)
>                          }
>  
> -                        let property = AnimatableLonghand::from_declaration(declaration).unwrap();
> +                        let mut pdb = PropertyDeclarationBlock::with_one(declaration.clone(),
> +                                                                         Importance::Normal);
> +                        for custom in custom_properties.iter() {
> +                            pdb.push((*custom).clone(), Importance::Normal);

It's somewhat unfortunate to put this in every declaration block, this is far from linear.

Can't we instead add a `PropertyValuePair` each time we find a custom property declaration (in the same spirit of what we do for animatable properties) and ensure it ends up in the final composed rule?
Attachment #8911695 - Flags: review?(emilio) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #19)

> ::: servo/ports/geckolib/glue.rs:3643
> (Diff revision 1)
> >                          }
> >  
> > -                        let property = AnimatableLonghand::from_declaration(declaration).unwrap();
> > +                        let mut pdb = PropertyDeclarationBlock::with_one(declaration.clone(),
> > +                                                                         Importance::Normal);
> > +                        for custom in custom_properties.iter() {
> > +                            pdb.push((*custom).clone(), Importance::Normal);
> 
> It's somewhat unfortunate to put this in every declaration block, this is
> far from linear.
> 
> Can't we instead add a `PropertyValuePair` each time we find a custom
> property declaration (in the same spirit of what we do for animatable
> properties) and ensure it ends up in the final composed rule?

Yeah, this is actually inefficient, but for a good thing or a bad thing, current approach does not fix below case;

  @keyframes bar {
    from { color: blue; }
    to { color: var(--endColor); }
    to { --endColor: green; }
  }

and it matches current Gecko behavior, if we stores custom properties in a new PropertyValuePair, it will solve the above case as well. Anyway I will try to store it in an independent PropertyValuePair.

Thanks!
Attachment #8911697 - Flags: review?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) PTO on 26 and 28 Sep. from comment #20)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #19)
> 
> > ::: servo/ports/geckolib/glue.rs:3643
> > (Diff revision 1)
> > >                          }
> > >  
> > > -                        let property = AnimatableLonghand::from_declaration(declaration).unwrap();
> > > +                        let mut pdb = PropertyDeclarationBlock::with_one(declaration.clone(),
> > > +                                                                         Importance::Normal);
> > > +                        for custom in custom_properties.iter() {
> > > +                            pdb.push((*custom).clone(), Importance::Normal);
> > 
> > It's somewhat unfortunate to put this in every declaration block, this is
> > far from linear.
> > 
> > Can't we instead add a `PropertyValuePair` each time we find a custom
> > property declaration (in the same spirit of what we do for animatable
> > properties) and ensure it ends up in the final composed rule?
> 
> Yeah, this is actually inefficient, but for a good thing or a bad thing,
> current approach does not fix below case;
> 
>   @keyframes bar {
>     from { color: blue; }
>     to { color: var(--endColor); }
>     to { --endColor: green; }
>   }
> 
> and it matches current Gecko behavior, if we stores custom properties in a
> new PropertyValuePair, it will solve the above case as well. Anyway I will
> try to store it in an independent PropertyValuePair.

The last patch has to be revised due to this change.
Comment on attachment 8911694 [details]
Bug 1402219 - Check the length of returned keyframes of getKeyframes().

https://reviewboard.mozilla.org/r/183092/#review188554
Attachment #8911694 - Flags: review?(bbirtles) → review+
Comment on attachment 8911695 [details]
Bug 1402219 - Store custom properties in keyframes into servo's PropertyDeclarationBlock.

https://reviewboard.mozilla.org/r/183094/#review188558

r=me with the changes you've discussed with emilio applied
Attachment #8911695 - Flags: review?(bbirtles) → review+
Emilio helped me on IRC to solve a lifetime issue of CustomPropertiesMap which is passed to custom_properties::cascade() in a function that I introduced, but after a try it turns out it's unnecessary because the function is called only if there is any custom properties in keyframe, so other custom properties in style rules were not applied at all.   What we needed to do is that when we compute a value in a keyframe, if a value is a css variables then we need to resolve the variable with context's custom properties and custom properties in the keyframe if needed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10112084c2411c691e84acfbb0feab3bc667c410
Comment on attachment 8911696 [details]
Bug 1402219 - Factor custom properties in keyframes into compute values in each keyframe.

https://reviewboard.mozilla.org/r/183096/#review188740

Looks reasonable to me, with the comments addressed. Let me know if you want me to take another look at this. r=me with the comments addressed.

::: servo/components/style/properties/declaration_block.rs:154
(Diff revision 2)
>  /// Iterator for AnimationValue to be generated from PropertyDeclarationBlock.
>  pub struct AnimationValueIterator<'a, 'cx, 'cx_a:'cx> {
>      iter: DeclarationImportanceIterator<'a>,
>      context: &'cx mut Context<'cx_a>,
>      default_values: &'a ComputedValues,
> +    custom_properties: Option<&'a PropertyDeclarationBlock>,

Let's call this `extra_custom_properties`, and document that it's a declaration block with the custom properties declared on a given keyframe (or such?).

(But, I think we may not even need this member, and we should resolve them upfront, read below)

::: servo/components/style/properties/declaration_block.rs:166
(Diff revision 2)
> +           custom_properties: Option<&'a PropertyDeclarationBlock>,
> +    ) -> AnimationValueIterator<'a, 'cx, 'cx_a> {
>          AnimationValueIterator {
>              iter: declarations.declaration_importance_iter(),
>              context: context,
>              default_values: default_values,

nit: You can use the shorthand initializer here.

::: servo/components/style/properties/declaration_block.rs:186
(Diff revision 2)
>                          let property = AnimatableLonghand::from_declaration(decl);
> -                        let animation = AnimationValue::from_declaration(decl, &mut self.context,
> +                        let animation = AnimationValue::from_declaration(
> +                            decl,
> +                            &mut self.context,
> +                            self.custom_properties,
> -                                                                         self.default_values);
> +                            self.default_values);

nit: given you're fixing up the indentation here, let's move the paren and semicolon to the next line.

::: servo/components/style/properties/declaration_block.rs:667
(Diff revision 2)
>          )
>      }
> +
> +    /// Get the custom properties map in this declaration block along with
> +    /// inherited custom properties.
> +    pub fn custom_properties_with_inherited(

Let's call this, maybe, `cascade_custom_properties`? `custom_properties_with_inherited` doesn't mention the important fact that this actually _cascades_ the properties.

::: servo/components/style/properties/declaration_block.rs:672
(Diff revision 2)
> +    pub fn custom_properties_with_inherited(
> +        &self,
> +        inherited_custom_properties: &Option<Arc<::custom_properties::CustomPropertiesMap>>,
> +    ) -> Option<Arc<::custom_properties::CustomPropertiesMap>> {
> +        let mut custom_properties = None;
> +        let mut seen_custom = HashSet::new();

Please add a `FIXME`, or file a bug or an issue for this to use the `PrecomputedHasher`, as we do for everything else that uses `Atom`s in the Stylist. I'm assuming this involves changing pretty much the whole `custom_properties` module, right?

(Please CC and ni? me if you want, should be easy to fix)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:645
(Diff revision 2)
>                      % endif
>                      % endfor
>                  }
>              },
>              PropertyDeclaration::WithVariables(id, ref unparsed) => {
> -                let custom_props = context.style().custom_properties();
> +                let custom_properties = if let Some(block) = custom_properties_block {

So this is not great, I think. This means that we re-resolve the cascade for every single declaration with variable references.

Instead, would it make sense to pass `custom_properties` to this method directly?

We would compute it upfront just once if there are any variable declarations, and it simplifies a bit the definition of `AnimationValueIterator`, I think (you'd only need to store a `resolved_custom_properties` field, which you can compute in `AnimationValueIterator::new`).

It seems silly to put variable declarations and not variable references, so I doubt we'd end up doing more work anyway.

::: servo/ports/geckolib/glue.rs:3408
(Diff revision 2)
>  
>              let declarations = unsafe { &*property.mServoDeclarationBlock.mRawPtr.clone() };
>              let declarations = Locked::<PropertyDeclarationBlock>::as_arc(&declarations);
>              let guard = declarations.read_with(&guard);
> +            let iter = guard.to_animation_value_iter(
> +                &mut context, &default_values, custom_properties_block);

let's use block indentation, even if it takes a bit more space:

```
let iter = guard.to_animation_value_iter(
    &mut context,
    &default_values,
    &custom_properties_block,
);
```

::: servo/ports/geckolib/glue.rs:3452
(Diff revision 2)
>      let declarations = Locked::<PropertyDeclarationBlock>::as_arc(&declarations);
>      let guard = declarations.read_with(&guard);
> -    for (index, anim) in guard.to_animation_value_iter(&mut context, &default_values).enumerate() {
> +    let iter = guard.to_animation_value_iter(
> +        &mut context,
> +        &default_values,
> +        /* SMIL has no extra cutom properties */ None);

Ditto re indentation (you can make the comment a single-line comment after the `None,` that way, if you prefer, though no big deal).

::: servo/ports/geckolib/glue.rs:3495
(Diff revision 2)
>      match declarations.read_with(&guard).declaration_importance_iter().next() {
>          Some((decl, imp)) if imp == Importance::Normal => {
> -            let animation = AnimationValue::from_declaration(decl, &mut context, default_values);
> +            let animation = AnimationValue::from_declaration(
> +                decl,
> +                &mut context,
> +                /* No extra custom properties for devtools */ None,

ditto
Attachment #8911696 - Flags: review?(emilio) → review+
Thanks for the review!

(In reply to Emilio Cobos Álvarez [:emilio] from comment #29)
> ::: servo/components/style/properties/declaration_block.rs:154
> (Diff revision 2)
> >  /// Iterator for AnimationValue to be generated from PropertyDeclarationBlock.
> >  pub struct AnimationValueIterator<'a, 'cx, 'cx_a:'cx> {
> >      iter: DeclarationImportanceIterator<'a>,
> >      context: &'cx mut Context<'cx_a>,
> >      default_values: &'a ComputedValues,
> > +    custom_properties: Option<&'a PropertyDeclarationBlock>,
> 
> Let's call this `extra_custom_properties`, and document that it's a
> declaration block with the custom properties declared on a given keyframe
> (or such?).
> 
> (But, I think we may not even need this member, and we should resolve them
> upfront, read below)

Oh right, indeed. To make them resolving upfront, it means that we need the solution for the lifetime issue of CustomPropertiesMap, I am really happy that the your help on IRC was not wasted. :)
> ::: servo/components/style/properties/declaration_block.rs:667
> (Diff revision 2)
> >          )
> >      }
> > +
> > +    /// Get the custom properties map in this declaration block along with
> > +    /// inherited custom properties.
> > +    pub fn custom_properties_with_inherited(
> 
> Let's call this, maybe, `cascade_custom_properties`?
> `custom_properties_with_inherited` doesn't mention the important fact that
> this actually _cascades_ the properties.

Yep, I will rename this function to cascade_custom_properties_with_context for computed values in keyframes (since this function will take Context) and name another variant of this function for getKeyframes() to cascade_custom_properties.

> ::: servo/components/style/properties/declaration_block.rs:672
> (Diff revision 2)
> > +    pub fn custom_properties_with_inherited(
> > +        &self,
> > +        inherited_custom_properties: &Option<Arc<::custom_properties::CustomPropertiesMap>>,
> > +    ) -> Option<Arc<::custom_properties::CustomPropertiesMap>> {
> > +        let mut custom_properties = None;
> > +        let mut seen_custom = HashSet::new();
> 
> Please add a `FIXME`, or file a bug or an issue for this to use the
> `PrecomputedHasher`, as we do for everything else that uses `Atom`s in the
> Stylist. I'm assuming this involves changing pretty much the whole
> `custom_properties` module, right?
> (Please CC and ni? me if you want, should be easy to fix)

Ok, I see. If I understand correctly your comment, I think the easy fix is to use PrecomputedHashSet only for |seen| and the change for custom_properties means to rewrite the module with PrecoputedHash. Anyway I will simply leave FIXME comment here.


> ::: servo/components/style/properties/helpers/animated_properties.mako.rs:645
> (Diff revision 2)
> >                      % endif
> >                      % endfor
> >                  }
> >              },
> >              PropertyDeclaration::WithVariables(id, ref unparsed) => {
> > -                let custom_props = context.style().custom_properties();
> > +                let custom_properties = if let Some(block) = custom_properties_block {
> 
> So this is not great, I think. This means that we re-resolve the cascade for
> every single declaration with variable references.
> 
> Instead, would it make sense to pass `custom_properties` to this method
> directly?

I will do it. Thanks!
Comment on attachment 8911697 [details]
Bug 1402219 - Compute css variables with custom properties in keyframes for getKeyframes().

https://reviewboard.mozilla.org/r/183098/#review189006

I'm afraid that until Friday I only have time to do a shallow review, but I don't want to block this from landing. The behavior here looks fine but if Emilio has a chance to check the rust side that would be a good idea.

::: commit-message-87159:1
(Diff revision 2)
> +Bug 1402219 - Compute css variables with custom properties in keframes for getKeyframes(). r?birtles

nit: s/keframes/keyframes/

::: dom/animation/KeyframeEffectReadOnly.cpp:1261
(Diff revision 2)
>      }
>  
> +    RefPtr<RawServoDeclarationBlock> customProperties;
> +    // For CSS Animations in servo backend, custom properties in keyframe are
> +    // stored in a servo's declaration block. Find the declaration block
> +    // to resolve CSS variables in the keyframe.

We should probably mention that this is a temporary workaround until bug 1391537 is resolved.

::: dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html:147
(Diff revision 2)
>    to { transform: translate(var(--var-100px), 0) }
>  }
>  @keyframes anim-variables-shorthand {
>    to { margin: var(--var-100px) }
>  }
> +@keyframes anim-variables-in-keyframe {

maybe anim-custom-property-in-keyframe ?

::: dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html:722
(Diff revision 2)
> +    { offset: 1, computedOffset: 1, easing: "ease",
> +      color: "rgb(0, 255, 0)" },

Long-term I'm pretty certain we *don't* want to resolve these in getKeyframes(). Instead, we want the value of color here to be 'var(--end-color)'. But I guess that's a different bug (bug 1391537 where we make getKeyframes() for CSS animations do a better job of returning the specified values).

::: servo/components/style/properties/declaration_block.rs:596
(Diff revision 2)
>              Err(_longhand_or_custom) => {
>                  if self.declarations.len() == 1 {
>                      let declaration = &self.declarations[0];
> +                    let custom_properties = if let Some(cv) = computed_values {
> +                        // If there are extra custom properties for this declaration block,
> +                        // factor them into.

"...factor them in too." ?
Attachment #8911697 - Flags: review?(bbirtles) → review+
Comment on attachment 8911696 [details]
Bug 1402219 - Factor custom properties in keyframes into compute values in each keyframe.

https://reviewboard.mozilla.org/r/183096/#review189010

Deferring to Emilio's review here.

::: servo/ports/geckolib/glue.rs:3452
(Diff revision 2)
>      let declarations = Locked::<PropertyDeclarationBlock>::as_arc(&declarations);
>      let guard = declarations.read_with(&guard);
> -    for (index, anim) in guard.to_animation_value_iter(&mut context, &default_values).enumerate() {
> +    let iter = guard.to_animation_value_iter(
> +        &mut context,
> +        &default_values,
> +        /* SMIL has no extra cutom properties */ None);

Also s/cutom/custom/
Attachment #8911696 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles, busy with MDN roadshow) from comment #31)

> :::
> dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html:722
> (Diff revision 2)
> > +    { offset: 1, computedOffset: 1, easing: "ease",
> > +      color: "rgb(0, 255, 0)" },
> 
> Long-term I'm pretty certain we *don't* want to resolve these in
> getKeyframes(). Instead, we want the value of color here to be
> 'var(--end-color)'. But I guess that's a different bug (bug 1391537 where we
> make getKeyframes() for CSS animations do a better job of returning the
> specified values).

Thanks for clarifying.  Yeah, I was wondering the custom property values returned by getKeyframes() are confusing. (e.g it's different from script animation ones), we might handle them properly in bug 1403415.
Comment on attachment 8911696 [details]
Bug 1402219 - Factor custom properties in keyframes into compute values in each keyframe.

https://reviewboard.mozilla.org/r/183096/#review189026

As per comment 32, deferring to Emilio's review for this (meaning his review is sufficient).
Attachment #8911696 - Flags: review?(bbirtles)
Attached file Servo PR — 

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8911695 -
        Attachment is obsolete: true

    
    

    
  
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a469ba7cc3d
Check the length of returned keyframes of getKeyframes(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/fe949f75ab28
Factor custom properties in keyframes into compute values in each keyframe. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9510323f9f10
Compute css variables with custom properties in keyframes for getKeyframes(). r=birtles

    
    

    
  


  
Approval Request Comment
[Feature/Bug causing the regression]: Regressed by stylo
[User impact if declined]: User will see incorrect CSS animations for custom properties
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:  Auto revendor <https://hg.mozilla.org/mozilla-central/rev/ea46d757d7dd> and change sets in comment 44
[Is the change risky?]: Low
[Why is the change risky/not risky?]: This path does an additional process for custom properties so it should not affect other animations that have been working
[String changes made/needed]: None

        Attachment #8913164 -
        Flags: approval-mozilla-beta?

    
    

    
  
Comment on attachment 8913164 [details] [diff] [review]
Servo side changes for beta

Not super happy about the size of the patch but I guess this is important for webcompat purposes.
Taking it to 57b4

        Attachment #8913164 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
> [List of other uplifts needed for the feature/fix]:  Auto revendor
> <https://hg.mozilla.org/mozilla-central/rev/ea46d757d7dd> and change sets in
> comment 44

I think https://hg.mozilla.org/mozilla-central/rev/cffc9b5b177e (ea46d757d7dd’s parent) is also needed: it’s the changes in servo/ synchronized from github.

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #45)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Hiroyuki Ikezoe's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: