Bug 40794 – When properties are missing from animation keyframes, interpolate between those keyframes that specify them

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

If you have keyframes like:

{
  from { left: 0; opacity: 1; }
  50% { opacity: 0.8; }
  to { left: 200px; opacity: 0.2; }
}
then the 50% keyframe will use the current value of left on the element being animated. This is not very useful; it would be much better to just interpolate 'left' between the first and last keyframes.

[Simon Fraser (smfr)]

<rdar://problem/8104038>

[Dean Jackson]

I generally support this, but have a few questions.

Do you always have to list every property you are animating in the from and to keyframes? If you don't, what value do you use? The value from the element's style?

I expect you want the property to animate through the missing keyframes as if it had been specified there with the value calculated from the animation? For example:

from { left: 0; opacity: 1; }
20% { opacity: 0.8; }
to {left: 200px; opacity: 0.2; }

... would calculate a left value of 40px at the 20% keyframe.

While, this:

from { left: 0; opacity: 1; }
20% { opacity: 0.8; }
40% {left: 40px; opacity: 0.4 }
to {left: 200px; opacity: 0.2; }

.. would provide a left value of 20px at the 20% keyframe.

And also, that the timing function would not matter. For example:


from { left: 0; opacity: 1; timing-function: ease-in; }
20% { opacity: 0.8; timing-function: ease-in; }
to {left: 200px; opacity: 0.2; }

... would still give 40px at 20%. The timing function only applies between keyframes, even if the property value is not specified. In fact, there is always a linear calculation for filling in the blank values?

Lastly, what about complex properties such as transform?

from { transform: rotate(10deg) translate(50px); opacity: 1; }
50% { opacity: 0.8; }
to { transform: rotate(50deg) translate(90px); opacity: 0.2; }

Is the value at 50% a matrix, or the list of functions rotate(30deg) translate(70px) ?

[Simon Fraser (smfr)]

I think this:

  @keyframes combined {
    0% {
      left: 0;
      top: 0;
      timing-function: ease-in;
    }
    
    50% {
      top: 50px;
    }

    100% {
      top: 200px;
      left: 200px;
    }
  }

should exactly equivalent to two separate animations:

  @keyframes top {
    0% {
      top: 0;
      timing-function: ease-in;
    }
    
    50% {
      top: 50px;
    }

    100% {
      top: 200px;
    }
  }


  @keyframes left {
    0% {
      left: 0;
      timing-function: ease-in;
    }
    
    100% {
      left: 200px;
    }
  }

so you can't just linearly interpolate the values of missing properties; the timing functions apply. I don't see that transforms need any special treatment.

[Simon Fraser (smfr)]

So when combining this with bug 41188, we'll have to say something like:
* if the 0% or 100% keyframes are missing, or if a property is not specified in these keyframes, then the initial/final value of that property is taken from the current style of the element.
* If a property is missing from an intermediate keyframe, then that property is interpolated as if that keyframe does not exist.

[Dean Jackson]

OK. So you think the keyframe should not exist, and I assumed that it should be filled in.

I think your approach is fine. But I sort of wonder why we are doing this if, as you point out, the functionality is already there - just write another animation.

[Simon Fraser (smfr)]

Because writing another animation is verbose, and now you have to keep two animation in sync. I would have found this really useful when doing my gallery stuff.

[Simon Fraser (smfr)]

We also need to consider repeating animations. How does 'top' animate with the following?

@keyframes move {
from { left: 0; }
50% {
  left: 50px;
  top: 100px;
 }
to { left: 100px; }
}

.box {
  top: 50px;
  animation: move 2s 2 linear alternate;
}

[Simon Fraser (smfr)]

Created attachment 65070 [details]
Patch for some initial cleanup

[WebKit Review Bot]

Attachment 65070 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:1470:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 65070 [details]
Patch for some initial cleanup

> +    const KeyframeValue& at(size_t index) const { return m_keyframes[index]; }

Please add and use operator[]; .at() is not commonly used. Vector also provides first() and last() which are preferred to [0] and [….size() - 1].

> +    const Vector<KeyframeValue>& keyframes() const { return m_keyframes; }

This doesn’t seem to be used. Perhaps you should just use this instead of covering Vector methods.

[Simon Fraser (smfr)]

Created attachment 65207 [details]
Patch for some initial cleanup

[WebKit Review Bot]

Attachment 65207 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:1470:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

(In reply to comment #7)
> We also need to consider repeating animations. How does 'top' animate with the following?
> 
> @keyframes move {
> from { left: 0; }
> 50% {
>   left: 50px;
>   top: 100px;
>  }
> to { left: 100px; }
> }
> 
> .box {
>   top: 50px;
>   animation: move 2s 2 linear alternate;
> }

The 0% and 100% keyframes with top:50px will be autogenerated, so that means this animation will not change from the existing behaviour. Right?

[Dean Jackson]

Comment on attachment 65207 [details]
Patch for some initial cleanup


WebCore/page/animation/KeyframeAnimation.cpp:85
 +          const KeyframeValue& currKeyframe = m_keyframes[i];

I know this is super nitpicky, but I think this should be currentKeyframe. We use full words elsewhere around this code.

Otherwise I'm very glad to see the end of iterators!

[Simon Fraser (smfr)]

(In reply to comment #13)
> (In reply to comment #7)
> > We also need to consider repeating animations. How does 'top' animate with the following?
> > 
> > @keyframes move {
> > from { left: 0; }
> > 50% {
> >   left: 50px;
> >   top: 100px;
> >  }
> > to { left: 100px; }
> > }
> > 
> > .box {
> >   top: 50px;
> >   animation: move 2s 2 linear alternate;
> > }
> 
> The 0% and 100% keyframes with top:50px will be autogenerated, so that means this animation will not change from the existing behaviour. Right?

I think what should happen is that the 0% and 100% keyframes should only apply at the ends of repeating animation, so "internal" iterations would interpolate over these. It's a little more complex to spec, but I think could allow useful behavior.

[Sam Weinig]

Comment on attachment 65207 [details]
Patch for some initial cleanup

>      double offset = 0;
> -    Vector<KeyframeValue>::const_iterator endKeyframes = m_keyframes.endKeyframes();
> -    for (Vector<KeyframeValue>::const_iterator it = m_keyframes.beginKeyframes(); it != endKeyframes; ++it) {
> -        if (t < it->key()) {
> +    size_t numKeyframes = m_keyframes.size();
> +    for (size_t i = 0; i < numKeyframes; ++i) {
> +        const KeyframeValue& currKeyframe = m_keyframes[i];

I don't think there is a really compelling reason to continue using curr rather than current here.

Otherwise, r=me.

[Simon Fraser (smfr)]

Comment on attachment 65207 [details]
Patch for some initial cleanup

http://trac.webkit.org/changeset/66310

[WebKit Review Bot]

http://trac.webkit.org/changeset/66310 might have broken Qt Linux Release

[Simon Fraser (smfr)]

Created attachment 65858 [details]
Patch

[Simon Fraser (smfr)]

Note that I didn't do any special behavior for the 'to' and 'from' keyframes. We can do that later if we think it's appropriate.

http://trac.webkit.org/changeset/66339