Closed Bug 1197100 Opened 5 years ago Closed 4 years ago

Background work for displaying keyframes in the animation-inspector

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

(Whiteboard: [devtools-platform])

Attachments

(7 files, 12 obsolete files)

5.16 KB, patch
miker
: review+
pbro
: checkin+
Details | Diff | Splinter Review
6.99 KB, patch
miker
: review+
pbro
: checkin+
Details | Diff | Splinter Review
45.21 KB, patch
miker
: review+
pbro
: checkin+
Details | Diff | Splinter Review
20.67 KB, image/png
Details
199.51 KB, image/png
Details
10.76 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
9.43 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
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.
Blocks: 1153271
No longer blocks: 1153271
Blocks: 1201279
Depends on: 1198708
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.
Assignee: nobody → pbrosset
Blocks: 1210793
Blocks: 1210795
Blocks: 1210796
Part 1 - Just adds the getFrames method to the actor, and a test.
Attachment #8668946 - Flags: review?(mratcliffe)
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`
Attachment #8668947 - Flags: review?(mratcliffe)
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.
Attachment #8668948 - Flags: review?(mratcliffe)
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).
Attachment #8668950 - Flags: review?(ttromey)
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.
Attachment #8668964 - Flags: review?(ttromey)
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.
Attachment #8668970 - Flags: review?(mratcliffe)
Sorry for the noise, some left-over I forgot to clean up in part 4.
Attachment #8668950 - Attachment is obsolete: true
Attachment #8668950 - Flags: review?(ttromey)
Attachment #8668975 - Flags: review?(ttromey)
Rebased part 6 on top of the new part 4.
Attachment #8668970 - Attachment is obsolete: true
Attachment #8668970 - Flags: review?(mratcliffe)
Attachment #8668982 - Flags: review?(mratcliffe)
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.
Rebased part 5.
Attachment #8668964 - Attachment is obsolete: true
Attachment #8668964 - Flags: review?(ttromey)
Attachment #8668990 - Flags: review?(ttromey)
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.
Flags: needinfo?(hholmes)
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.
Attachment #8668990 - Flags: review?(ttromey) → review+
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.
Attachment #8668975 - Flags: review?(ttromey) → review+
Attachment #8668946 - Flags: review?(mratcliffe) → review+
Attachment #8668947 - Flags: review?(mratcliffe) → review+
Attachment #8668948 - Flags: review?(mratcliffe) → review+
Attachment #8668982 - Flags: review?(mratcliffe) → review+
(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.
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
(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.)
Flags: needinfo?(hholmes)
(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)
Blocks: 1211923
Blocks: 1211928
(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!
Attachment #8668946 - Flags: checkin+
Attachment #8668947 - Flags: checkin+
Attachment #8668948 - Flags: checkin+
Folded parts 4 and 5 together. Carried over R+ from Tom.
Attachment #8668975 - Attachment is obsolete: true
Attachment #8668990 - Attachment is obsolete: true
Attachment #8670298 - Flags: review+
Rebased. Carried over R+ from Mike.
Attachment #8668982 - Attachment is obsolete: true
Attachment #8670299 - Flags: review+
Comment on attachment 8668983 [details] [diff] [review]
Bug_1197100_-_7_-_Add_an_animationDetails_componen.diff

Marking as obsolete this last WIP part.
Attachment #8668983 - Attachment is obsolete: true
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.
Attachment #8670298 - Attachment is obsolete: true
Attachment #8670299 - Attachment is obsolete: true
Attachment #8675074 - Flags: review+
This is the second half of what used to be part 4.
Carrying R+ over for this too.
Attachment #8675075 - Flags: review+
And this used to be part 5. Was R+'d too.
Attachment #8675076 - Flags: review+
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 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
Attachment #8675074 - Flags: checkin+
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
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.
Summary: Display animations keyframes in the animation-inspector → Background work for displaying keyframes in the animation-inspector
Blocks: 1228005
Attachment #8675076 - Attachment is obsolete: true
Attachment #8675078 - Attachment is obsolete: true
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.
Attachment #8675075 - Attachment is obsolete: true
Attachment #8692066 - Flags: review+
Keywords: checkin-needed
Attachment #8692066 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/f9798fa51643
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
No longer blocks: 1210795
No longer blocks: 1210793
No longer blocks: 1210796
No longer blocks: 1211928
No longer blocks: 1211923
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.