LayoutTests/svg/animations/animate-restart-never.html - Issue 1119723002: SVG animate should respect attribute 'restart=never'.

Side by Side Diff: LayoutTests/svg/animations/animate-restart-never.html

Issue 1119723002: SVG animate should respect attribute 'restart=never'. (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Align with review comments Created 5 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <head>
	fs 2015/05/04 14:46:51 Not needed. Not needed.
	3 <script>

	4 if (window.testRunner)

	5 testRunner.waitUntilDone();

	6

	7 function animateTest()

	8 {

	9 var rect = document.getElementById('rect');

	10 var anim = document.getElementById('anim');

	11 var actions = [

	12 function() {

	13 rect.setAttribute("fill", "green");
	fs 2015/05/04 14:46:51 I'm not quite sure why you'd need this. I'm not quite sure why you'd need this. Shanmuga Pandi 2015/05/04 15:21:31 As you said earlier, Green color should be used fo Show quoted text On 2015/05/04 14:46:51, fs wrote: > I'm not quite sure why you'd need this. As you said earlier, Green color should be used for expected result. So I have used this. fs 2015/05/04 15:37:11 Yes, but why not just set on the <rect> in the con Show quoted text On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > On 2015/05/04 14:46:51, fs wrote: > > I'm not quite sure why you'd need this. > > As you said earlier, Green color should be used for expected result. So I have > used this. Yes, but why not just set on the <rect> in the content below? Shanmuga Pandi 2015/05/05 06:53:26 Done. Show quoted text On 2015/05/04 15:37:11, fs wrote: > On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > > On 2015/05/04 14:46:51, fs wrote: > > > I'm not quite sure why you'd need this. > > > > As you said earlier, Green color should be used for expected result. So I have > > used this. > > Yes, but why not just set on the <rect> in the content below? Done.
	14 anim.setAttribute("from", "green");

	15 anim.setAttribute("to", "red");
	fs 2015/05/04 14:46:51 The element already have these set? The element already have these set? Shanmuga Pandi 2015/05/04 15:21:31 I will remove this. Show quoted text On 2015/05/04 14:46:51, fs wrote: > The element already have these set? I will remove this.
	16 anim.setAttribute("fill", "freeze");
	fs 2015/05/04 14:46:51 Set this on the element in the content instead. Set this on the element in the content instead. Shanmuga Pandi 2015/05/04 15:21:31 If I added into this content, then it is really di Show quoted text On 2015/05/04 14:46:51, fs wrote: > Set this on the element in the content instead. If I added into this content, then it is really difficult to find out whether animation got completed or not. If we freeze there then rect always having "red" at the end. Also this test case will pass , even without this fix. fs 2015/05/04 15:37:11 I'd say that either you need fill=freeze or you do Show quoted text On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > On 2015/05/04 14:46:51, fs wrote: > > Set this on the element in the content instead. > > If I added into this content, then it is really difficult to find out whether > animation got completed or not. If we freeze there then rect always having "red" > at the end. Also this test case will pass , even without this fix. I'd say that either you need fill=freeze or you don't. If you need it, then just put it in the content rather than than mutate it like this. Shanmuga Pandi 2015/05/05 06:53:25 Acknowledged. Show quoted text On 2015/05/04 15:37:11, fs wrote: > On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > > On 2015/05/04 14:46:51, fs wrote: > > > Set this on the element in the content instead. > > > > If I added into this content, then it is really difficult to find out whether > > animation got completed or not. If we freeze there then rect always having > "red" > > at the end. Also this test case will pass , even without this fix. > > I'd say that either you need fill=freeze or you don't. If you need it, then just > put it in the content rather than than mutate it like this. Acknowledged.
	17 },

	18 function() {

	19 anim.beginElement();

	20 },

	21 function() {

	22 if (window.testRunner)

	23 testRunner.notifyDone();

	24 },

	25 ];

	26

	27 function nextAction() {
	fs 2015/05/04 14:46:51 Just drop this whole mechanism - it's overkill for Just drop this whole mechanism - it's overkill for what you're trying to achieve (test) here. Something along the lines of: function animationEnded() { anim.beginElement(); testRunner.displayAsyncThen(function() { testRunner.notifyDone(); }); } ... <animate ... onend="animationEnded()"/> ought to work. (The dur="..." still worries me a bit though.) Shanmuga Pandi 2015/05/04 15:21:31 I will try this as you commented. Show quoted text On 2015/05/04 14:46:51, fs wrote: > Just drop this whole mechanism - it's overkill for what you're trying to achieve > (test) here. > > Something along the lines of: > > function animationEnded() { > anim.beginElement(); > testRunner.displayAsyncThen(function() { > testRunner.notifyDone(); > }); > } > ... > <animate ... onend="animationEnded()"/> > > ought to work. (The dur="..." still worries me a bit though.) I will try this as you commented.
	28 var f = actions.shift();

	29 if (!f)

	30 return;

	31 f();

	32 setTimeout(nextAction,50);

	33 }

	34 setTimeout(nextAction,50);

	35 }

	36 </script>

	37 </head>

	38 <body onload="animateTest()">
	fs 2015/05/04 14:46:51 Could set onload within <script> instead, and then Could set onload within <script> instead, and then you wouldn't need to define the <body>.
	39 <svg height="200" width="200">

	40 <rect id="rect" width="100px" height="100px" fill="red"/>

	41 <animate id="anim" xlink:href="#rect"
	fs 2015/05/04 14:46:51 Just make this element a child of the <rect> and d Just make this element a child of the <rect> and drop the href. Shanmuga Pandi 2015/05/05 06:53:25 Done. Show quoted text On 2015/05/04 14:46:51, fs wrote: > Just make this element a child of the <rect> and drop the href. Done.
	42 begin="0s"

	43 dur="0.01s"
	fs 2015/05/04 14:46:51 I get a suspicion that this will be flaky. I get a suspicion that this will be flaky. Shanmuga Pandi 2015/05/05 06:53:26 Acknowledged. Show quoted text On 2015/05/04 14:46:51, fs wrote: > I get a suspicion that this will be flaky. Acknowledged.
	44 attributeType="xml"
	fs 2015/05/04 14:46:51 This doesn't looked necessary. This doesn't looked necessary. Shanmuga Pandi 2015/05/05 06:53:26 Done. Show quoted text On 2015/05/04 14:46:51, fs wrote: > This doesn't looked necessary. Done.
	45 attributeName="fill"

	46 from="green" to="red"

	47 repeatCount="1"
	fs 2015/05/04 14:46:51 Nor this. Nor this. Shanmuga Pandi 2015/05/05 06:53:26 Done. Show quoted text On 2015/05/04 14:46:51, fs wrote: > Nor this. Done.
	48 restart="never"/>

	49 </svg>

	50 </body>

	51 </html>

OLD	NEW

« no previous file with comments | « no previous file | LayoutTests/svg/animations/animate-restart-never-expected.html » ('j') | Source/core/svg/animation/SVGSMILElement.cpp » ('J')