1340961 - stylo: handle missing final keyframe with !important on CSS Animations properly

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Boris Chiou [:boris]]

For this example [1]:

@keyframes important2
{
   from { opacity: 0.5;
          transform: translate(100px); }
   to   { opacity: 0.2 !important; /* ignored */
          transform: translate(50px); }
}

[1] http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/layout/style/test/test_animations_omta.html#140-145

If there is an !important rule in @keyframes, we ignore it. However, we don't handle this case properly. While building the AnimationPropertySegment in BuildSegmentsFromValueEntries() for opacity, we create a missing final segment as [0.2, nullptr]. Its mValue with offset 1.0 is a null RawServoAnimationValue, so we cannot do interpolation for that.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

A problem I (and other guys) have noticed is that we don't check all properties [1] in @keyframes.  But this is not the case.

[1] https://hg.mozilla.org/mozilla-central/file/16effd5b21ab/servo/components/style/keyframes.rs#l251

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Urgh!  In this case we inject values for the missing properties in KeyframesStepValue::Declarations block if it's the initial or finial keyframe.

https://hg.mozilla.org/mozilla-central/file/16effd5b21ab/servo/ports/geckolib/glue.rs#l1256

Comment 3

[Hiroyuki Ikezoe (:hiro)]

we *have to* inject.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

OK. This is highly related to bug 1339332. Once bug 1339332 is fixed, gecko does not fill missing keyframes for CSS animations at all. That means a bunch of our automation test cases will be modified, So, stylo should follow it. But a problem is that bug 1339332 depends additive animation (actually missing keyframe handling), but yes, stylo does not have the one.. it will be tough.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

So, we don't need to inject the missing values into keyframes at all, moreover we should drop KeyframesStepValue::ComputedValues case [1] there.  

[1] https://hg.mozilla.org/mozilla-central/file/16effd5b21ab/servo/ports/geckolib/glue.rs#l1236

Comment 6

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> OK. This is highly related to bug 1339332. Once bug 1339332 is fixed, gecko
> does not fill missing keyframes for CSS animations at all.

The plan there is to fill in the missing keyframes, but it simply sets the property values to null.

So

@keyframes abc {
  to { opacity: 1 }
}

Becomes:

[
 { offset: 0, opacity: null, easing: 'ease' },
 { offset: 1, opacity 1 }
]

It's because of the easing that we can't just omit the keyframe.

(When you call getKeyframes() from Javascript, however, we will resolve the base style and replace 'null' with that value.)

Comment 7

[Hiroyuki Ikezoe (:hiro)]

OK. Thanks. That's really good to know. I misunderstood that bug.
Then in this bug, we do *inject* mProperty and mServoDeclarationBlock with computed values, and later we will set mServoDeclarationBlock to null there once we fix bug 1311257.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch

This will partially conflict with a patch for bug 1338927.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

The patch does not cover all of missing value case. One example:

@keyframes anim {
  from { opacity: 0; transform: none; }
  to { transform: none; }
}

For this case we should fix get_animated_properties as I commented in comment 1.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: get all animated properties in all keyframes

The servo side fix is something like this. I will send a PR with tests to servo.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc797275e38d18925f87d79518fc0a49376c19f9
I've decided to post both of patches in this bug.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1340961 - Part 3: Fill computed property values where the propery is missing in initial and final keyframes.

@keyframes anim {
  from { opacity: 0; transform: none; }
  to { transform: none; }
}

In above case, we have to add opacity property and value in the 'to' keyframe.

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1340961 - Part 2: Get all animated properties in *all* keyframes.

@keyframes anim {
  from { transform: none; }
  to { opacity: 0; transform: none; }
}

In above case, we have to add opacity property and value in the 'from' keyframe.

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

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Emilio, could you please take a look these patches?

We have to handle following keyframes.

@keyframes no-opacity-in-final-keyframe {
  from { opacity: 0; transform: none; }
  to { transform: none; }
}

@keyframes no-opacity-in-initial-keyframe {
  from { transform: none; }
  to { opacity: 0; transform: none; }
}

Thanks!

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839328 [details]
Bug 1340961 - Part 3: Fill computed property values where the propery is missing in initial and final keyframes.

https://reviewboard.mozilla.org/r/113998/#review115552

::: servo/ports/geckolib/glue.rs:1492
(Diff revision 1)
>                             .iter()
>                             .filter(|&&(ref declaration, _)| {
>                                 declaration.is_animatable()
>                             });
> +
> +                  let mut properties_on_this_keyframe: HashSet<TransitionProperty> = HashSet::new();

We could try to use `PropertyBitfield` for these.

