1096776 - Support Animations without a timeline or with an inactive timeline

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

Currently we do fairly haphazard checks of whether or not we have a null timeline time. Our handling when it is null is completely untested and in some cases incomplete.

In particular play() is tricky. For example, consider the following sequence of events:

1. Call AnimationPlayer.play()
(2. Animation timeline disappears or becomes inactive)
3. First frame is rendered
4. We go to resolve the start time of the player but now the timeline time is null

Firstly, the spec needs to be updated to address this. I *think* what should happen is we remain in the pending state until we get an active timeline. But we probably don't want to keep the player in the document's hash of pending players and check it over and over again.

Instead I think we'll want to do something like:
* Add an extra mIsWaitingForActiveTimeline member to AnimationPlayer
* In AnimationPlayer::PlayerState() return pending if the player is in the set of pending players OR mIsWaitingForActiveTimeline is true
* In AnimationPlayer::SetTimeline check if the timeline is now active and complete the pending operation if so
  (And in that case, use the timeline time as the "ready time")
* In AnimationPlayer::Cancel(), Pause() etc. reset mIsWaitingForActiveTimeline as appropriate
* For timelines that can become active/inactive, give them a way to update their listeners so they can complete any pending operation (or maybe we could just check this in AnimationPlayer::Tick?)

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: sample-documentTimeline-null.html

The DocumentTimeline will null following case:
1) write document of iframe using document.write API.
2) re-write document by same method.
3) back to previous page( in this case, previous page is 1) using history.back API

In this case, documentTimeline.currentTime is null. So Web Animation API won't performed.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: DocumentTimeline-Null-Testcase.html

We can reproduce this phenomenon with more simple test case.
The flow of this test case is as follow:
 1) Write the content using document.write API.
 2) Call the location.reload API

Perhaps, I think that the reason of this phenomenon is calling to reload.
The gecko will stop loading page when called the location.reload API[1], however the gecko won't init the timing variables when load to the next content if content is wyciwyg[2]. So the DocumentTimeline will null.

[1] https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/docshell/base/nsDocShell.cpp#7331
[2] https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/docshell/base/nsDocShell.cpp#7244

I don't know that this phenomenon is correct. (related bug 703855 ?)
In either case, the DocumentTimeline could be null. So we should check whether DocumentTimeline is null.

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f21a15f86ae7cf09424f72e7d8b0dc32e5badd0c

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

I think that we should call to AnimationTimeline::RemoveAnimation() when setting the new timeline. We will associate with an animation object and the timeline.[1] 

[1] https://dxr.mozilla.org/mozilla-central/rev/c3f5e6079284a7b7053c41f05d0fe06ff031db03/dom/animation/AnimationTimeline.cpp#49

I think that we can reproduce this phenomenon using this code:
(We can run to this code after landing bug 1178662 and bug 1267510)

------------------------------------------------------------
timeline1 = new DocumentTimeline(0);
animation1 = new Animation(new KeyframeEffect(img, null, 100 * 1000));
animation2 = new Animation(new KeyframeEffect(img, null, 100 * 1000));

// timeline1 have a relationship with animation1
animation1.timeline = timeline1;

// document.timeline have a relationship with animation2
animation2.timeline = document.timeline;

// animation2 already have a relationship with document.timeline.
// So LinkedListElement(Animation1) of timeline1 can't add relationship with animation2 object.
// Because LinkedListElement will reject adding object which is not belong with list.[2]
animation2.timeline = timeline1;
------------------------------------------------------------
[2] https://dxr.mozilla.org/mozilla-central/source/mfbt/LinkedList.h#282

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

In current implementation, The code of using Timeline is as follow:

[The codes of accessing timeline]
  * Animation.cpp
      All codes of accessing timeline have a null check.
  * Animation.h
      This header have a getter for timeline. So it is responsibility for caller to check null.
  * AnimationTimeline.cpp
      All codes of accessing timeline have a null check.
  * DocumentTimeline.cpp
      This part don't have a null check code.[1]
      But this check will not require if we modify as comment #4.
  * PendingAnimationTracker.cpp
      All codes of accessing timeline have a null check.
  * nsDisplayList.cpp
      All codes of accessing timeline have a null check.

[1] https://dxr.mozilla.org/mozilla-central/rev/f3f2fa1d7eed5a8262f6401ef18ff8117a3ce43e/dom/animation/DocumentTimeline.cpp#124

