Bug 59149 - (Regression) Existing animations are not replaced when filling.
Summary: (Regression) Existing animations are not replaced when filling.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks: 59839
  Show dependency treegraph
 
Reported: 2011-04-21 15:47 PDT by Dean Jackson
Modified: 2011-04-29 15:30 PDT (History)
1 user (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (30.70 KB, patch)
2011-04-29 10:44 PDT, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 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.
Comment 1 Dean Jackson 2011-04-21 15:47:59 PDT
Created attachment 90619 [details]
Testcase
Comment 2 Dean Jackson 2011-04-21 15:51:30 PDT
<rdar://problem/9281794>
Comment 3 Dean Jackson 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.
Comment 4 Dean Jackson 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.
Comment 5 Dean Jackson 2011-04-29 07:53:04 PDT
Created attachment 91679 [details]
Patch
Comment 6 Simon Fraser (smfr) 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.
Comment 7 Simon Fraser (smfr) 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.
Comment 8 Dean Jackson 2011-04-29 10:44:15 PDT
Created attachment 91698 [details]
Patch
Comment 9 Dean Jackson 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)
Comment 10 Simon Fraser (smfr) 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
Comment 11 Dean Jackson 2011-04-29 11:01:14 PDT
Great suggestions. Done in
http://trac.webkit.org/changeset/85338