1336668 - stylo: implement discrete type animation

Status: RESOLVED FIXED

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

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Implement discrete type animations (just implemented align-items)

For stylo, to make a property animatable we need to implement clone_xx() and Intepolate trait for the property.  But with this patch, we don't need to implement Interpolate trait respectively.

So rest of work should be easy.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Created attachment 8854308 [details] [diff] [review]
> Implement discrete type animations (just implemented align-items)
> 
> For stylo, to make a property animatable we need to implement clone_xx() and
> Intepolate trait for the property.  But with this patch, we don't need to
> implement Interpolate trait respectively.

To be precise, we don't need to implement Interpolate trait for discrete type animation properties.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Implement discrete type animations (just implemented align-items)

I did forget to set animatable flag True, and the previous patch could not be compiled.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

In attachment 8854366 [details] [diff] [review], I added a new flag to represent that the property is animatable as discrete, but after some thought 
I think it would be better to make current 'animatable' flag tri-state. E.g.

animatable=None
animatable="Normal"
animatable="Discrete"

But "Normal" is somewhat blurry. Brian, any ideas?

Note for logical properties, there is already a flag for it, so we can distinguish it.

https://hg.mozilla.org/mozilla-central/file/867df9483d5a/servo/components/style/properties/data.py#l99

Comment 5

[Brian Birtles (:birtles)]

Why tri-state? Why isn't discrete just another type of animation?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #5)
> Why tri-state? Why isn't discrete just another type of animation?

In Servo, we don't have animation-type flag for each property just like Gecko, Servo just implements Interpolate trait for each *type*, e.g. 'impl Interpolate for LengthOrPercentage', etc.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > Why tri-state? Why isn't discrete just another type of animation?
> 
> In Servo, we don't have animation-type flag for each property just like
> Gecko, Servo just implements Interpolate trait for each *type*, e.g. 'impl
> Interpolate for LengthOrPercentage', etc.

But for discrete type properties, we don't need to implement Interpolate at all, i.e skip calling Interpolate::interpolate() method respectively.  This is my intention in attachment 8854366 [details] [diff] [review].

Comment 8

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > Why tri-state? Why isn't discrete just another type of animation?
> 
> In Servo, we don't have animation-type flag for each property just like
> Gecko, Servo just implements Interpolate trait for each *type*, e.g. 'impl
> Interpolate for LengthOrPercentage', etc.