So I think that we need to modify the constructor of Animation and setter of timeline(comment #4) only.

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=849e2d8d2ef7

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: MozReview Request: Bug 1096776 part 2 - Modify animation in order to support null timeline. r?birtles

Review commit: https://reviewboard.mozilla.org/r/53448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53448/

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: MozReview Request: Bug 1096776 - Part2. Remove animation associated timelline when setting the new timeline. r?birtles

We should remove relation of animation and timeline when setting the new
timeline.

 In bug 1223445, an animation class became child class of LinkedListElement, and
timeline has a LinkedList of an Animation object. These change work well when
animation can't set the new timeline.

 However LinkedList doesn't allow insert new object which belong with other
list. So we can't set new timeline to animation when new timeline is belong with
other animation. See bug 1096776 comment #4, for more detail.

Review commit: https://reviewboard.mozilla.org/r/53450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53450/

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: MozReview Request: Bug 1096776 part 1 - Add animation mochitest for examine timeline is null. r?birtles

We should add test of setting timeline, after land
bug 1178662(Make Animation.timeline writable).
In this bug, I focused on existing tests.

Review commit: https://reviewboard.mozilla.org/r/53810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53810/

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8754178 [details]
MozReview Request: Bug 1096776 part 2 - Modify animation in order to support null timeline. r?birtles

https://reviewboard.mozilla.org/r/53448/#review50534

I think we need to modify testing/web-platform/meta/web-animations/interfaces/Animation/constructor.html.ini as well, right?

Otherwise we will get an unexpected pass? i.e. we should move the change from part 3 to part 1.

r=me with the changes to test expectations needed so that this patch passes all tests when applied independently of part 3.

(Also a very very minor nit, instead of "Bug 1096776 - Part1. ..." if we use something like "Bug 1096776 part 1 - ..." I think the messages that get applied to web-platform-tests will be more clear. Also, 'Part1' without a space looks like a mistake in English.)

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8754179 [details]
MozReview Request: Bug 1096776 - Part2. Remove animation associated timelline when setting the new timeline. r?birtles

https://reviewboard.mozilla.org/r/53450/#review50550

I don't think we should make this change in this bug. We don't have a test case to prove it is necessary (and it's not clear to me why it's necessary yet since the call to AnimationTimeline::NotifyAnimationUpdated should remove the animation from the existing timeline). Let's make this change when we implement the timeline setter if it's needed then.

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8754180 [details]
MozReview Request: Bug 1096776 part 1 - Add animation mochitest for examine timeline is null. r?birtles

https://reviewboard.mozilla.org/r/53810/#review50532

Most of these tests are really good.

In terms of ordering patches, when possible it's best to add the tests first. If they fail, then, at least for web-platform-tests, we can add an annotation in /meta/. For mochitests we can use todo() and so on. Then if the new tests are just extra regression tests, we can easily try them without the changes in the bug. If the new tests are for the features we are about to add, it's clear to the reviewer that the tests are testing the correct thing (i.e. they fail without the changes). It also eliminates the risk that we will check in the changes without tests.

You can see an example in bug 1264822 of this approach.

Unfortunately, for some types of tests (like those in /dom/animation/tests/) we can't easily annotate tests as failing so for those kinds of tests it's ok to add the tests later.


Also, it might be nice to add tests that the "update the finish state" procedure does the right thing when we have no timeline. That can be a separate patch however.


> We should add test of setting timeline, after land
> bug 1178662(Make Animation.timeline writable).
> In this bug, I focused on existing tests.

I don't think we need this comment.

::: testing/web-platform/meta/MANIFEST.json:35942
(Diff revision 1)
> -      "web-animations/animatable/animate.html",
> -      "web-animations/keyframe-effect/getComputedTiming.html",
> +      "web-animations/animation/onfinish.html",
> +      "web-animations/animation-effect-timing/getComputedStyle.html",
> -      "web-animations/animation/cancel.html",
> -      "web-animations/animation/reverse.html",
>        "web-animations/animation-model/animation-types/not-animatable.html",
> -      "web-animations/animation/playbackRate.html",
> -      "web-animations/keyframe-effect/keyframe-handling.html",
> +      "web-animations/animation/ready.html",
> +      "web-animations/keyframe-effect/setFrames.html",
> -      "web-animations/animation/finish.html",
> -      "web-animations/animation/startTime.html",
> -      "web-animations/animation-timeline/document-timeline.html",
>        "web-animations/animation-model/keyframes/effect-value-context.html",
> +      "web-animations/keyframe-effect/getComputedTiming.html",
> +      "web-animations/keyframe-effect/effect-easing.html",
> +      "web-animations/animation/pause.html",
>        "web-animations/animation-effect-timing/delay.html",

manifest.py appears to be doing silly things here. Do you mind filing a bug and quoting this changeset.

You can see that many of the lines are the same but the order has changed a lot.

::: testing/web-platform/tests/web-animations/interfaces/Animation/reverse.html:154
(Diff revision 1)
> +  assert_throws('InvalidStateError',
> +                function() { animation.reverse(); },
> +                'reverse() should occur the InvalidStateError when ' +
> +                'an animation doesn\'t have an AnimationTimeline.');

I think we can just drop the message here.

::: testing/web-platform/tests/web-animations/interfaces/Animation/reverse.html:159
(Diff revision 1)
> +  assert_throws('InvalidStateError',
> +                function() { animation.reverse(); },
> +                'reverse() should occur the InvalidStateError when ' +
> +                'an animation doesn\'t have an AnimationTimeline.');
> +
> +}, 'Catch the InvalidStateError when reverse with no AnimationTimeline');

