1129454 - Add a play/pause all animation button to the animations panel

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Being able to pause all currently running animations in the current page with one button is pretty useful, this avoid having to know which element is animated before hand.

The WebAnimations API provides `document.timeline` and timelines are supposed to have a way to retrieve linked AnimationPlayer objects, but that's not implemented yet.

In the meantime, I think an approach where we just loop through all nodes and pause corresponding animation players from there is acceptable.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug1129454-1-actor-playpause-all-methods.patch

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug1129454-1-actor-playpause-all-methods.patch

Actually, this should be nicer. This exposes a new actor that looks and behaves like the AnimationPlayerActor except it controls all animations, so calling play/pause will play/pause all animations.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug1129454-1-actor-playpause-all-methods v1.patch

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug1129454-2-global-playpause-button v1.patch

Comment 5

[Patrick Brosset <:pbro>]

Attachment: play-pause.gif

With the patches applied, this is what the UI looks like.
You basically get a second button in the animations panel when the selected node isn't animated.
This button allows to play/pause all of the animations at once.

This means that when an animated node is selected, this button isn't displayed anywhere.

I'm out of ideas of where to put this button otherwise. I've been thinking adding a toolbar at the top of the panel to hold this button, so it's always there, but it would look a bit empty at this stage.
Maybe when we get more controls, like playback rate, it would make more sense to have a toolbar for this, but right now, I don't know if it's a good idea.

The feature itself is really useful. There are tons of quick animations that run in webpages and for which you don't necessarily know where is the node in the inspector. So having this global button is handy because you can click it quickly when you know the animation is running, and then select the corresponding element.

Brian: let me know your thoughts about this.

Comment 6

[Patrick Brosset <:pbro>]

Needinfo'ing Jeff too.

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

My comment from the meeting was that it's useful to have access to both the global play/pause button as well as play/pause for an animation on the currently selected node (if there is one)

Comment 8

[Brian Grinstead [:bgrins]]

As far as the UI goes, I agree that it'd be good to see a global play / pause at all times, and a single element play / pause when it's possible.  I don't have any bright ideas about the UX though.  I think adding a toolbar to the top that said "All animations: ▶" seems reasonable.

Comment 9

[Patrick Brosset <:pbro>]

Cool, thanks for the feedback. I opted for a bottom toolbar, to be consistent with the computed-view toolbar (and the soon to come rule-view filtering, which I think is going to be at the bottom too).
I need to add some tests and will then upload patches.

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #9)
> Cool, thanks for the feedback. I opted for a bottom toolbar, to be
> consistent with the computed-view toolbar (and the soon to come rule-view
> filtering, which I think is going to be at the bottom too).
So, apparently, the rule-view is going to have a top toolbar instead. It will contain the search tool and pseudo-class lock mode too.
So it makes more sense to have the animation toolbar at the top too.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug1129454-1-actor-playpause-all-methods v2.patch

New playAll/pauseAll methods at actor level + tests.
This should be ready for review.
The patch is pretty simple. The only thing to know about this is that to toggle all animations, it iterates through all DOM nodes, in all current windows and get animationPlayer objects from there.
Once we get access, using the WebAnimations API, to the document timeline's player, we'll be able to simplify this a lot.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug1129454-2-global-playpause-button v2.patch

And that's the UI part. It adds a persistent top toolbar to the panel that has (for now) just one button and a label.

One detail worth mentioning: if there are player widgets displayed in the panel for ongoing animations: instead of sending the request to pause all animations and then wait for the individual widgets to pause when receiving their new states, they are paused first. This makes the UI feel more snappy.

Try build for both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c3cb7fbdc2f

Comment 13

[Victor Porof [:vporof][:vp]]

Comment on attachment 8560838 [details] [diff] [review]
bug1129454-2-global-playpause-button v2.patch

Review of attachment 8560838 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/animationinspector/animation-controller.js
@@ +186,5 @@
>      done();
>    }),
>  
> +  /**
> +   * Not all server versions provide a way to pause all animations at once.

Maybe use the framework level feature detection from bug 1069673 here, instead of adding a new trait. Sync with Jordan for details.

Comment 14

[Patrick Brosset <:pbro>]

Attachment: bug1129454-2-global-playpause-button v3.patch

Thanks Victor, I had forgotten about this new API.
Updated the patch.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: bug1129454-1-actor-playpause-all-methods v3.patch

I removed the new trait added to root.js since the front-end doesn't need it anymore.

Comment 16

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

Comment on attachment 8562118 [details] [diff] [review]
bug1129454-1-actor-playpause-all-methods v3.patch

Review of attachment 8562118 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the following fixed.

::: toolkit/devtools/server/actors/animation.js
@@ +434,5 @@
> +   * Iterates through all nodes in all of the tabActor's window documents and
> +   * finds all existing animation players.
> +   * This is currently used to allow playing/pausing all animations at once
> +   * until the WebAnimations API provides a way to play/pause via the document
> +   * timeline. <Bug reference needed here>

Does bug 1123524 not cover that?

@@ +444,5 @@
> +    // Typically, there will be very few windows, and getElementsByTagName is
> +    // really fast even on large DOM trees.
> +    for (let window of this.tabActor.windows) {
> +      let root = window.document.body || window.document;
> +      for (let element of root.getElementsByTagName("*")) {

This will miss elements in namespaced documents. You should use this instead:
for (let element of root.getElementsByTagNameNS("*", "*")) {

Comment 17

[Patrick Brosset <:pbro>]

Attachment: bug1129454-1-actor-playpause-all-methods v4.patch

Thanks Mike, patch updated as per your comments.

Comment 18

[Patrick Brosset <:pbro>]

Attachment: bug1129454-2-global-playpause-button v4.patch

Fixed a test failure.

Pushed the 2 patches to fx-team:

remote:   https://hg.mozilla.org/integration/fx-team/rev/b04a0da319b6
remote:   https://hg.mozilla.org/integration/fx-team/rev/ecd8e5e79346

Most recent try push was: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c3cb7fbdc2f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b04a0da319b6
https://hg.mozilla.org/mozilla-central/rev/ecd8e5e79346

Comment 20

[Stefan Plewako [:stef]]

> https://hg.mozilla.org/mozilla-central/rev/ecd8e5e79346

What was the point in touching almost every line in browser/locales/en-US/chrome/browser/devtools/animationinspector.dtd file, uniquely changing its license header and adding files with non MPL2 licenses to tests?