Summary: | When properties are missing from animation keyframes, interpolate between those keyframes that specify them | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Component: | CSS | Assignee: | 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
Simon Fraser (smfr)
2010-06-17 12:01: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) ? 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. 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. 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. 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. 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; } Created attachment 65070 [details]
Patch for some initial cleanup
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. Created attachment 65207 [details]
Patch for some initial cleanup
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.
(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 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!
(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 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 on attachment 65207 [details] Patch for some initial cleanup http://trac.webkit.org/changeset/66310 http://trac.webkit.org/changeset/66310 might have broken Qt Linux Release Created attachment 65858 [details]
Patch
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 |