Closed Bug 1244643 Opened 8 years ago Closed 8 years ago

implement AnimationEffectTiming easing

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: motozawa, Assigned: daisuke)

References

()

Details

Attachments

(3 files)

      No description provided.
Assignee: motozawa → daisuke
I'd like to take on, so could you assign this bug to me...?
Kato-san, thank you for your message!
However, I already implemented this..
Could you help to fix other bugs?
Thank you for quick response! Then I will try to fix others ;)
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)
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/
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.
(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!
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/
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/
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)
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 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)
We also need some test cases to check styles when the easing is changed.
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+
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)
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/
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 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)
(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 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)
(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.
(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!
(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', ..);
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)
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/
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 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 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+
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)
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/
Attachment #8737676 - Flags: review?(hiikezoe)
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?
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)
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 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)
(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?
(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.
(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.
(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 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*
(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!
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)
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 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+
Keywords: checkin-needed
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
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/
You need to log in before you can comment on or make changes to this bug.