::: servo/ports/geckolib/glue.rs:1514
(Diff revision 1)
> +                  }
> +
> +                  // Append missing property values in the initial or the finial keyframes.
> +                  if step.start_percentage.0 == 0. ||
> +                     step.start_percentage.0 == 1. {
> +                      for (index, property) in animation.properties_changed.iter().enumerate() {

I'm unsure how this works? `properties_changed` only looks at the first keyframe, right? Shouldn't we fix that too?

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839329 [details]
Bug 1340961 - Part 2: Get all animated properties in *all* keyframes.

https://reviewboard.mozilla.org/r/114000/#review115554

Ok, should've checked this patch too.

r=me with the patches either squashed or reordered, and using `PropertyBitfield`. We can add helpers to either `PropertyBitfield` or `TransitionProperty` to make it easier.

Thanks!

::: servo/components/style/keyframes.rs:248
(Diff revision 1)
>      /// The properties that change in this animation.
>      pub properties_changed: Vec<TransitionProperty>,
>  }
>  
> -/// Get all the animated properties in a keyframes animation. Note that it's not
> -/// defined what happens when a property is not on a keyframe, so we only peek
> +/// Get all the animated properties in a keyframes animation.
> +fn get_animated_properties(keyframes: &[Arc<RwLock<Keyframe>>]) -> Vec<TransitionProperty> {

Oh, of course this was the second patch, heh :)

::: servo/components/style/keyframes.rs:258
(Diff revision 1)
> +        let keyframe = keyframe.read();
> -    for &(ref declaration, importance) in keyframe.block.read().declarations.iter() {
> +        for &(ref declaration, importance) in keyframe.block.read().declarations.iter() {
> -        assert!(!importance.important());
> +            assert!(!importance.important());
>  
> -        if let Some(property) = TransitionProperty::from_declaration(declaration) {
> +            if let Some(property) = TransitionProperty::from_declaration(declaration) {
> +                if !ret.contains(&property) {

This `contains` check is linear, we should use a `PropertyBitfield` ideally, so it's faster.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839328 [details]
Bug 1340961 - Part 3: Fill computed property values where the propery is missing in initial and final keyframes.

https://reviewboard.mozilla.org/r/113998/#review115556

r=me with the comments in the other patch addressed, thanks for doing this!

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Thank you Emilio, I will add a patch to add a helper for PropertyBitFiled as a part 1 patch and revise order of the rest of patches. 

Thanks!

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1340961 - Part 1: Make PropertyFiledBit usable for TransitionProperty.

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

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8839329 [details]
Bug 1340961 - Part 2: Get all animated properties in *all* keyframes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114000/diff/1-2/

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8839328 [details]
Bug 1340961 - Part 3: Fill computed property values where the propery is missing in initial and final keyframes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113998/diff/1-2/

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839752 [details]
Bug 1340961 - Part 1: Make PropertyFiledBit usable for TransitionProperty.

https://reviewboard.mozilla.org/r/114320/#review115856

Nice! thanks for doing this. r=me with the following nits :)

::: servo/components/style/properties/properties.mako.rs:191
(Diff revision 1)
>      use logical_geometry::WritingMode;
> +    use properties::animated_properties::TransitionProperty;
>  
>      /// A bitfield for all longhand properties, in order to quickly test whether
>      /// we've seen one of them.
> +    /// Note: The last bit is for TransitionProperty::All.

Do we need this bit? As far as I know we can't get `TransitionProperty::All` with keyframes.

I think `All` is enough of a special case that is not worth to reserve a bit for it (I can't think of a use case where it would be useful).

Let's use `unreachable!("Tried to {get,set} TransitionProperty::All in a PropertyBitfield")` in the relevant branches instead, and note in the documentation for `get_transition_property` and `has_transition_property` that this method panics if `TransitionProperty::All` is given to them.

::: servo/components/style/properties/properties.mako.rs:246
(Diff revision 1)
>                  }
>              % endif
>          % endfor
> +
> +        /// Set the corresponding bit of TransitionProperty.
> +        pub fn set_property(&mut self, property: &TransitionProperty) {

I'd just rename these to `set_transition_property` and `has_transition_property`, since I'd expect `{set,has}_property` to operate on `LonghandId`s.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)

> ::: servo/components/style/properties/properties.mako.rs:191
> (Diff revision 1)
> >      use logical_geometry::WritingMode;
> > +    use properties::animated_properties::TransitionProperty;
> >  
> >      /// A bitfield for all longhand properties, in order to quickly test whether
> >      /// we've seen one of them.
> > +    /// Note: The last bit is for TransitionProperty::All.
> 
> Do we need this bit? As far as I know we can't get `TransitionProperty::All`
> with keyframes.
> 
> I think `All` is enough of a special case that is not worth to reserve a bit
> for it (I can't think of a use case where it would be useful).

That's right.  I was actually thinking the All case should be panic!. 

> Let's use `unreachable!("Tried to {get,set} TransitionProperty::All in a
> PropertyBitfield")` in the relevant branches instead, and note in the
> documentation for `get_transition_property` and `has_transition_property`
> that this method panics if `TransitionProperty::All` is given to them.

I did not know unreachable!.  Thanks!

> ::: servo/components/style/properties/properties.mako.rs:246
> (Diff revision 1)
> >                  }
> >              % endif
> >          % endfor
> > +
> > +        /// Set the corresponding bit of TransitionProperty.
> > +        pub fn set_property(&mut self, property: &TransitionProperty) {
> 
> I'd just rename these to `set_transition_property` and
> `has_transition_property`, since I'd expect `{set,has}_property` to operate
> on `LonghandId`s.

That's sounds a good idea.  Oops.  I just renamed it.  Unfortunately we have already set_transition_property() for short hand of 'transition'...

Comment 24

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > I'd just rename these to `set_transition_property` and
> > `has_transition_property`, since I'd expect `{set,has}_property` to operate
> > on `LonghandId`s.
> 
> That's sounds a good idea.  Oops.  I just renamed it.  Unfortunately we have
> already set_transition_property() for short hand of 'transition'...

Wrong. This was not a short hand. transition-property actually.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

I've added _bit suffix to the function names.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Servo PR. https://github.com/servo/servo/pull/15683
We don't need to land any patches in autoland in this bug.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/mozilla-central/rev/887ae49f0e2f