So Servo conflates the CSS type with animation type. I think we'll need to fix that at some point (in effect, that's what you're proposing, just on a limited scale) but for now it's probably ok.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > (In reply to Brian Birtles (:birtles) from comment #5)
> > > Why tri-state? Why isn't discrete just another type of animation?
> > 
> > In Servo, we don't have animation-type flag for each property just like
> > Gecko, Servo just implements Interpolate trait for each *type*, e.g. 'impl
> > Interpolate for LengthOrPercentage', etc.
> 
> So Servo conflates the CSS type with animation type. I think we'll need to
> fix that at some point (in effect, that's what you're proposing, just on a
> limited scale) but for now it's probably ok.

Thanks, then I will rename 'animatable' variable to 'animation-type' in this bug, and just make it represent None, 'Normal' or 'Discrete' for now.  Later we will add more types there.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1336668 - Rename animatable to animation_type.

animation_type takes following values:

animation_type="none" for non-animatable properties
animation_type="normal" for animatable properties
animation_type="discrete" for animatable but discrete type of animations

We use string value to distinguish the case where no animation_type is
specified.
animation_type="discrete" will be used in a subsequent patch to make a property
animatable as discrete type.

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

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1336668 - Add a tweak to avoid calling interpolate() if animation_type is discrete.

For discrete type of animations, we just need to return 'from' value if
progress is less than 0.5 and otherwise return 'to' value.

https://w3c.github.io/web-animations/#discrete-animation-type

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1336668 - Make align-items animatable as discrete type.

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8855177 [details]
Bug 1336668 - Rename animatable to animation_type.

https://reviewboard.mozilla.org/r/127062/#review129864

::: servo/components/style/properties/data.py:138
(Diff revision 1)
>          self.allowed_in_keyframe_block = allowed_in_keyframe_block \
>              and allowed_in_keyframe_block != "False"
>  
>          # This is done like this since just a plain bool argument seemed like
>          # really random.
> -        if animatable is None:
> +        if animation_type is None:

Let's ensure here that animation_type is one of the expected values.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8855178 [details]
Bug 1336668 - Add a tweak to avoid calling interpolate() if animation_type is discrete.

https://reviewboard.mozilla.org/r/127064/#review129866

::: commit-message-c04df:1
(Diff revision 1)
> +Bug 1336668 - Add a tweak to avoid calling interpolte() if animation_type is discrete. r?emilio

nit: Typo in the commit message (`interpolate` instead of `interpolte`).

::: commit-message-c04df:6
(Diff revision 1)
> +Bug 1336668 - Add a tweak to avoid calling interpolte() if animation_type is discrete. r?emilio
> +
> +For discrete type of animations, we just need to return 'from' value if
> +progress is less than 0.5 and otherwise return 'to' value.
> +
> +https://w3c.github.io/web-animations/#discrete-animation-type

Let's add a link to the spec also in the code (as a comment near the `if prop.animation_type == "discrete"`.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:222
(Diff revision 1)
>      /// animation at `progress`.
>      pub fn update(&self, style: &mut ComputedValues, progress: f64) {
>          match *self {
>              % for prop in data.longhands:
>                  % if prop.animatable:
> +                    % if prop.animation_type == "discrete":

nit: Let's move the `if prop.animation_type` inside the `AnimatedProperty::..` branch:

```
AnimatedProperty::${prop.camel_case}(ref from, ref to) => {
    % if prop.animation_type == "discrete":
         let value = ...;
    % else:
         if let Ok(value) = ...
    % endif
}
```

Also, we could share the mutate call, like so:

```
% if prop.animation_type == "discrete":
    let value = ...;
% else:
    let value = match from.interpolate(to, progress) {
        Ok(value) => value,
        Err(()) => return,
    };
% endif
    style.mutate_${prop...}.set_${prop.ident}(value);
```

wdyt?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8855178 [details]
Bug 1336668 - Add a tweak to avoid calling interpolate() if animation_type is discrete.

https://reviewboard.mozilla.org/r/127064/#review129868

::: servo/components/style/properties/helpers/animated_properties.mako.rs:404
(Diff revision 1)
>      fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
>          match (self, other) {
>              % for prop in data.longhands:
>                  % if prop.animatable:
> +                    % if prop.animation_type == "discrete":
> +                        (&AnimationValue::${prop.camel_case}(ref from),

Oh, also, same recommendation about moving the if inside the branches here.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8855179 [details]
Bug 1336668 - Make align-items animatable as discrete type.

https://reviewboard.mozilla.org/r/127066/#review129870

Looks good, is this tested?

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> ::: servo/components/style/properties/helpers/animated_properties.mako.rs:222
> (Diff revision 1)
> >      /// animation at `progress`.
> >      pub fn update(&self, style: &mut ComputedValues, progress: f64) {
> >          match *self {
> >              % for prop in data.longhands:
> >                  % if prop.animatable:
> > +                    % if prop.animation_type == "discrete":
> 
> nit: Let's move the `if prop.animation_type` inside the
> `AnimatedProperty::..` branch:
> 
> ```
> AnimatedProperty::${prop.camel_case}(ref from, ref to) => {
>     % if prop.animation_type == "discrete":
>          let value = ...;
>     % else:
>          if let Ok(value) = ...
>     % endif
> }
> ```
> 
> Also, we could share the mutate call, like so:
> 
> ```
> % if prop.animation_type == "discrete":
>     let value = ...;
> % else:
>     let value = match from.interpolate(to, progress) {
>         Ok(value) => value,
>         Err(()) => return,
>     };
> % endif
>     style.mutate_${prop...}.set_${prop.ident}(value);
> ```


Neat! Thanks! I will do it.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> Comment on attachment 8855179 [details]
> Bug 1336668 - Make align-items animatable as discrete type.
> 
> https://reviewboard.mozilla.org/r/127066/#review129870
> 
> Looks good, is this tested?

We have a test case in web-platform-tests.
https://hg.mozilla.org/mozilla-central/file/3c68d659c2b7/testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js#l10

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8855177 [details]
Bug 1336668 - Rename animatable to animation_type.

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

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8855178 [details]
Bug 1336668 - Add a tweak to avoid calling interpolate() if animation_type is discrete.

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8855179 [details]
Bug 1336668 - Make align-items animatable as discrete type.

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

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8855177 [details]
Bug 1336668 - Rename animatable to animation_type.

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

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8855178 [details]
Bug 1336668 - Add a tweak to avoid calling interpolate() if animation_type is discrete.

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

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8855179 [details]
Bug 1336668 - Make align-items animatable as discrete type.

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

Comment 25

[Brian Birtles (:birtles)]

Comment on attachment 8855177 [details]
Bug 1336668 - Rename animatable to animation_type.

https://reviewboard.mozilla.org/r/127062/#review130100

Looks good.

("normal" seems not to explain what is happening, however. But "interpolable" doesn't seem right either since, for example, "visibility" is neither "discrete" nor "interpolable". So let's just stick with "normal" for now since I don't have any better suggestions.)

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Thanks for the review!

https://github.com/servo/servo/pull/16292

Comment 27

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/autoland/rev/3315299058de