1197100 - Background work for displaying keyframes in the animation-inspector

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

In the animation-inspector (v3), it should be possible to select an animation from the list to inspect its keyframes.

Selecting an animation could either replace the whole sidebar content with a new panel, or split it in 2 to have the keyframes panel at the bottom.
Alternatively, it could expand an area just below the animation to show information there, along time.

This requires platform work to get the keyframes info from the WebAnimations API.

Comment 1

[Patrick Brosset <:pbro>]

Bug 1198708 has progressed sufficiently for me to try out the new platform API.
let frames = el.getAnimations()[0].effect.getFrames();
This returns an array of frame objects. Each object has an offset (and computedOffset) which lets us know the position of the keyframe. Each object also has a list of animated properties and their corresponding values.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_1_-_Add_getFrames_method_to_the_Anim.diff

Part 1 - Just adds the getFrames method to the actor, and a test.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_2_-_Rename_all_animation_actor_tests.diff

Part 2 - I was getting tired of this useless test naming convention:
browser_animation_actor_01.js
I came up with it in the first place, but now realize that it was a bad idea because there are many tests now, and it should be possible to know what a test does just by looking at the file name.
So this patch is just a big `hg mv`

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_3_-_Remove_code_duplication_in_anima.diff

Part 3 - This part is also related to tests. The server-side animation actor tests shared a lot of code. This patch moves the common code to head.js instead.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_4_-_Move_the_time-block_UI_code_to_i.diff

Part 4 - This is just a code refactoring, so there's no new tests for this. The existing tests should cover this change.
The original idea when I created the animation-inspector UI was to organize its bits and pieces into small UI components that I could init/render/destroy easily.
And that's what I did, but then I created the Timeline component, which is the central piece, and it sort of grew out of control and it's now just one big component that does a lot of things.
This patch breaks away one crucial part: the time-block part (the thing that displays a single animation).

Comment 6

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_5_-_Make_the_time-block_component_cl.diff

Part 5 - This parts adds the first UI piece needed to display keyframes. This makes lines (animations) in the timeline clickable. When you click on an animation, it becomes selected: it gets a class applied to apply a different background color, and it emits an event.
This event isn't yet used in this part. Future parts will make use of it.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_6_-_Simplify_TimeScale_method_signat.diff

Part 6 - This is just a simple refactor to avoid having to pass a second argument to many TimeScale methods all over the place. The TimeScale object contains several methods to translate time to width and width to time, and this relies on knowing the total width of the container.
In this patch, I just set that width once and for all so that when calling the translation methods later, it doesn't need to be passed again.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_4_-_Move_the_time-block_UI_code_to_i.diff

Sorry for the noise, some left-over I forgot to clean up in part 4.

Comment 9

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_6_-_Simplify_TimeScale_method_signat.diff

Rebased part 6 on top of the new part 4.

Comment 10

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_7_-_Add_an_animationDetails_componen.diff

Part 7 is still a WIP, not up for review. It lacks tests, and so far it's more of a UI concept than anything else.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_5_-_Make_the_time-block_component_cl.diff

Rebased part 5.

Comment 12

[Patrick Brosset <:pbro>]

Here is a screencast of the UI so far. Pretty simple. There are other ways to display this, but I chose this one for a reason: displaying the list of keyframes and properties in a separate panel at the bottom allows that panel to be full-width and therefore makes it easier to see the frames.

The panel contains 2 things:
- a keyframes diagram
- a list of animated properties that is displayed when a keyframe is selected in the diagram

Unfortunately, I didn't have the time to create a nice demo page for this that has more interesting animations with multiple keyframes and multiple animated properties. So in this video, you'll see animations that have just 2 keyframes (from - to), and for each of them, just one animated property. So, pretty simple stuff. But the UI would work with multiple keyframes and properties just the same.

