Bug 41188 – Animations should not require from/to keyframes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dean Jackson]

Currently animations without a from/0 or to/100 keyframe are ignored. I propose that the style system should fill in those missing keyframes with the element style. 

This would allow animations to be more generic. Eg. you could apply an animation to objects with different initial settings and not have to create keyframes on the fly, or have the objects jump.

Open question though, should you be required to specify at least one keyframe? I think so. It could be any keyframe, but there needs to be one. This would mean that an empty list of keyframes is still an invalid animation.

[Dean Jackson]

Created attachment 59760 [details]
patch

proposed patch with testcases

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:1460:  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 8 files


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

[Dean Jackson]

I'm going to ignore the style bot here. It makes it more clear that we are checking against the 0% keyframe, rather than just the fact that a value exists. It also aligns well with the similar code below that is testing for the 1 or 100% key value.

[Simon Fraser (smfr)]

Is this patch fixing both this and bug 41209?

[Simon Fraser (smfr)]

Comment on attachment 59760 [details]
patch

WebCore/css/CSSStyleSelector.cpp:1466
 +      if (initialListSize > 0 && (list.endKeyframes()-1)->key() != 1) {
Need spaces around the minus here.

LayoutTests/ChangeLog:8
 +          * animations/fill-mode-missing-from-to-keyframes-expected.txt: Added.
 +          * animations/fill-mode-missing-from-to-keyframes.html: Added.

Did you mean to include these?WebCore/css/CSSStyleSelector.h:115
 +          void generateKeyframeStyle(const RenderStyle*, const WebKitCSSKeyframeRule*, KeyframeList&);

This should be private.

Does this patch do the right thing if there's a transition running on the property that the animation is going to affect, and the 0% keyframe is missing?

[Dean Jackson]

(In reply to comment #4)
> Is this patch fixing both this and bug 41209?

Nope - it is only fixing this.

And yes, I should leave the fill mode test out of this patch, otherwise I'd have to skip it or check in intentionally failing results.

[Dean Jackson]

(In reply to comment #5)
 
> Does this patch do the right thing if there's a transition running on the property that the animation is going to affect, and the 0% keyframe is missing?

It does!!! attaching a manual test.

Well, at least I think it does the right thing. It uses the final style of the transition. eg. if you are in the middle of a transition from left: 100 to left: 200, then apply an animation, the 0% of the animation will start from 200. Do you expect it to use the transitioning value? If so that's slightly weird, because we say that animations trump the transition, so as far as the animation is concerned, the value of left is 200.

[Dean Jackson]

Created attachment 59806 [details]
updated patch with review fixes, new testcase for HW animations, and a manual test

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:1460:  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 9 files


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

[Simon Fraser (smfr)]

Comment on attachment 59806 [details]
updated patch with review fixes, new testcase for HW animations, and a manual test

WebCore/ChangeLog:12
 +          Tests: animations/fill-mode-missing-from-to-keyframes.html
Wrong test listed here, and transform test is missing.

I'd prefer that CSSStyleSelector::generateKeyframeStyle() not do the insertion into the KeyframeList. I think it should just return a RenderStyle. You could make another method that sets the RenderStyle for the appropriate keys.

The manual test should say what is being tested, and reference this bug.

r- to refactor use of generateKeyframeStyle().

[Dean Jackson]

Created attachment 59809 [details]
updated patch with refactored ::styleForKeyframe

Refactored into ::styleForKeyframe, which returns a RefPtr. Now the keyframe code gets that and assigns it in, releasing when it can.

Also updated the manual test.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:1464:  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 9 files


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

[Simon Fraser (smfr)]

Comment on attachment 59809 [details]
updated patch with refactored ::styleForKeyframe

> +    RefPtr<RenderStyle> keyframeStyle;
>      
>      // Construct and populate the style for each keyframe
>      for (unsigned i = 0; i < rule->length(); ++i) {

You should declare keyframeStyle inside the loop.

> +        keyframeStyle.release();
> +        

Extra blank line here.

r=me

[Dean Jackson]

commited r61933