Closed
Bug 1244643
Opened 8 years ago
Closed 8 years ago
implement AnimationEffectTiming easing
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: motozawa, Assigned: daisuke)
References
()
Details
Attachments
(3 files)
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: motozawa → daisuke
Comment 1•8 years ago
|
||
I'd like to take on, so could you assign this bug to me...?
Assignee | ||
Comment 2•8 years ago
|
||
Kato-san, thank you for your message! However, I already implemented this.. Could you help to fix other bugs?
Comment 3•8 years ago
|
||
Thank you for quick response! Then I will try to fix others ;)
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30e6f88412d9
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/44081/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44081/
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/44083/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44083/
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8737675 [details] MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44081/diff/1-2/
Attachment #8737675 -
Attachment description: MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r? → MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r?birtles
Attachment #8737676 -
Attachment description: MozReview Request: Bug 1244643 - Part 2: append tests for easing. r? → MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?birtles
Attachment #8737675 -
Flags: review?(bbirtles)
Attachment #8737676 -
Flags: review?(bbirtles)
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44083/diff/1-2/
Assignee | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8f25ae3c6b2
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bcce1b76e51
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/44083/#review41985 ::: dom/animation/test/chrome/test_animation_observers.html:1675 (Diff revision 2) > }); > > +addAsyncAnimTest("change_easing", > + { observe: div, subtree: true }, function*() { > + var anim = target.animate({ opacity: [ 0, 1 ] }, > + { duration: 100, easing: "steps(2, start)" }); Please use longer duration to avoid timeouts.
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11) > https://reviewboard.mozilla.org/r/44083/#review41985 > > ::: dom/animation/test/chrome/test_animation_observers.html:1675 > (Diff revision 2) > > }); > > > > +addAsyncAnimTest("change_easing", > > + { observe: div, subtree: true }, function*() { > > + var anim = target.animate({ opacity: [ 0, 1 ] }, > > + { duration: 100, easing: "steps(2, start)" }); > > Please use longer duration to avoid timeouts. OK!
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=695593db3b98
Assignee | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=358ecd148084
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8737675 [details] MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44081/diff/2-3/
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44083/diff/2-3/
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8737675 [details] MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44081/diff/3-4/
Attachment #8737675 -
Attachment description: MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r?birtles → MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r?hiro
Attachment #8737676 -
Attachment description: MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?birtles → MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?hiro
Attachment #8737675 -
Flags: review?(bbirtles) → review?(hiikezoe)
Attachment #8737676 -
Flags: review?(bbirtles) → review?(hiikezoe)
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44083/diff/3-4/
Comment 19•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro https://reviewboard.mozilla.org/r/44083/#review42177 We should consider unifying test codes in easing.html and the other, and reusing it in both of them. ::: dom/animation/test/chrome/test_animation_observers.html:1674 (Diff revision 4) > + var anim = target.animate({ opacity: [ 0, 1 ] }, > + { duration: 100 * MS_PER_SEC, I know we can not use addDivAndAnimate function which asserts if duration is shorter than 100s, but we can introduce a wrapper to create an animation on a given element and assert against short duration. What do you think? ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:5 (Diff revision 4) > +<!DOCTYPE html> > +<meta charset=utf-8> > +<title>easing tests</title> > +<link rel="help" href="https://w3c.github.io/web-animations/#dom-animationeffecttiming-easing"> > +<link rel="author" title="Daisuke Akatsuka" href="mailto:daisuke@mozilla-japan.org"> Please drop this author line. ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:72 (Diff revision 4) > +gEffectEasingTests.forEach(function(options) { > + test(function(t) { > + var target = createDiv(t); > + target.style.position = 'absolute'; > + var anim = target.animate([ { left: '0px' }, { left: '100px' } ], > + { duration: 1000, Please use longer duration here as well to prevent someone copies this somewhere in future even if this test is not asynchronous. ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:91 (Diff revision 4) > + }, options.desc); > +}); > + > +test(function(t) { > + var div = createDiv(t); > + var anim = div.animate({ opacity: [ 0, 1 ] }, 100); Same here.
Attachment #8737676 -
Flags: review?(hiikezoe)
Comment 20•8 years ago
|
||
We also need some test cases to check styles when the easing is changed.
Comment 21•8 years ago
|
||
Comment on attachment 8737675 [details] MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r=hiro https://reviewboard.mozilla.org/r/44081/#review42185 ::: dom/animation/AnimationEffectTiming.cpp:133 (Diff revision 4) > + TimingParams::ParseEasing( > + aEasing, > + AnimationUtils::GetCurrentRealmDocument(aCx), aRv); We can format these lines; TimingParams::ParseEasing(aEasing, AnimationUtils::GetCurrentRealmDocument(aCx), aRv); or TimingParams::ParseEasing( aEasing, AnimationUtils::GetCurrentRealmDocument(aCx), aRv);
Attachment #8737675 -
Flags: review?(hiikezoe) → review+
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f21726614edf
Assignee | ||
Comment 23•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45717/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45717/
Attachment #8737675 -
Attachment description: MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r?hiro → MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r=hiro
Attachment #8740317 -
Flags: review?(hiikezoe)
Attachment #8737676 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8737675 [details] MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44081/diff/4-5/
Assignee | ||
Comment 25•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44083/diff/4-5/
Comment 26•8 years ago
|
||
Comment on attachment 8740317 [details] MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r=hiro https://reviewboard.mozilla.org/r/45717/#review42275 This is not what I meant in comment #19 and on IRC. Test code for KeyframeEffect constructor in animation-effect-timing/easing.html is weird. What I meant is to create a new js file which includes common test cases and test codes and reuse it in animation-effect-timing/easing.html and keyframe-effect/effect-easing.html. Is there any reason we can do that?
Attachment #8740317 -
Flags: review?(hiikezoe)
Comment 27•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #26) > Comment on attachment 8740317 [details] > MozReview Request: Bug 1244643 - Part 3: move tests of animation effect > timing from keyframe tests. r?hiro > > https://reviewboard.mozilla.org/r/45717/#review42275 > > This is not what I meant in comment #19 and on IRC. Test code for > KeyframeEffect constructor in animation-effect-timing/easing.html is weird. After some thought, the test code in animation-effect-timing/easing.html is not so weird if we move all tests related to effect-easing (!keyframe-easing) into animation-effect-timing/easing.html.
Comment 28•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro https://reviewboard.mozilla.org/r/44083/#review42269 ::: dom/animation/test/chrome/test_animation_observers.html:6 (Diff revisions 4 - 5) > <!DOCTYPE html> > <meta charset=utf-8> > <title>Test chrome-only MutationObserver animation notifications</title> > <script src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> > <script src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script> > +<script src="../testharness.js"></script> Ah, this reminds me why I left test_animation_observers.html when I introduced addDivAndAnimate. Let's use MS_PER_SEC and not introduce addAnimate for now. Once we rewrite test_animation_observers.html with promise_test, we will re-consider about addAnimate. ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:105 (Diff revisions 4 - 5) > }); > + anim.cancel(); > }, 'Test invalid easing value'); > > +test(function(t) { > + var easingStepStart = { text: "steps(2, start)", func: stepStart(2) }; I probably did use 'easing' instead of 'text' in another test. ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:114 (Diff revisions 4 - 5) > + assert_style_left_at(anim, 250 * MS_PER_SEC, easingStepStart.func); > + > + anim.effect.timing.easing = easingStepEnd.text; > + assert_style_left_at(anim, 750 * MS_PER_SEC, easingStepEnd.func); I don't think this is a test to check styles when the easing is dynamically changed because assert_style_left_at sets currentTime in it. Should we write these tests somewhere in layout/style/test? Also I think we really need these dynamically changing test cases for functions which produce out of [0,1] values.
Attachment #8737676 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 29•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #26) > > Comment on attachment 8740317 [details] > > MozReview Request: Bug 1244643 - Part 3: move tests of animation effect > > timing from keyframe tests. r?hiro > > > > https://reviewboard.mozilla.org/r/45717/#review42275 > > > > This is not what I meant in comment #19 and on IRC. Test code for > > KeyframeEffect constructor in animation-effect-timing/easing.html is weird. > > After some thought, the test code in animation-effect-timing/easing.html is > not so weird if we move all tests related to effect-easing > (!keyframe-easing) into animation-effect-timing/easing.html. I'm sorry about this changing without any description. Yes, I already moved all tests that have no keyframe-easing. Remaining tests in keyframe-effect/effect-easing.html use both effect-easing and keyframe-easing at same time.
Assignee | ||
Comment 30•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28) > Comment on attachment 8737676 [details] > MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?hiro > > https://reviewboard.mozilla.org/r/44083/#review42269 > > ::: dom/animation/test/chrome/test_animation_observers.html:6 > (Diff revisions 4 - 5) > > <!DOCTYPE html> > > <meta charset=utf-8> > > <title>Test chrome-only MutationObserver animation notifications</title> > > <script src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> > > <script src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script> > > +<script src="../testharness.js"></script> > > Ah, this reminds me why I left test_animation_observers.html when I > introduced addDivAndAnimate. > > Let's use MS_PER_SEC and not introduce addAnimate for now. Once we rewrite > test_animation_observers.html with promise_test, we will re-consider about > addAnimate. OK! I'll drop addAnimate function in this time. > > ::: > testing/web-platform/tests/web-animations/animation-effect-timing/easing. > html:105 > (Diff revisions 4 - 5) > > }); > > + anim.cancel(); > > }, 'Test invalid easing value'); > > > > +test(function(t) { > > + var easingStepStart = { text: "steps(2, start)", func: stepStart(2) }; > > I probably did use 'easing' instead of 'text' in another test. Ok, I'll use 'easing' and 'easingFunction' as same as another one. > > ::: > testing/web-platform/tests/web-animations/animation-effect-timing/easing. > html:114 > (Diff revisions 4 - 5) > > + assert_style_left_at(anim, 250 * MS_PER_SEC, easingStepStart.func); > > + > > + anim.effect.timing.easing = easingStepEnd.text; > > + assert_style_left_at(anim, 750 * MS_PER_SEC, easingStepEnd.func); > > I don't think this is a test to check styles when the easing is dynamically > changed because assert_style_left_at sets currentTime in it. Should we > write these tests somewhere in layout/style/test? Ok! > Also I think we really need these dynamically changing test cases for > functions which produce out of [0,1] values. Ok! Thanks!
Comment 31•8 years ago
|
||
(In reply to Daisuke Akatsuka (:daisuke) from comment #29) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #26) > > > Comment on attachment 8740317 [details] > > > MozReview Request: Bug 1244643 - Part 3: move tests of animation effect > > > timing from keyframe tests. r?hiro > > > > > > https://reviewboard.mozilla.org/r/45717/#review42275 > > > > > > This is not what I meant in comment #19 and on IRC. Test code for > > > KeyframeEffect constructor in animation-effect-timing/easing.html is weird. > > > > After some thought, the test code in animation-effect-timing/easing.html is > > not so weird if we move all tests related to effect-easing > > (!keyframe-easing) into animation-effect-timing/easing.html. > > I'm sorry about this changing without any description. > > Yes, I already moved all tests that have no keyframe-easing. > Remaining tests in keyframe-effect/effect-easing.html use both effect-easing > and keyframe-easing at same time. I think there are still some test cases which are worthwhile for effect-easing. I looked at effect-easing.html now, I think all of test cases for effect-easing. I *think* test cases you left are more important than others. Let's leave keyframe-effect/effect-easing.htm as it is for checking results of styles. And let's check only setting results in animation-effect-timing/easing.html just like this: anim.effect.timing.easing = 'ease'; assert_equals(anim.effect.timing.easing, 'ease', ..);
Assignee | ||
Comment 32•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f49c04553b0c
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8737675 [details] MozReview Request: Bug 1244643 - Part 1: implement AnimationEffectTiming easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44081/diff/5-6/
Attachment #8740317 -
Attachment description: MozReview Request: Bug 1244643 - Part 3: move tests of animation effect timing from keyframe tests. r?hiro → MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r?hiro
Attachment #8737676 -
Flags: review?(hiikezoe)
Attachment #8740317 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 34•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44083/diff/5-6/
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8740317 [details] MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45717/diff/1-2/
Comment 36•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro https://reviewboard.mozilla.org/r/44083/#review42865 ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:15 (Diff revision 6) > +<script> > +'use strict'; > + > +function assert_progress(animation, time, delay, easingFunction) { > + animation.currentTime = time; > + var portion = (time - delay) / animation.effect.timing.duration; We should handle the case time - delay < 0 here? ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:96 (Diff revision 6) > + }); > + assert_throws({ name: 'TypeError' }, > + function() { > + anim.effect.timing.easing = 'test'; > + }); > + anim.cancel(); Is this really needed here? ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:116 (Diff revision 6) > + assert_progress(anim, 1750 * MS_PER_SEC, delay, easingStepEnd.easingFunction); > + anim.effect.timing.easing = easingStepStart.easing; > + assert_progress(anim, 1750 * MS_PER_SEC, > + delay, easingStepStart.easingFunction); assert_progress() sets currentTime inside it, I know setting the same time as currentTime does not change it but we should check the progress without setting currentTime? ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:126 (Diff revision 6) > + anim.currentTime = 2500 * MS_PER_SEC; > + anim.effect.timing.easing = easingStepEnd.easing; > + assert_equals(anim.effect.getComputedTiming().progress, 1, > + 'easing replace to stepEnd at after phase'); > + > + anim.cancel(); Same here.
Attachment #8737676 -
Flags: review?(hiikezoe)
Comment 37•8 years ago
|
||
Comment on attachment 8740317 [details] MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r=hiro https://reviewboard.mozilla.org/r/45717/#review42879 The reason why you named the new file as 'easing-tests.html' is that you are going to use it for keyframe easing as well? ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:10 (Diff revision 2) > <link rel="author" title="Hiroyuki Ikezoe" href="mailto:hiikezoe@mozilla-japan.org"> > <script src="/resources/testharness.js"></script> > <script src="/resources/testharnessreport.js"></script> > <script src="../testcommon.js"></script> > +<script src="../resources/easing-tests.js"></script> > + Nit: a blank line.
Attachment #8740317 -
Flags: review?(hiikezoe) → review+
Assignee | ||
Comment 38•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44083/diff/6-7/
Attachment #8740317 -
Attachment description: MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r?hiro → MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r=hiro
Attachment #8737676 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 39•8 years ago
|
||
Comment on attachment 8740317 [details] MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45717/diff/2-3/
Updated•8 years ago
|
Attachment #8737676 -
Flags: review?(hiikezoe)
Comment 40•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro https://reviewboard.mozilla.org/r/44083/#review42891 ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:14 (Diff revisions 6 - 7) > + // stepStart and stepEnd of easingFunction parameter calculates > + // even if the portion is over 1.0 or negative value. > + // So we should check the parameters. This comment does not make sense to me because this assertion is also called against cubic bezier functions. And I think we should make this assertion generic, I mean this assertion should accept time - delay < 0 case for now, and eventually it will consider fill mode or other attributes as well? By the time, this assertion will be in testcommon.js. ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:124 (Diff revisions 6 - 7) > anim.effect.timing.easing = easingStepStart.easing; > assert_progress(anim, 1750 * MS_PER_SEC, > delay, easingStepStart.easingFunction); Hmmm, it seems to me that assert_progress is still to set currentTime. Am I missing something?
Assignee | ||
Comment 41•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44083/diff/7-8/
Attachment #8737676 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 42•8 years ago
|
||
Comment on attachment 8740317 [details] MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45717/diff/3-4/
Comment 43•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro https://reviewboard.mozilla.org/r/44083/#review42923 ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:13 (Diff revision 8) > +<body> > +<div id="log"></div> > +<script> > +'use strict'; > + > +function assert_progress(animation, currentTime, delay, easingFunction) { Please file a new bug for making assert_progress generic and moving it into testcommon.js ::: testing/web-platform/tests/web-animations/animation-effect-timing/easing.html:125 (Diff revision 8) > + > + anim.effect.timing.easing = easingStepEnd.easing; > + assert_equals(anim.effect.getComputedTiming().progress, 0, > + 'easing replace to stepEnd at before phase'); > + > + assert_progress(anim, 1750 * MS_PER_SEC, delay, easingStepEnd.easingFunction); I think we should set currentTime outside assert_progress here otherwise it is hard to understand what the purpose of this test. For example: anim.currentTime = delay + 750 * MS_PER_SEC; assert_equals(progress, 0.5); // steps(2, end); anim.effect.timing.easing = 'steps(2, start)'; assert_equals(progress, 1); This pattern looks easy to understand for me. What do you think?
Attachment #8737676 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 44•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > Comment on attachment 8737676 [details] > MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?hiro > > https://reviewboard.mozilla.org/r/44083/#review42923 > > ::: > testing/web-platform/tests/web-animations/animation-effect-timing/easing. > html:13 > (Diff revision 8) > > +<body> > > +<div id="log"></div> > > +<script> > > +'use strict'; > > + > > +function assert_progress(animation, currentTime, delay, easingFunction) { > > Please file a new bug for making assert_progress generic and moving it into > testcommon.js > ::: > testing/web-platform/tests/web-animations/animation-effect-timing/easing. > html:125 > (Diff revision 8) > > + > > + anim.effect.timing.easing = easingStepEnd.easing; > > + assert_equals(anim.effect.getComputedTiming().progress, 0, > > + 'easing replace to stepEnd at before phase'); > > + > > + assert_progress(anim, 1750 * MS_PER_SEC, delay, easingStepEnd.easingFunction); > > I think we should set currentTime outside assert_progress here otherwise it > is hard to understand what the purpose of this test. > > For example: > > anim.currentTime = delay + 750 * MS_PER_SEC; > assert_equals(progress, 0.5); // steps(2, end); > anim.effect.timing.easing = 'steps(2, start)'; > assert_equals(progress, 1); > > This pattern looks easy to understand for me. > > What do you think? I agree with you! If so, don't we have to support 'delay' parameter and the assertions in assert_progress since all tests will not need 'delay' in this case? Then should we add new assertions for currentTime is over duration and negative value instead?
Comment 45•8 years ago
|
||
(In reply to Daisuke Akatsuka (:daisuke) from comment #44) > If so, don't we have to support 'delay' parameter and the assertions in > assert_progress since all tests will not need 'delay' in this case? Right. At least for this case. > Then should we add new assertions for currentTime is over duration and > negative value instead? As we discussed this morning, most of test codes for web animations API should check progress, so I think assert_progress needs delay sooner or later. I guess currentTime is not needed for assert_progress eventually.
Comment 46•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > Comment on attachment 8737676 [details] > MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?hiro > > https://reviewboard.mozilla.org/r/44083/#review42923 > > ::: > testing/web-platform/tests/web-animations/animation-effect-timing/easing. > html:13 > (Diff revision 8) > > +<body> > > +<div id="log"></div> > > +<script> > > +'use strict'; > > + > > +function assert_progress(animation, currentTime, delay, easingFunction) { > > Please file a new bug for making assert_progress generic and moving it into > testcommon.js As you know, I'm a little hesitant about adding utility functions that hide what part of the API we're testing, especially when they are in another file. What does assert_progress do? I would expect it to be something like: function assert_progress(effect, expected, description) { assert_equals(effect.getComputedTiming().progress, expected, description); } But it doesn't seem to be anything like that. What is it doing? Let me know and I can help with finding a more suitable name. > ::: > testing/web-platform/tests/web-animations/animation-effect-timing/easing. > html:125 > (Diff revision 8) > > + > > + anim.effect.timing.easing = easingStepEnd.easing; > > + assert_equals(anim.effect.getComputedTiming().progress, 0, > > + 'easing replace to stepEnd at before phase'); > > + > > + assert_progress(anim, 1750 * MS_PER_SEC, delay, easingStepEnd.easingFunction); > > I think we should set currentTime outside assert_progress here otherwise it > is hard to understand what the purpose of this test. > > For example: > > anim.currentTime = delay + 750 * MS_PER_SEC; > assert_equals(progress, 0.5); // steps(2, end); > anim.effect.timing.easing = 'steps(2, start)'; > assert_equals(progress, 1); I agree this is a *lot* easier to understand.
Comment 47•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #46) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > > Comment on attachment 8737676 [details] > > MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?hiro > > > > https://reviewboard.mozilla.org/r/44083/#review42923 > > > > ::: > > testing/web-platform/tests/web-animations/animation-effect-timing/easing. > > html:13 > > (Diff revision 8) > > > +<body> > > > +<div id="log"></div> > > > +<script> > > > +'use strict'; > > > + > > > +function assert_progress(animation, currentTime, delay, easingFunction) { > > > > Please file a new bug for making assert_progress generic and moving it into > > testcommon.js > > As you know, I'm a little hesitant about adding utility functions that hide > what part of the API we're testing, especially when they are in another file. > > What does assert_progress do? I would expect it to be something like: > > function assert_progress(effect, expected, description) { > assert_equals(effect.getComputedTiming().progress, expected, > description); > } > > But it doesn't seem to be anything like that. Yes, right. I thought the same thing the last morning. Bug Daisuke introduced delay as an extra argument to assert_progress in a new patch, so I thought he has an aim to use the assertion for other testing, then I'd suggest it to make generic.
Comment 48•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #47) > (In reply to Brian Birtles (:birtles) from comment #46) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > > > Comment on attachment 8737676 [details] > > > MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?hiro > > > > > > https://reviewboard.mozilla.org/r/44083/#review42923 > > > > > > ::: > > > testing/web-platform/tests/web-animations/animation-effect-timing/easing. > > > html:13 > > > (Diff revision 8) > > > > +<body> > > > > +<div id="log"></div> > > > > +<script> > > > > +'use strict'; > > > > + > > > > +function assert_progress(animation, currentTime, delay, easingFunction) { > > > > > > Please file a new bug for making assert_progress generic and moving it into > > > testcommon.js > > > > As you know, I'm a little hesitant about adding utility functions that hide > > what part of the API we're testing, especially when they are in another file. > > > > What does assert_progress do? I would expect it to be something like: > > > > function assert_progress(effect, expected, description) { > > assert_equals(effect.getComputedTiming().progress, expected, > > description); > > } > > > > But it doesn't seem to be anything like that. > > Yes, right. I thought the same thing the last morning. Bug Daisuke *But*
Assignee | ||
Comment 49•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #47) > (In reply to Brian Birtles (:birtles) from comment #46) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > > > Comment on attachment 8737676 [details] > > > MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?hiro > > > > > > https://reviewboard.mozilla.org/r/44083/#review42923 > > > > > > ::: > > > testing/web-platform/tests/web-animations/animation-effect-timing/easing. > > > html:13 > > > (Diff revision 8) > > > > +<body> > > > > +<div id="log"></div> > > > > +<script> > > > > +'use strict'; > > > > + > > > > +function assert_progress(animation, currentTime, delay, easingFunction) { > > > > > > Please file a new bug for making assert_progress generic and moving it into > > > testcommon.js > > > > As you know, I'm a little hesitant about adding utility functions that hide > > what part of the API we're testing, especially when they are in another file. > > > > What does assert_progress do? I would expect it to be something like: > > > > function assert_progress(effect, expected, description) { > > assert_equals(effect.getComputedTiming().progress, expected, > > description); > > } > > > > But it doesn't seem to be anything like that. > > Yes, right. I thought the same thing the last morning. Bug Daisuke > introduced delay as an extra argument to assert_progress in a new patch, so > I thought he has an aim to use the assertion for other testing, then I'd > suggest it to make generic. In this time, I wanted to use "delay" with easing function, so I extended assert_progress. Now, we don't need "delay" after modify above. I'll remove "delay" from assert_progress at this time. Also regarding to common assertion,, when we need that, please let me discuss again. Thanks!
Assignee | ||
Comment 50•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44083/diff/8-9/
Attachment #8737676 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 51•8 years ago
|
||
Comment on attachment 8740317 [details] MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45717/diff/4-5/
Comment 52•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro https://reviewboard.mozilla.org/r/44083/#review43289
Attachment #8737676 -
Flags: review?(hiikezoe) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 53•8 years ago
|
||
Comment on attachment 8737676 [details] MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44083/diff/9-10/
Attachment #8737676 -
Attachment description: MozReview Request: Bug 1244643 - Part 2: append tests for easing. r?hiro → MozReview Request: Bug 1244643 - Part 2: append tests for easing. r=hiro
Assignee | ||
Comment 54•8 years ago
|
||
Comment on attachment 8740317 [details] MozReview Request: Bug 1244643 - Part 3: move common test cases to a new file. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45717/diff/5-6/
Comment 55•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b535eb7300d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd4ec949121 https://hg.mozilla.org/integration/mozilla-inbound/rev/7887410b3392
Keywords: checkin-needed
Comment 56•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b535eb7300d8 https://hg.mozilla.org/mozilla-central/rev/6cd4ec949121 https://hg.mozilla.org/mozilla-central/rev/7887410b3392
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•