'Reversing an animation without an active timeline throws an InvalidStateError'

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:13
(Diff revision 1)
> +promise_test(function(t)
> +{

I think we use this style in some files, but I'm pretty sure it's not correct. I think it's supposed to be just:

promise_test(function(t) {

Likewise elsewhere in this file.

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:21
(Diff revision 1)
> +    assert_not_equals(animation.currentTime, null,
> +      'Animation has a resolved hold time');

We should assert that it is 0. (assert_equals does a === check so it won't pass accidentally if the current time is null.)

We should make the statement, 'Current time returns the hold time set when entering the play-pending state'

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:24
(Diff revision 1)
> +  animation.play();
> +  return animation.ready.then(function() {
> +    assert_not_equals(animation.currentTime, null,
> +      'Animation has a resolved hold time');
> +  });
> +}, 'Animation return the resolved hold time');

'The current time returns the hold time when set'

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:32
(Diff revision 1)
> +  return animation.ready.then(function() {
> +    assert_equals(animation.currentTime, null,
> +      'Animation doesn\'t have the associated timeline');
> +  });
> +}, 'Animation return the unresolved current time when animation has ' +
> +   'no associated timeline');

I think both these messages are really saying the same thing so we can drop the first message and make the second message:

'The current time is unresolved when there is no associated timeline (and no hold time is set)'

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:38
(Diff revision 1)
> +    assert_equals(animation.currentTime, null,
> +      'Animation doesn\'t have the associated timeline');
> +  });
> +}, 'Animation return the unresolved current time when animation has ' +
> +   'no associated timeline');
> +

Let's add:

// FIXME: Test that the current time is unresolved when we have an inactive
// timeline if we find a way of creating an inactive timeline!

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:39
(Diff revision 1)
> +promise_test(function(t)
> +{
> +  var animation =
> +    new Animation(new KeyframeEffect(createDiv(t), null, 100 * MS_PER_SEC),
> +                  document.timeline);
> +
> +  animation.play();
> +  return animation.ready.then(function() {
> +    animation.startTime = null;
> +    assert_not_equals(animation.currentTime, null,
> +      'Animation has a unresolved start time');
> +  });

Could we just:

* Make this a regular test()
* Drop the call to play() and animation.ready
* Then do the test?

I think that would be simpler?

(In fact, I wonder why this current test passes. When we call play() I imagine we resolve the hold time, then I would have expected the setter for the startTime to maintain the hold time.)

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:51
(Diff revision 1)
> +}, 'Animation return the unresolved current time when animation has ' +
> +   'unresolved start time.');

Again, let's just leave the one message:

'The current time is unresolved when the start time is unresolved (and no hold time is set)'

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:60
(Diff revision 1)
> +  animation.startTime = document.timeline.currentTime;
> +  var timelineTime = document.timeline.currentTime;
> +  var startTime = animation.startTime;
> +  var playbackRate = animation.playbackRate;
> +
> +  assert_equals(animation.currentTime,
> +                (timelineTime - startTime) * playbackRate,
> +                'Animation has a unresolved start time');

