Closed Bug 1120833 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set

Tracking

(firefox40 fixed)

VERIFIED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(7 files, 1 obsolete file)

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.
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.
Flags: needinfo?(cam)
Blocks: 985861
Blocks: 1120900
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.
Thanks a lot Cameron!
Flags: needinfo?(cam)
Depends on: 1123523
No longer depends on: 1123524
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Blocks: 1149999
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.
/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/
Attachment #8586793 - Flags: review?(past)
Attachment #8586793 - Flags: review?(mratcliffe)
Attachment #8586793 - Flags: review?(bgrinstead)
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..
Attachment #8586793 - Flags: review?(bgrinstead) → review+
Patrick, do you need me to review anything here? I see an r? in bugzilla, but ReviewBoard says the change is discarded.
(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.
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.
Attachment #8586793 - Flags: review?(past) → review+
Keywords: leave-open
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+
Attachment #8586793 - Flags: review?(mratcliffe) → review+
part 2 in fx-team after rebase and corrections as per Panos' feedback: https://hg.mozilla.org/integration/fx-team/rev/0b0353d78365
(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.
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
Keywords: leave-open
Depends on: 1150615
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.
https://hg.mozilla.org/mozilla-central/rev/48511c3e78fd
https://hg.mozilla.org/mozilla-central/rev/e2a6360204f1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Attachment #8586793 - Attachment is obsolete: true
Attachment #8619113 - Flags: review+
Attachment #8619114 - Flags: review+
Attachment #8619115 - Flags: review+
Attachment #8619116 - Flags: review+
Is there any proper way of manually testing this?
Flags: needinfo?(pbrosset)
Attached file 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
Flags: needinfo?(pbrosset)
Attached video 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.
Flags: needinfo?(pbrosset)
(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?
Flags: needinfo?(pbrosset)
I've logged Bug 1221074 for the issue found while testing this bug, so I'm marking this issue Verified Fix.
Status: RESOLVED → VERIFIED
See Also: → 1170159, 1221074
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.