Closed Bug 1129454 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(2 files, 9 obsolete files)

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.
Blocks: 1120900
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
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.
Attachment #8559173 - Attachment is obsolete: true
Attachment #8559226 - Attachment is obsolete: true
Attached image play-pause.gif (obsolete) —
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.
Flags: needinfo?(bgrinstead)
Needinfo'ing Jeff too.
Flags: needinfo?(jgriffiths)
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)
Flags: needinfo?(jgriffiths)
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.
Flags: needinfo?(bgrinstead)
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.
(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.
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.
Attachment #8559753 - Attachment is obsolete: true
Attachment #8559757 - Attachment is obsolete: true
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
Attachment #8559754 - Attachment is obsolete: true
Attachment #8560837 - Flags: review?(mratcliffe)
Attachment #8560838 - Flags: review?(vporof)
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.
Attachment #8560838 - Flags: review?(vporof) → review+
Thanks Victor, I had forgotten about this new API.
Updated the patch.
Attachment #8560838 - Attachment is obsolete: true
Attachment #8562114 - Flags: review+
I removed the new trait added to root.js since the front-end doesn't need it anymore.
Attachment #8560837 - Attachment is obsolete: true
Attachment #8560837 - Flags: review?(mratcliffe)
Attachment #8562118 - Flags: review?(mratcliffe)
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("*", "*")) {
Attachment #8562118 - Flags: review?(mratcliffe) → review+
Thanks Mike, patch updated as per your comments.
Attachment #8562118 - Attachment is obsolete: true
Attachment #8563403 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b04a0da319b6
https://hg.mozilla.org/mozilla-central/rev/ecd8e5e79346
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Depends on: 1132783
> 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?
Blocks: 1137129
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.