This doesn't make sense.

var startTime = animation.startTime = document.timeline.currentTime;
var timelineTime = document.timeline.currentTime;
∴ startTime == timelineTime
∴ (timelineTime - startTime) * playbackRate
  = 0 * playbackRate
  = 0

i.e. this is just testing that animation.currentTime == 0.

(Also, the message 'has a unresolved start time' is not correct and doesn't really explain what we're testing)

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

https://reviewboard.mozilla.org/r/53450/#review50550

Thanks Brian!
OK, I'll add this change on bug 1178662 if we should fix this.

Comment 14

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8754180 [details]
MozReview Request: Bug 1096776 part 1 - Add animation mochitest for examine timeline is null. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53810/diff/1-2/

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8754178 [details]
MozReview Request: Bug 1096776 part 2 - Modify animation in order to support null timeline. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53448/diff/1-2/

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

https://reviewboard.mozilla.org/r/53810/#review50532

> We should assert that it is 0. (assert_equals does a === check so it won't pass accidentally if the current time is null.)
> 
> We should make the statement, 'Current time returns the hold time set when entering the play-pending state'

Depends on test timing, Animation.currentTime isn't zero. So I used assert_greater_than_equal function.
This function used 'typeof actual === "number"'. So it won't pass when the current time is null.

> Could we just:
> 
> * Make this a regular test()
> * Drop the call to play() and animation.ready
> * Then do the test?
> 
> I think that would be simpler?
> 
> (In fact, I wonder why this current test passes. When we call play() I imagine we resolve the hold time, then I would have expected the setter for the startTime to maintain the hold time.)

OK. I reduced unnecessary parts.

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8754180 [details]
MozReview Request: Bug 1096776 part 1 - Add animation mochitest for examine timeline is null. r?birtles

https://reviewboard.mozilla.org/r/53810/#review50846

::: testing/web-platform/tests/web-animations/interfaces/Animation/reverse.html:154
(Diff revision 2)
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation =
> +   new Animation(new KeyframeEffect(div, null, 100 * MS_PER_SEC));
> +
> +  assert_throws('InvalidStateError', function() { animation.reverse(); }, '');

I don't think you need '' at the end, right?

::: testing/web-platform/tests/web-animations/interfaces/Animation/reverse.html:156
(Diff revision 2)
> +  var animation =
> +   new Animation(new KeyframeEffect(div, null, 100 * MS_PER_SEC));
> +
> +  assert_throws('InvalidStateError', function() { animation.reverse(); }, '');
> +
> +}, 'Reversing an animation without an active timeline throws an InvalidStateError');

Editor settings.

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:3
(Diff revision 2)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>The current time tests</title>

Tests for current time

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:18
(Diff revision 2)
> +  animation.play();
> +  return animation.ready.then(function() {
> +    assert_greater_than_equal(animation.currentTime, 0,
> +      'Current time returns the hold time set when entering the play-pending ' +
> +      'state');
> +  });