The other option I investigated was displaying the keyframes just below each animation, inside the timeline. But I finally rejected this because when you have many animations displayed, each with varying durations, some of them might be pretty small in there, just a few pixels wide, and displaying the keyframes on there would be hard. I think that this could be a good solution if we implemented zooming/panning in the timeline

The video: https://youtu.be/6BjAjVCq3js

Helen: I'd love to have your thoughts on this concept. Note that this isn't urgent, feel free to put that somewhere low in your TODO list if you need to. I'm not going to be able to work on this in the coming 3 weeks or so anyway.

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8668990 [details] [diff] [review]
Bug_1197100_-_5_-_Make_the_time-block_component_cl.diff

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

Looks good, though note that the code changes described in comment #6 don't
appear in this version of the patch.

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8668975 [details] [diff] [review]
Bug_1197100_-_4_-_Move_the_time-block_UI_code_to_i.diff

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

Looks good.

Comment 15

[Patrick Brosset <:pbro>]

(In reply to Tom Tromey :tromey from comment #13)
> note that the code changes described in comment #6 don't
> appear in this version of the patch.
Something really weird happened with parts 4 and 5, I'll just fold them together, that will be easier.

Comment 16

[Patrick Brosset <:pbro>]

Pending try build for parts 1 to 6 (really 1 to 5 in fact, because I've folded 4 and 5): https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd1c73647178

Comment 17

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: rules area design language.png

Comment 18

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: Screen Shot 2015-10-06 at 9.22.34 AM.png

Comment 19

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #12)
> The video: https://youtu.be/6BjAjVCq3js
> 
> Helen: I'd love to have your thoughts on this concept. Note that this isn't
> urgent, feel free to put that somewhere low in your TODO list if you need
> to. I'm not going to be able to work on this in the coming 3 weeks or so
> anyway.

So, I've only looked at the video and not a build, so some of my questions hinge on that:

Is it possible to edit those values? So, turn rotate(40deg) into skew(20deg)? Right now they /seem/ editable, so my advice changes based on whether or not they are:

IF THEY ARE: might be a good idea to make them feel more familiar by sharing some of the design language/colors of the rules area, since it seems like they act the same way. (I'd also left-align the css property with the 0%/0s, right now it seems like they're a little askew.) --->>> see comment 17

IF NOT: they should probably be grayed out so that's evident.

Other thoughts: might be nice if they are editable to expand all of the transforms into longhand if you have a lot of stuff going on, like: (rotate(30deg) skew(40deg) translateY(90deg)). For debugging!

Also, perhaps you can tell me more about that bar in comment 18, but to me it seems like we're losing a fair amount of space for that pause/play button. (Not that we don't need one, but I'm curious why it's living in a gray bar the way it is.)

Comment 20

[Patrick Brosset <:pbro>]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #19)
> Is it possible to edit those values? So, turn rotate(40deg) into
> skew(20deg)? Right now they /seem/ editable, so my advice changes based on
> whether or not they are:
They're not editable for now, but the plan for later is to make them editable.
 
> IF THEY ARE: might be a good idea to make them feel more familiar by sharing
> some of the design language/colors of the rules area, since it seems like
> they act the same way. (I'd also left-align the css property with the 0%/0s,
> right now it seems like they're a little askew.) --->>> see comment 17
Great point. I'll change this.

> Other thoughts: might be nice if they are editable to expand all of the
> transforms into longhand if you have a lot of stuff going on, like:
> (rotate(30deg) skew(40deg) translateY(90deg)). For debugging!
Like the rule-view you mean?
We should almost reuse a filtered down version of the rule-view here in fact, but one that only shows the animated properties.

> Also, perhaps you can tell me more about that bar in comment 18, but to me
> it seems like we're losing a fair amount of space for that pause/play
> button. (Not that we don't need one, but I'm curious why it's living in a
> gray bar the way it is.)
Yes it does look like it uses a lot of space for nothing, but the long term plan is to use the toolbar for other things as well:
- rewind (bug 1205681)
- play/pause
- change the playback rate (bug 1211801)
- display the current time (bug 1199589)

Comment 21

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #20)
> They're not editable for now, but the plan for later is to make them
> editable.
Can we gray them out in the interim? It's a bit confusing otherwise.

> > Other thoughts: might be nice if they are editable to expand all of the
> > transforms into longhand if you have a lot of stuff going on, like:
> > (rotate(30deg) skew(40deg) translateY(90deg)). For debugging!
> Like the rule-view you mean?
> We should almost reuse a filtered down version of the rule-view here in
> fact, but one that only shows the animated properties.
Yes, that sounds great!

Comment 22

[Patrick Brosset <:pbro>]

Landed the first 3 parts (try is green) to avoid bitrot:

remote:   https://hg.mozilla.org/integration/fx-team/rev/4dc3da9ae19f
remote:   https://hg.mozilla.org/integration/fx-team/rev/84af369e2aef
remote:   https://hg.mozilla.org/integration/fx-team/rev/598ca49e0db1

Comment 23

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_4_-_Move_time-block_UI_to_its_own_co.diff

Folded parts 4 and 5 together. Carried over R+ from Tom.

Comment 24

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_5_-_Simplify_TimeScale_method_signat.diff

Rebased. Carried over R+ from Mike.

Comment 25

[Patrick Brosset <:pbro>]

Comment on attachment 8668983 [details] [diff] [review]
Bug_1197100_-_7_-_Add_an_animationDetails_componen.diff

Marking as obsolete this last WIP part.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4dc3da9ae19f
https://hg.mozilla.org/mozilla-central/rev/84af369e2aef
https://hg.mozilla.org/mozilla-central/rev/598ca49e0db1

Comment 27

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_4_-_Move_time-block_UI_to_its_own_co.diff

I've split part 4 in 2 parts: 1) moving the TimeBlock thing in its own component, and 2) making it selectable.
I don't want to land 2) in 44 , but I do want 1) to go in as soon as possible as other patches are based on it.
It's been R+'d before and try was green, so I'll check this new part 4 in, and I'll upload the remaining part here in a bit.

Comment 28

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_5_-_Make_time_blocks_selectable.diff

This is the second half of what used to be part 4.
Carrying R+ over for this too.

Comment 29

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_6_-_Simplify_TimeScale_method_signat.diff

And this used to be part 5. Was R+'d too.

Comment 30

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_7_-_Add_an_animationDetails_componen.diff

And for those interested, this is the last part, which actually adds a keyframes diagram at the bottom of the panel when you select an animation + a property table when you click on a keyframe.
This is still a WIP.
This is what was used to produce the video in comment 8.

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8675074 [details] [diff] [review]
Bug_1197100_-_4_-_Move_time-block_UI_to_its_own_co.diff

https://hg.mozilla.org/integration/fx-team/rev/a6d52da2c539

Comment 32

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/72d238481e4b

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/52f7eb6257e0

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/52f7eb6257e0

Comment 35

[Patrick Brosset <:pbro>]

The last part of this bug is not a simple one, we need to come up with some UI to display keyframes and animated properties. Started this to discuss about it: https://docs.google.com/document/d/1IGfzNcJOGgHcpyQURV3iNx_aGMM0aPNgeACljVuWEKk/edit

Comment 36

[Patrick Brosset <:pbro>]

This bug has been going on for too long.
Let's make this a preparation bug, needed to display the keyframes.
With this in mind, part 5 can land here, it's been R+'d a long time ago, I'm just doing one last try push to verify and then I'll land it.

Part 6 is useless knowing that I'm working on bug 1171863.
And I'll move part 7 in a new bug.

Comment 37

[Patrick Brosset <:pbro>]

Attachment: Bug_1197100_-_5_-_Make_time_blocks_selectable__r_t.diff

Rebased part 5.
It has already been R+'d some time ago by Tom.
Try is happy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8d5ff492693&selectedJob=14108521

So let's land this last patch and close this bug.

Comment 38

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/f9798fa51643

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9798fa51643