Closed Bug 1244633 Opened 4 years ago Closed 4 years ago

implement AnimationEffectTiming delay

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: motozawa, Assigned: daisuke)

References

()

Details

Attachments

(2 files)

No description provided.
Assignee: motozawa → daisuke
Comment on attachment 8735324 [details]
MozReview Request: Bug 1244633 - Part 1: implement AnimationEffectTiming delay. r=birtles

https://reviewboard.mozilla.org/r/42705/#review39401
Attachment #8735324 - Flags: review?(bbirtles) → review+
Comment on attachment 8735325 [details]
MozReview Request: Bug 1244633 - Part 2: append tests for delay. r=birtles

https://reviewboard.mozilla.org/r/42707/#review39403

This is good but needs animation observer tests that:

* We create a change record when changing the delay
* We *don't* create a change record when setting the delay to its current value (including when the current value is not zero--e.g. by specifying a different value in the constructor)

Please add those tests before landing.

::: testing/web-platform/tests/web-animations/animation-effect-timing/delay.html:4
(Diff revision 1)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>delay tests</title>
> +<link rel="help" href="https://w3c.github.io/web-animations/#dom-animationeffecttimingreadonly-delay">

Wrong URL I think---this is not for the readonly interface, right?

::: testing/web-platform/tests/web-animations/animation-effect-timing/delay.html:33
(Diff revision 1)
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 100);
> +  assert_throws({name: "TypeError"}, function() {
> +    anim.effect.timing.delay = Infinity;
> +  }, 'we can not assign Infinity to timing.delay');
> +}, 'set delay Infinity');
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 100);
> +  assert_throws({name: "TypeError"}, function() {
> +    anim.effect.timing.delay = -Infinity;
> +  }, 'we can not assign negative Infinity to timing.delay');
> +}, 'set delay negative Infinity');
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 100);
> +  assert_throws({name: "TypeError"}, function() {
> +    anim.effect.timing.delay = NaN;
> +  }, 'we can not assign NaN to timing.delay');
> +}, 'set delay NaN');

I don't think we need these three tests. WebIDL should do it for us.

::: testing/web-platform/tests/web-animations/animation-effect-timing/delay.html:63
(Diff revision 1)
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 100);
> +  anim.effect.timing.delay = 100;
> +  assert_equals(anim.effect.getComputedTiming().progress, null);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, null);
> +}, 'Test that changing the delay to positive');

'Test adding a positive delay to an animation without a backwards fill makes it no longer active'

::: testing/web-platform/tests/web-animations/animation-effect-timing/delay.html:72
(Diff revision 1)
> +  var anim = div.animate({ opacity: [ 0, 1 ] },
> +                         { fill: 'both',
> +                           duration: 100 });
> +  anim.effect.timing.delay = -50;
> +  assert_equals(anim.effect.getComputedTiming().progress, 0.5);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, 0);

Not sure we need the check for currentIteration here?

::: testing/web-platform/tests/web-animations/animation-effect-timing/delay.html:74
(Diff revision 1)
> +                           duration: 100 });
> +  anim.effect.timing.delay = -50;
> +  assert_equals(anim.effect.getComputedTiming().progress, 0.5);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, 0);
> +}, 'Test that changing the delay to ' +
> +   'negative value that is half of duration');

'Test seeking an animation by setting a negative delay'

::: testing/web-platform/tests/web-animations/animation-effect-timing/delay.html:84
(Diff revision 1)
> +                         { fill: 'both',
> +                           duration: 100 });
> +  anim.effect.timing.delay = -100;
> +  assert_equals(anim.effect.getComputedTiming().progress, 1);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, 0);
> +}, 'Test that changing the delay to ' +

'Test finishing an animation using a large negative delay'
Attachment #8735325 - Flags: review?(bbirtles) → review+
Comment on attachment 8735324 [details]
MozReview Request: Bug 1244633 - Part 1: implement AnimationEffectTiming delay. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42705/diff/1-2/
Attachment #8735324 - Attachment description: MozReview Request: Bug 1244633 - Part 1: implement AnimationEffectTiming delay. r?birtles → MozReview Request: Bug 1244633 - Part 1: implement AnimationEffectTiming delay. r=birtles
Attachment #8735325 - Attachment description: MozReview Request: Bug 1244633 - Part 2: append tests for delay. r?birtles → MozReview Request: Bug 1244633 - Part 2: append tests for delay. r=birtles
Comment on attachment 8735325 [details]
MozReview Request: Bug 1244633 - Part 2: append tests for delay. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42707/diff/1-2/
Keywords: checkin-needed
hi part 2 failed to apply:

1 out of 1 hunks FAILED -- saving rejects to file dom/animation/test/chrome/test_animation_observers.html.rej
Flags: needinfo?(daisuke)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #9)
> hi part 2 failed to apply:
> 
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/animation/test/chrome/test_animation_observers.html.rej

OK! Thanks!
Flags: needinfo?(daisuke)
Comment on attachment 8735324 [details]
MozReview Request: Bug 1244633 - Part 1: implement AnimationEffectTiming delay. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42705/diff/2-3/
Comment on attachment 8735325 [details]
MozReview Request: Bug 1244633 - Part 2: append tests for delay. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42707/diff/2-3/
Fix bitrot.
https://hg.mozilla.org/mozilla-central/rev/b52399acb450
https://hg.mozilla.org/mozilla-central/rev/2effa07d02e1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.