Sorry, my comment on this the last time was unclear. We don't need to wait on animation.ready. After calling play() we are in the play-pending state and the hold time is set. We should just assert that currentTime is zero without waiting on ready (and make this a regular test()).

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:32
(Diff revision 2)
> +  var animation =
> +    new Animation(new KeyframeEffect(createDiv(t), null, 100 * MS_PER_SEC),
> +                  null);
> +
> +  return animation.ready.then(function() {
> +    assert_equals(animation.currentTime, null, '');

I don't think we need the ''

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:37
(Diff revision 2)
> +// FIXME: Test that the current time is unresolved when we have an inactive
> +// timeline if we find a way of creating an inactive timeline!
> +test(function(t) {

Nit: Add a blank line between this comment and the test since otherwise it looks like the comment is describing the test.

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:45
(Diff revision 2)
> +  var animation =
> +    new Animation(new KeyframeEffect(createDiv(t), null, 100 * MS_PER_SEC),
> +                  document.timeline);
> +
> +  animation.startTime = null;
> +  assert_equals(animation.currentTime, null, '');

I don't think we need the '' at the end

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:54
(Diff revision 2)
> +  animation.playbackRate = 2;
> +  animation.startTime = document.timeline.currentTime - 25 * MS_PER_SEC;
> +  var timelineTime = document.timeline.currentTime;
> +  var startTime = animation.startTime;
> +  var playbackRate = animation.playbackRate;
> +
> +  assert_times_equal(animation.currentTime,
> +                     (timelineTime - startTime) * playbackRate,
> +                     'Animation has a unresolved start time');

Minor nit: It would be more clear to put the blank line after before the alias variables you're creating. e.g.

  animation.playbackRate = 2;
  animation.startTime = document.timeline.currentTime - 25 * MS_PER_SEC;
  
  var timelineTime = document.timeline.currentTime;
  var startTime = animation.startTime;
  var playbackRate = animation.playbackRate;
  assert_times_equal(animation.currentTime,
                     (timelineTime - startTime) * playbackRate,
                     'Animation has a unresolved start time');


Then you're clearly separating test actions from test assertions.

::: testing/web-platform/tests/web-animations/timing-model/animations/current-time.html:63
(Diff revision 2)
> +}, 'The current time is \'(timeline time - start time) * playback rate\' ' +
> +   'when there is associated timeline and start time is resolved (and ' +
> +   ' no hold time is set)');

I think the following message would be enough:

'The current time is calculated from the timeline time, start time and playback rate'

Comment 18

[Brian Birtles (:birtles)]

https://reviewboard.mozilla.org/r/53448/#review50848

::: testing/web-platform/meta/web-animations/timing-model/animations/set-the-timeline-of-an-animation.html.ini:1
(Diff revision 2)
> +[set-the-timeline-of-an-animation.html]
> +  type: testharness
> +  [Setting the timeline which same previous timeline without an active timeline]
> +    expected: FAIL
> +    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1178662
> +
> +  [Setting the timeline which same previous timeline]
> +    expected: FAIL
> +    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1178662
> +
> +  [Setting the timeline which created using by constructor]
> +    expected: FAIL
> +    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1267510
> +
> +  [Reset animation' pending task after setting null timeline]
> +    expected: FAIL
> +    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1178662

Does this change belong in this patch? I don't think the test in question has landed, right?

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8754180 [details]
MozReview Request: Bug 1096776 part 1 - Add animation mochitest for examine timeline is null. r?birtles

https://reviewboard.mozilla.org/r/53810/#review50850

Comment 20

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8754180 [details]
MozReview Request: Bug 1096776 part 1 - Add animation mochitest for examine timeline is null. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53810/diff/2-3/

Comment 21

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8754178 [details]
MozReview Request: Bug 1096776 part 2 - Modify animation in order to support null timeline. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53448/diff/2-3/

Comment 22

[Mantaroh Yoshinaga[:mantaroh]]

https://reviewboard.mozilla.org/r/53810/#review50846

> Editor settings.

Sorry, I set the editor setting to enable column marker function on c++ mode onlly,, I modified my editor setting.

Comment 23

[Mantaroh Yoshinaga[:mantaroh]]

https://reviewboard.mozilla.org/r/53810/#review50532

> manifest.py appears to be doing silly things here. Do you mind filing a bug and quoting this changeset.
> 
> You can see that many of the lines are the same but the order has changed a lot.

I filed the bug as bug 1274217.

Comment 24

[Mantaroh Yoshinaga[:mantaroh]]

https://reviewboard.mozilla.org/r/53448/#review50848

> Does this change belong in this patch? I don't think the test in question has landed, right?

Sorry, This change is bug 1178662.

Comment 25

[Mantaroh Yoshinaga[:mantaroh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd8a6272d280d895017f37ca50a3d92a68c865ea

Comment 26

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8754180 [details]
MozReview Request: Bug 1096776 part 1 - Add animation mochitest for examine timeline is null. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53810/diff/3-4/

Comment 27

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8754178 [details]
MozReview Request: Bug 1096776 part 2 - Modify animation in order to support null timeline. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53448/diff/3-4/

Comment 28

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #26)
> Comment on attachment 8754180 [details]
> MozReview Request: Bug 1096776 part 1 - Add animation mochitest for examine
> timeline is null. r?birtles
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/53810/diff/3-4/
Rebased patch.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0b200337d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf2927eae67d

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf0b200337d3
https://hg.mozilla.org/mozilla-central/rev/bf2927eae67d

Comment 31

[Chris Mills [:cmills]]

I've indicated that in the Animation() constructor, timelines are optional and can also be set to null.

https://developer.mozilla.org/en-US/docs/Web/API/Animation/Animation#Parameters

I've also included a note in the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/49#Others