1514342 - ToAnimatedValue should just move/clone self if it is not animatable

Status: NEW

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

Description

[Boris Chiou [:boris]]

While adding ToAnimatedValue for ShapeSource, I found we didn't handle "#[animation(error)]" properly style_derive/to_animated_value.rs.

For example [1]:

pub enum ShapeSource<BasicShape, ReferenceBox, ImageOrUrl> {
    #[animation(error)]
    ImageOrUrl(ImageOrUrl),
    Shape(BasicShape, Option<ReferenceBox>),
    #[animation(error)]
    Box(ReferenceBox),
    #[css(function)]
    Path(Path),
    #[animation(error)]
    None,
}

If I want to derive "ToAnimatedValue", we should just move/copy the arms of "ImageOrUrl(...)" and "Box(...)" because they are not animatable. ToAnimatedZero and ComputeSquaredDistance handle this properly, but ToAnimatedValue doesn't.

What I expected is something like this:

impl<B, R, I> ToAnimatedValue for ShapeSource<B, R, I> {
    type AnimatedValue = ShapeSource<B::AnimatedValue,
                                     R::AnimatedValue,
                                     I::AnimatedValue>;
    #[inline]
    fn to_animated_value(self) -> Self {
        match self {
            ImageOrUrl(_) => ImageOrUrl(_),  // Just move it.
            Shape(b, r) => {
                Shape(b.to_animated_value(),
                      r.to_animated_value())
            },
            ...
        }
    }

    #[inline]
    fn from_animated_value(animated: Self::AnimatedValue) -> Self {
        match animated {
            ImageOrUrl(_) => ImageOrUrl(_),  // Just move it.
            Shape(b, r) => {
                Shape(B::from_animated_value(b),
                      R::from_animated_value(r))
            },
            ...
        }
    }
}

[1] https://searchfox.org/mozilla-central/rev/49e78df13e7a505827a3a86daae9efdf827133c6/servo/components/style/values/generics/basic_shape.rs#58-74

Comment 1

[Emilio Cobos Álvarez [:emilio]]

So to be clear, the behavior now is that we call (or try to?) `to_animated_value`, right?

Comment 2

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)
> So to be clear, the behavior now is that we call (or try to?)
> `to_animated_value`, right?

Yes. There are two functions in this trait:
1. to_animated_value(self)
2. from_animated_value(animated: Self::AnimatedValue)

We need this trait to convert the computed value into an intermediate value for interpolation (for some properties, e.g. color)