1155661 - Add playback control buttons to the top toolbar in the animation inspector

Status: VERIFIED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

The animation inspector will show more animations with bug 1153271, and specifically, animations that are often sequenced together.

In this context, it makes sense for the top toolbar (now only used for the global play/pause button) to become the toolbar that can control all currently displayed animation widgets.

I can see 2 cases:

- there are currently no animations displayed inside the sidebar:

in this case, the top toolbar should contain the global play/pause button, like today + a global playback rate select to set the playback rate for *all* animations on the page.

- there are animations displayed in the sidebar:

in that case, the controls in the top toolbar become contextual to the animations currently displayed and allow: play/pause, playback rate selection, rewind, fast-forward.

See the mockup in bug 1153271 comment 1.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: animation-inspector-v3-pause.gif

Status update: I'm currently working on a patch series to introduce a play/pause button above the timeline. This button will pause all animations currently displayed in the timeline. Clicking again will resume. Once the scrubber has reached the end of the timeline (i.e. all animations are finished), clicking again will start again from the beginning.
Attaching an animated GIF of my current work in progress.
I should start attaching patches for review soon.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug_1155661_-_1_-_Rename_toolbar_to_global-toolbar.diff

Part 1 - Just a simple rename of the existing animation-inspector toolbar

Why? Because the mechanism I've chosen to implement the new toolbar is:
- the global toolbar that exists today is there for global buttons, things that impact *all* animations in the page (and in nested iframes too), whether or not they are displayed in the timeline at this stage,
- whereas timeline-related toolbar buttons are going to impact only those animations currently displayed in the timeline, and the set of buttons may be different from the global ones.

Therefore, the next patch will introduce a second toolbar element (hence the need to change the current one's name), and it will substitute the global toolbar element whenever there are animations displayed in the timeline.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug_1155661_-_2_-_Add_a_new_timeline_toolbar_shown.diff

Part 2 - Actually adds the new timeline toolbar, and implements the mechanism that shows/hides the right toolbar depending on the presence of animations in the UI.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug_1155661_-_3_-_Add_a_way_to_play_pause_the_curr.diff

Part 3 - This adds a new method to the AnimationsActor that allows to play/pause a number of AnimationPlayers at once. This isn't used in this patch yet, will be useful for later parts. I've added a test for this.

The reason for adding this is that if you just pause/play all animations from the front-end, by sending requests one by one, that's going to offset the animations by some amount of time (due to the time it takes the request to reach the server).

Comment 5

[Patrick Brosset <:pbro>]

Attachment: Bug_1155661_-_4_-_Add_the_timeline_play_pause_butt.diff

Part 4 - This part only adds the new play/pause button in the timeline toolbar, but doesn't add any behavior to it. Later parts will take care of this.

This part however changes some of panel's CSS to be able to reuse styles from the pause button that was already defined.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: Bug_1155661_-_5_-_Make_the_timeline_emit_data_abou.diff

Part 5 - This part implements one side of the play/pause button logic, that is, when the user clicks in the timeline and moves the scrubber around, the button should react to this and change its state accordingly.
When the scrubber is moving (i.e. animations are playing), it should show a pause icon.
After the scrubber has been moved around (i.e. animations are now paused), it should show a play icon.

The other side of the logic is when the button is clicked, we want the animations to play/pause. The next patch will take care of this.

Comment 7

[Patrick Brosset <:pbro>]

I will upload the final part (part 6) tomorrow after I've finalized a new test I'm adding.
In the meantime, here's a try push for parts 1 to 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79d82d1637bc

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8661799 [details] [diff] [review]
Bug_1155661_-_4_-_Add_the_timeline_play_pause_butt.diff

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

Looks reasonable

::: browser/themes/shared/devtools/animationinspector.css
@@ +96,2 @@
>  
> +#global-toolbar button,

Maybe this should be .devtools-button instead of button to match the selectors below

Comment 9

[Patrick Brosset <:pbro>]

Attachment: Bug_1155661_-_6_-_Implement_the_behavior_behind_th.diff

Part 6 - This is the last part. It implements the 2nd part of the play/pause logic, which is the mechanism that actually plays/pauses animations when the toolbar button is pressed.

Note that when I first coded the panel, I chose to implement parts of the UI as components. Components have a render method and are rendered everytime a piece of the state that they are interested in changes.
So to not break this convention, what happens when the button is clicked is that the animations are played/paused on the server, then the corresponding fronts are just refreshed and the timeline component is re-rendered. This helps keep the UI update mechanism simple.

Another interesting thing to note is: when animations are paused (using the web animations API), their startTime becomes null (this is something defined by the API's spec). Because of this, if you click on the button, then refresh the animations' states, and then re-render the timeline, you can't position the animations anymore in the UI.
What I ended up doing here is add a 'previousStartTime' field to the animations' states which is always equal to 'startTime' as long as it isn't null. If it is null, then 'previousStartTime' is just the last known value.

Comment 10

[Patrick Brosset <:pbro>]

Try push for parts 1 to 6: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ef43a5f06e0

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8661796 [details] [diff] [review]
Bug_1155661_-_2_-_Add_a_new_timeline_toolbar_shown.diff

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

Looks good to me! Tested also locally.

Comment 12

[Patrick Brosset <:pbro>]

Rebased, addressed review comments, and landed on fx-team:

remote:   https://hg.mozilla.org/integration/fx-team/rev/204ecb6628ce
remote:   https://hg.mozilla.org/integration/fx-team/rev/6c3532823c25
remote:   https://hg.mozilla.org/integration/fx-team/rev/b8d10d58218e
remote:   https://hg.mozilla.org/integration/fx-team/rev/0c461fc0a823
remote:   https://hg.mozilla.org/integration/fx-team/rev/fb954e0271ec
remote:   https://hg.mozilla.org/integration/fx-team/rev/bb283bb54d56

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/204ecb6628ce
https://hg.mozilla.org/mozilla-central/rev/6c3532823c25
https://hg.mozilla.org/mozilla-central/rev/b8d10d58218e
https://hg.mozilla.org/mozilla-central/rev/0c461fc0a823
https://hg.mozilla.org/mozilla-central/rev/fb954e0271ec
https://hg.mozilla.org/mozilla-central/rev/bb283bb54d56

Comment 14

[Alfred Kayser]

Why add another button, when only one of the global or timeline buttons is shown at the same time?

Comment 15

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to test the functionality of the Play/Pause button from the top toolbar on Nightly 44.0a1 (2015-10-20) across platforms [1].
I can confirm that:
- a Play/Pause button is displayed if there are no animations available inside the sidebar and another Play/Pause button if there are animations available in the sidebar.
- the button shows the correct icon, according to the animation's state (paused, playing)
- the scrubber can be manually moved. While this action is performed, the button shows the Play icon.
- if clicking the pause button, the animation, the scrubber and the time are paused.

[1]Windows 10 x86, Ubuntu 13.10 x64 and Mac OS X 10.10.4