1120833 - Refresh the list of animations in the Animations Panel when animations are created.

Status: VERIFIED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

For now the animations panel displays the list of AnimationPlayer objects for a node at the time the node is selected.
If one of the animations ends, the UI widget for its player will be removed from the panel *but* if a new animations is created, it won't show up in the list. You'd have to select another node first and then re-select the same node again to refresh the list.

Comment 1

[Patrick Brosset <:pbro>]

Cameron, Brian Birtles said I should talk to you with regards to waapi events. I think have an event that tells us when the list of AnimationPlayers for a node changes would be great.

For info, our devtools actor works this way for now:
- the devtools UI selects a node, and using this node requests the list of AnimationPlayers with this method: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/animation.js#344
- this returns a list of AnimationPlayerActor objects (http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/animation.js#46), which essentially are just wrappers on waapi AnimationPlayer objects.

One solution would be, whenever the method getAnimationPlayersForNode(node) on the AnimationsActor is called, add an event listener on "new-player" (or whatever we end up calling this new event) on node, and whenever it happens, the actor would instantiate a new AnimationPlayerActor and send it to the UI.

So we'd need this "new-player" event.

Let me know what you think.

Comment 2

[Cameron McCormack (:heycam)]

I think it makes sense to have a notification when a new AnimationPlayer gets created or when the list of AnimationPlayers changes.  Either way.  I've filed bug 1123523 for the overall notification/observer mechanism for Web Animations API state changes, and bug 1123524 for the specific notification you need here.

Comment 3

[Patrick Brosset <:pbro>]

Thanks a lot Cameron!

Comment 4

[Patrick Brosset <:pbro>]

I have a set of patches ready for this which I'll upload in a minute.
One note before though:
Having animations show up automatically when they are added also works for transitions, but transitions aren't supported as nicely right now, so there will need to be follow-up bugs:

- Modifying finished transitions doesn't work, see bug 1149990.
- Transitions are added multiple times (since the underlying Animation object is different every time). So we need reconciliation, see bug 1149999.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://1120833/pbrosset

/r/6463 - Bug 1120833 - 1 - Remove EventEmitter usage in AnimationPlayerFront
/r/6465 - Bug 1120833 - 2 - Fire events about changed animations in the AnimationsActor
/r/6467 - Bug 1120833 - 3 - Refresh the list of animation widgets when new animations are added
/r/6469 - Bug 1120833 - 4 - Tests for animation inspector refresh on addition/removal

Pull down these commits:

hg pull -r 1f53813689857d002c5f63db8bf46cd86cae012f https://reviewboard-hg.mozilla.org/gecko/

Comment 6

[Patrick Brosset <:pbro>]

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

Comment 7

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/6463/#review5353

I like the old API more, but these changes are fine.  I commented on Bug 952653 about wanting .once to return a promise.

::: browser/devtools/animationinspector/test/head.js
(Diff revision 1)
> +  player.once(player.AUTO_REFRESH_EVENT, onRefresh.resolve);

The EventTarget .once function doesn't return a promise?  That's unfortunate..

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Patrick, do you need me to review anything here? I see an r? in bugzilla, but ReviewBoard says the change is discarded.

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Panos Astithas [:past] from comment #8)
> Patrick, do you need me to review anything here? I see an r? in bugzilla,
> but ReviewBoard says the change is discarded.
As discussed on IRC, this happened because I pushed a review, then realized there was a typo, cancelled the review, then fixed the typo (edited the individual commit by committing, rebasing and histediting), and pushed to review again. Everything looked fine on the reviewboard UI, but you're right, reviewboard seems to think this commit is discarded. Can you try to review it on reviewboard still? If that doesn't work I can try and push again.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

https://reviewboard.mozilla.org/r/6465/#review5377

::: toolkit/devtools/server/actors/animation.js
(Diff revision 1)
> -    let actors = [];
> +    // No care is taken here to destroyed the previously stored actors because

Typo: "to destroy"

::: toolkit/devtools/server/actors/animation.js
(Diff revision 1)
> +        // are finished and not filling forward.

Nit: "moving forward" maybe?

::: toolkit/devtools/server/tests/browser/browser_animation_actors_13.js
(Diff revision 1)
> +  is(changes[0].player.initialState. name, "move",

There appears to be a space between dot and "name".

::: toolkit/devtools/server/tests/browser/browser_animation_actors_13.js
(Diff revision 1)
> +  is(changes[1].player.initialState. name, "glow",

Same here.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8586793 [details]
MozReview Request: bz://1120833/pbrosset

https://reviewboard.mozilla.org/r/6461/#review5379

Ship It!

Comment 12

[Patrick Brosset <:pbro>]

Pushed part 1 to fx-team: https://hg.mozilla.org/integration/fx-team/rev/7bbdae4751bc

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8586793 [details]
MozReview Request: bz://1120833/pbrosset

https://reviewboard.mozilla.org/r/6461/#review5381

::: toolkit/devtools/server/tests/browser/browser_animation_actors_13.js
(Diff revision 1)
> +  is(changes[0].player.initialState. name, "move",

is(changes[0].player.initialState. name, "move",

Should be:

is(changes[0].player.initialState.name, "move",

Also:

is(changes[1].player.initialState. name, "glow",

Should be:

is(changes[1].player.initialState.name, "glow",

With those changes r+

Comment 14

[Patrick Brosset <:pbro>]

part 2 in fx-team after rebase and corrections as per Panos' feedback: https://hg.mozilla.org/integration/fx-team/rev/0b0353d78365

Comment 15

[Patrick Brosset <:pbro>]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #13)
> Comment on attachment 8586793 [details]
> MozReview Request: bz://1120833/pbrosset
> 
> https://reviewboard.mozilla.org/r/6461/#review5381
> 
> ::: toolkit/devtools/server/tests/browser/browser_animation_actors_13.js
> (Diff revision 1)
> > +  is(changes[0].player.initialState. name, "move",
> 
> is(changes[0].player.initialState. name, "move",
> 
> Should be:
> 
> is(changes[0].player.initialState.name, "move",
> 
> Also:
> 
> is(changes[1].player.initialState. name, "glow",
> 
> Should be:
> 
> is(changes[1].player.initialState.name, "glow",
> 
> With those changes r+
Thanks Mike, but Panos has already reviewed this commit (see comment 10).
I seems like reviewboard doesn't make it very simple to know what has been reviewed and what has not.

Comment 16

[Patrick Brosset <:pbro>]

Looking at the bug and reviewboard, I wasn't sure what had been and hadn't been reviewed, so I checked with Mike on IRC and he did review the whole thing.
So here are parts 3 and 4 in fx-team:

remote:   https://hg.mozilla.org/integration/fx-team/rev/501fa7c170ed
remote:   https://hg.mozilla.org/integration/fx-team/rev/2fe22ffef154

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1150615.
https://hg.mozilla.org/integration/fx-team/rev/0cb8a23d435d

Comment 18

[Patrick Brosset <:pbro>]

So, parts 3 and 4 have been backed out, because of one of the tests I added which fails on windows.
I've gone through the test logs and code, and I have a pretty simple way to fix it.
But I've relanded the fixes for bug 1134500 and bug 1120833 which had been part of the group backout, and so I'll wait for this to stick first.

Comment 19

[Patrick Brosset <:pbro>]

Attachment: 4 - Tests for animation inspector refresh on addition/removal

Comment 20

[Patrick Brosset <:pbro>]

Try push with the new parts 3 and 4: https://treeherder.mozilla.org/#/jobs?repo=try&revision=641a48df4fa5

Comment 21

[Patrick Brosset <:pbro>]

Re-landed parts 3 and 4:

remote:   https://hg.mozilla.org/integration/fx-team/rev/48511c3e78fd
remote:   https://hg.mozilla.org/integration/fx-team/rev/e2a6360204f1

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/48511c3e78fd
https://hg.mozilla.org/mozilla-central/rev/e2a6360204f1

Comment 24

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1120833 - 2 - Fire events about changed animations in the AnimationsActor

Comment 25

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1120833 - 3 - Refresh the list of animation widgets when new animations are added

Comment 26

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1120833 - 4 - Tests for animation inspector refresh on addition/removal

Comment 27

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1120833 - 1 - Remove EventEmitter usage in AnimationPlayerFront

Comment 28

[Mihai Boldan, Desktop QA [:mboldan]]

Is there any proper way of manually testing this?

Comment 29

[Patrick Brosset <:pbro>]

Attachment: animation.html

To test this:
- open this test HTML page in firefox
- open the animation-inspector
- click on the 2 circles to toggle the animations on them
- every time you do, the animation-inspector should refresh to display the current list of animations

Comment 30

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: video.mov

Using STR from Comment 29, I managed to Verified this feature on Nightly 44.0a1 (2015-10-29) across platforms.
While testing I observed a potential issue:
STR:
- open this test HTML page from Comment 29 in Firefox 
- open the animation-inspector
- stop the animations 
- click on one circle in order to toggle the animation
- pause the animation by clicking on the elapsed time bar
- click on the other circle in order to toggle the animation
- click again on the same circle in order to stop the animation.

AR:
If the second animation is stopped after the scrubber is at the end, the scrubber from the first animation is played, but the animation is paused.
If the second animation is stopped before the scrubber is at the end, the scrubber from the firs animation is missing (See the attached video).

Note that the issue is not reproducible if the Pause button is used for pausing the animation.

Comment 31

[Patrick Brosset <:pbro>]

(In reply to Mihai Boldan, QA [:mboldan] from comment #30)
> Created attachment 8681149 [details]
> video.mov
> 
> Using STR from Comment 29, I managed to Verified this feature on Nightly
> 44.0a1 (2015-10-29) across platforms.
> While testing I observed a potential issue:
> STR:
> - open this test HTML page from Comment 29 in Firefox 
> - open the animation-inspector
> - stop the animations 
> - click on one circle in order to toggle the animation
> - pause the animation by clicking on the elapsed time bar
> - click on the other circle in order to toggle the animation
> - click again on the same circle in order to stop the animation.
> 
> AR:
> If the second animation is stopped after the scrubber is at the end, the
> scrubber from the first animation is played, but the animation is paused.
> If the second animation is stopped before the scrubber is at the end, the
> scrubber from the firs animation is missing (See the attached video).
> 
> Note that the issue is not reproducible if the Pause button is used for
> pausing the animation.
Could you please file a bug for these issues?
They come from the fact that we don't deal with mixed-playstate animations in the tool yet. So it's an improvement we need to make in future versions, but it's not an easy one.
Bug 1170159 is partly about this, but I think it makes sense to file a bug with your STRs and video just to be sure we don't loose this info. Could you maybe link the 2 bugs with the "see also" input?

Comment 32

[Mihai Boldan, Desktop QA [:mboldan]]

I've logged Bug 1221074 for the issue found while testing this bug, so I'm marking this issue Verified Fix.