Bug 59149

Summary:

(Regression) Existing animations are not replaced when filling.

Product:

WebKit

Reporter:

Dean Jackson <dino>

Component:

CSS

Assignee:

Dean Jackson <dino>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

simon.fraser

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

59839

Attachments:

Description	Flags
Testcase	none
Patch	none
Patch	simon.fraser: review+

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