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.
Created attachment 59760 [details] patch proposed patch with testcases
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.
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.
Is this patch fixing both this and bug 41209?
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?
(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.
(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.
Created attachment 59806 [details] updated patch with review fixes, new testcase for HW animations, and a manual test
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.
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().
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.
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.
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
commited r61933