RESOLVED FIXED Bug 59149
(Regression) Existing animations are not replaced when filling. 
https://bugs.webkit.org/show_bug.cgi?id=59149
Summary (Regression) Existing animations are not replaced when filling.
Dean Jackson
Reported 2011-04-21 15:47:33 PDT
When an animation is done but filling, changing the animation style doesn't trigger the new animation. Well, not exactly. The computed style updates, but the rendering doesn't change. This could be on HW anims only.
Attachments
Testcase (789 bytes, text/html)
2011-04-21 15:47 PDT, Dean Jackson 		no flags
Details
Patch (6.47 KB, patch)
2011-04-29 07:53 PDT, Dean Jackson 		no flags
DetailsFormatted DiffDiff
Patch (30.70 KB, patch)
2011-04-29 10:44 PDT, Dean Jackson 		simon.fraser: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dean Jackson
Comment 1 2011-04-21 15:47:59 PDT
Created attachment 90619 [details] Testcase
Dean Jackson
Comment 2 2011-04-21 15:51:30 PDT
<rdar://problem/9281794>
Dean Jackson
Comment 3 2011-04-21 18:25:04 PDT
This is a ridiculous bug on my part. The filling animation is not postActive, so it isn't being removed. I assumed the animation name change would remove it.
Dean Jackson
Comment 4 2011-04-21 19:03:52 PDT
Actually, not quite. The animation is being removed in CompositeAnimation, but CA isn't being asked to remove it.
Dean Jackson
Comment 5 2011-04-29 07:53:04 PDT
Created attachment 91679 [details] Patch
Simon Fraser (smfr)
Comment 6 2011-04-29 09:15:46 PDT
Comment on attachment 91679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91679&action=review > LayoutTests/animations/replace-filling-transform.html:2 > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" > + "http://www.w3.org/TR/html4/loose.dtd"> Can be just <!DOCTYPE html> > LayoutTests/animations/replace-filling-transform.html:4 > +<html lang="en"> Kill the lang. > LayoutTests/animations/replace-filling-transform.html:7 > + <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> > + <title>Replace a transform animation that is filling</title> Kill, kill > LayoutTests/animations/replace-filling-transform.html:8 > + <style type="text/css" media="screen"> Kill the attributes > LayoutTests/animations/replace-filling-transform.html:23 > + from { > + -webkit-transform: translate3d(100px, 0, 0); > + } > + to { > + -webkit-transform: translate3d(200px, 0, 0); > + } If this test is about hardware animation, then it should go into animation/3d > LayoutTests/animations/replace-filling-transform.html:35 > + <script type="text/javascript" charset="utf-8"> Kill the attributes > LayoutTests/animations/replace-filling-transform.html:49 > + if (computedValue == expectedValue) { > + result.innerHTML = "PASS - final state was " + expectedValue; > + } else { > + result.innerHTML = "FAIL - final state was " + computedValue + " expected " + expectedValue; > + } No need for braces on single lines. > LayoutTests/animations/replace-filling-transform.html:53 > + } Ditto. > LayoutTests/animations/replace-filling-transform.html:71 > + if (window.layoutTestController) { > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); > + } We normally put this code bare in the <script> tag.
Simon Fraser (smfr)
Comment 7 2011-04-29 10:07:52 PDT
Comment on attachment 91679 [details] Patch The test needs to dump pixels to reveal the failure. You can use dumpAsText(true) to dump pixels and text results.
Dean Jackson
Comment 8 2011-04-29 10:44:15 PDT
Created attachment 91698 [details] Patch
Dean Jackson
Comment 9 2011-04-29 10:46:18 PDT
updated from review comments and added pixel test (which means the test will fail where it should fail - even when getComputedStyle lies)
Simon Fraser (smfr)
Comment 10 2011-04-29 10:50:44 PDT
Comment on attachment 91698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91698&action=review r=me but please consider improving the pixel test. > LayoutTests/animations/3d/replace-filling-transform.html:78 > +This sets up two animations. It runs the first and then triggers the 2nd. The first fills > +forwards, but should still be replaced by the second. The first is a horizontal animation, the second is > +vertical, so the box should end up translated vertically down the page. Please put this text into an HTML comment, so that it doesn't show in the pixel result. That way, the result can be cross-platform. Also, make #result have opacity: 0 so it doesn't pollute the pixel result. For extra points, make the test box a 100x100px green square, and put a red square under its correct end position. Then when the test fails, red will be visible. If you've done all this, the pixel result can then be platform-agnostic and go into LayoutTests/animations/3d
Dean Jackson
Comment 11 2011-04-29 11:01:14 PDT
Great suggestions. Done in http://trac.webkit.org/changeset/85338
Note You need to log in before you can comment on or make changes to this bug.