Bug 40794

Summary: When properties are missing from animation keyframes, interpolate between those keyframes that specify them
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarrin, dino, eric, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Bug Depends on: 41188    
Bug Blocks:    
Attachments:
Description Flags
Patch for some initial cleanup
none
Patch for some initial cleanup
none
Patch darin: review+

Description Simon Fraser (smfr) 2010-06-17 12:01:15 PDT
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.
Comment 1 Simon Fraser (smfr) 2010-06-17 12:01:37 PDT
<rdar://problem/8104038>
Comment 2 Dean Jackson 2010-06-24 17:42:15 PDT
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) ?
Comment 3 Simon Fraser (smfr) 2010-06-24 17:57:44 PDT
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.
Comment 4 Simon Fraser (smfr) 2010-06-24 18:00:12 PDT
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.
Comment 5 Dean Jackson 2010-06-24 18:19:17 PDT
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.
Comment 6 Simon Fraser (smfr) 2010-06-24 18:22:12 PDT
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.
Comment 7 Simon Fraser (smfr) 2010-08-22 15:18:26 PDT
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;
}
Comment 8 Simon Fraser (smfr) 2010-08-22 19:13:43 PDT
Created attachment 65070 [details]
Patch for some initial cleanup
Comment 9 WebKit Review Bot 2010-08-22 19:15:53 PDT
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 10 mitz 2010-08-22 19:31:01 PDT
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.
Comment 11 Simon Fraser (smfr) 2010-08-23 20:08:57 PDT
Created attachment 65207 [details]
Patch for some initial cleanup
Comment 12 WebKit Review Bot 2010-08-23 20:14:10 PDT
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.
Comment 13 Dean Jackson 2010-08-24 12:38:06 PDT
(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?
Comment 14 Dean Jackson 2010-08-24 12:41:04 PDT
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!
Comment 15 Simon Fraser (smfr) 2010-08-24 13:04:57 PDT
(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.
Comment 16 Sam Weinig 2010-08-25 06:13:03 PDT
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.
Comment 17 Simon Fraser (smfr) 2010-08-28 10:39:44 PDT
Comment on attachment 65207 [details]
Patch for some initial cleanup

http://trac.webkit.org/changeset/66310
Comment 18 WebKit Review Bot 2010-08-28 11:08:00 PDT
http://trac.webkit.org/changeset/66310 might have broken Qt Linux Release
Comment 19 Simon Fraser (smfr) 2010-08-29 11:40:50 PDT
Created attachment 65858 [details]
Patch
Comment 20 Simon Fraser (smfr) 2010-08-29 16:17:10 PDT
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