1228005 - Display the list of keyframes and animated properties in the animation-inspector

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

With bug 1197100, we should have everything we need to work on the UI for this: ability to get the list of frames, ability to select an animation in the timeline.

Also bug 1171863 will be useful because it will make it possible to resize the sidebar and have the timeline adapt as you do this. That's important because keyframes add a tone of information to the timeline, and users will need to make the sidebar big to be able to use this.

This is a big UI change, so we need UI and UX help for this.
This is being done in this document: https://docs.google.com/document/d/1IGfzNcJOGgHcpyQURV3iNx_aGMM0aPNgeACljVuWEKk/edit

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Bug_1228005_-_1_-_Display_animated_property_list_w.diff

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug_1228005_-_2_-_Show_animated_properties__values.diff

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug_1228005_-_3_-_Tests_for_the_keyframes_and_anim.diff

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug_1228005_-_1_-_Display_animated_property_list_w.diff

Comment 5

[Patrick Brosset <:pbro>]

Attachment: Bug_1228005_-_3_-_Tests_for_the_keyframes_and_anim.diff

Comment 6

[Patrick Brosset <:pbro>]

Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a82934164e5a

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8694172 [details] [diff] [review]
Bug_1228005_-_2_-_Show_animated_properties__values.diff

Helen, before I ask for a code review, I'd like a UI review from you if possible.
Here's the link to download the build for Mac:
http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-a82934164e5a6f501c1e6360e99b6aafd5c10b74/try-macosx64/

Comment 8

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

Hey Patrick, just so I'm understanding correctly, is it in the works that you'll see the applied property here?: http://cl.ly/image/0n1m053w070v

It feels really strange that I'm only seeing it in a hover-tooltip on the keyframe itself, and not seeing it on the left where there's better space for it. Since AE is our benchmark for this stuff, I was anticipating something more like this: http://cl.ly/image/2d3P3U3i0M0q 

Another question the current designs leave me with is what this partially-shaded part is: http://cl.ly/image/0z020X0k2V2W What does that mean for the animation?

Also, what are the lightning bolts for? http://cl.ly/image/342N2b2T2Y3o It seems like all of the animations for the page I'm currently looking at have them. It seems like they house additional information about the animation; maybe we could make the entire box trigger that info?

For the way you toggle the additional info and the way you drag the playhead around, I think that's looking really good. This is super exciting stuff!~

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #8)
> Hey Patrick, just so I'm understanding correctly, is it in the works that
> you'll see the applied property here?: http://cl.ly/image/0n1m053w070v
> 
> It feels really strange that I'm only seeing it in a hover-tooltip on the
> keyframe itself, and not seeing it on the left where there's better space
> for it. Since AE is our benchmark for this stuff, I was anticipating
> something more like this: http://cl.ly/image/2d3P3U3i0M0q 
Yes, and I originally started with this, but then backed out a little bit and did the tooltips instead. Showing all the values next to the property names is kind of more complex, but also has more values, so I decided to move this to a later bug. Do you think I should get rid of the tooltip in the meantime?

> Another question the current designs leave me with is what this
> partially-shaded part is: http://cl.ly/image/0z020X0k2V2W What does that
> mean for the animation?
That's how we've been representing animation delays. If you hover over an animation, you should get a tooltip that contains data about the duration and delay. Maybe we can find a better way to represent this (although that would be done in another bug).

> Also, what are the lightning bolts for? http://cl.ly/image/342N2b2T2Y3o It
> seems like all of the animations for the page I'm currently looking at have
> them. It seems like they house additional information about the animation;
> maybe we could make the entire box trigger that info?
This indicates that the animation is currently running on the compositor. This is helpful to developers to know if they're hitting the fast track (animating properties like opacity or transform which do not cause reflows).

> For the way you toggle the additional info and the way you drag the playhead
> around, I think that's looking really good. This is super exciting stuff!~
Awesome!

Comment 10

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

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #9)
> Yes, and I originally started with this, but then backed out a little bit
> and did the tooltips instead. Showing all the values next to the property
> names is kind of more complex, but also has more values, so I decided to
> move this to a later bug. Do you think I should get rid of the tooltip in
> the meantime?

Hm, I wouldn't get rid of the tooltips, but I think I would make that next priority, and once that change is made then I /would/ get rid of the tooltips. It almost feels like a dark pattern, so I think that both changes should be made before it gets bundled up to go down the train for sure.

> That's how we've been representing animation delays. If you hover over an
> animation, you should get a tooltip that contains data about the duration
> and delay. Maybe we can find a better way to represent this (although that
> would be done in another bug).
Ohhhhh, all right. Yeah, might just need a state on hover just to explain what it is. Doing that in a separate bug makes sense to me unless it's super easy.

> This indicates that the animation is currently running on the compositor.
> This is helpful to developers to know if they're hitting the fast track
> (animating properties like opacity or transform which do not cause reflows).
Hm, all right. How would you feel about it toggling on hover for the entire area, instead of having the lightning bolt? The bolt just doesn't feel very semantic, so it was kind of confusing. (I was thinking, "Is this a super fast animation or something????" haha)

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #10)
> (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #9)
> > Yes, and I originally started with this, but then backed out a little bit
> > and did the tooltips instead. Showing all the values next to the property
> > names is kind of more complex, but also has more values, so I decided to
> > move this to a later bug. Do you think I should get rid of the tooltip in
> > the meantime?
> 
> Hm, I wouldn't get rid of the tooltips, but I think I would make that next
> priority, and once that change is made then I /would/ get rid of the
> tooltips. It almost feels like a dark pattern, so I think that both changes
> should be made before it gets bundled up to go down the train for sure.
Ok, considering the next merge date, I'm not sure I'll have time to add that change.
- either I don't land this patch before the merge (which is right after Orlando) so I have time to display the values as we discussed before the next merge,
- or I remove the tooltips and land this patch.
I think I'll do the latter if it sounds fine with you. Displaying keyframes is definitely a useful addition, whether we show values or not, so I'd like to push this to our users sooner rather than later.

> > That's how we've been representing animation delays. If you hover over an
> > animation, you should get a tooltip that contains data about the duration
> > and delay. Maybe we can find a better way to represent this (although that
> > would be done in another bug).
> Ohhhhh, all right. Yeah, might just need a state on hover just to explain
> what it is. Doing that in a separate bug makes sense to me unless it's super
> easy.
Filed bug 1229688.

> > This indicates that the animation is currently running on the compositor.
> > This is helpful to developers to know if they're hitting the fast track
> > (animating properties like opacity or transform which do not cause reflows).
> Hm, all right. How would you feel about it toggling on hover for the entire
> area, instead of having the lightning bolt? The bolt just doesn't feel very
> semantic, so it was kind of confusing. (I was thinking, "Is this a super
> fast animation or something????" haha)
Filed bug 1229683.

Comment 12

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

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #11)
> Ok, considering the next merge date, I'm not sure I'll have time to add that
> change.
> - either I don't land this patch before the merge (which is right after
> Orlando) so I have time to display the values as we discussed before the
> next merge,
> - or I remove the tooltips and land this patch.
> I think I'll do the latter if it sounds fine with you. Displaying keyframes
> is definitely a useful addition, whether we show values or not, so I'd like
> to push this to our users sooner rather than later.

I trust you dude, do what you think is best! I'm all right with this—it's true that our next merge date isn't super convenient. It'll be fine I think, December's a weird month where much fewer people are using their computers anyway :)

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8694174 [details] [diff] [review]
Bug_1228005_-_1_-_Display_animated_property_list_w.diff

Alright, let's go for code review for part 1. I'll keep part 2 on the side, we should not need it. And I'll remove the tooltip-related tests tests in part 3.

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8694174 [details] [diff] [review]
Bug_1228005_-_1_-_Display_animated_property_list_w.diff

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

Sorry about the delay on this.

Looks good.

However, while I read over the CSS change, I must say I don't really know what the gotchas might be or what I am looking for.  So I would suggest a review of this by someone else.

::: devtools/client/animationinspector/components.js
@@ +1345,5 @@
> +      return "-" + c.toLowerCase();
> +    }
> +    return c;
> +  }).join("") + "";
> +}

It's more concise and IMO more readable (due to avoiding a nested function) to write:
 
jsPropertyName.replace(/[A-Z]/g, "-$&").toLowerCase()
 
Assuming anyway that the only upper-case characters to deal with are ASCII.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: Bug_1228005_-_1_-_Display_animated_property_list_w.diff

Comment 16

[Patrick Brosset <:pbro>]

Attachment: Bug_1228005_-_2_-_Tests_for_the_keyframes_and_anim.diff

Since I got rid of the tooltips for now, I removed the corresponding test in this patch.

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8695822 [details] [diff] [review]
Bug_1228005_-_1_-_Display_animated_property_list_w.diff

Brian, Tom already reviewed most of the code changes except for the CSS.
Could you take a look at that part please?

Comment 18

[Tom Tromey :tromey]

Comment on attachment 8695824 [details] [diff] [review]
Bug_1228005_-_2_-_Tests_for_the_keyframes_and_anim.diff

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

Looks good.

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8695822 [details] [diff] [review]
Bug_1228005_-_1_-_Display_animated_property_list_w.diff

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

CSS changes look good

::: devtools/client/themes/animationinspector.css
@@ +22,5 @@
>    --timeline-animation-height: 20px;
> +  /* The size of a keyframe marker in the keyframes diagram */
> +  --keyframes-marker-size: 10px;
> +  /* The color of the time graduation borders. This should match the the color
> +     devtools/client/animationinspector/utils.js */

No big deal, but we do have a way to share these colors by specifying it in the theme variables and then using this module: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/theme.js#78.

Right now it's limited to colors specified in the main variables file and since this is limited to a tool maybe we don't do it.  But worth thinking more about this use case if we end up doing it in other places.

@@ +259,5 @@
>    height: var(--timeline-animation-height);
>    position: relative;
>  }
>  
> +.animation-timeline .animation:nth-child(4n+1) {

Why this?

Comment 20

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #19)
> Comment on attachment 8695822 [details] [diff] [review]
> Bug_1228005_-_1_-_Display_animated_property_list_w.diff
> 
> Review of attachment 8695822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> CSS changes look good
> 
> ::: devtools/client/themes/animationinspector.css
> @@ +22,5 @@
> >    --timeline-animation-height: 20px;
> > +  /* The size of a keyframe marker in the keyframes diagram */
> > +  --keyframes-marker-size: 10px;
> > +  /* The color of the time graduation borders. This should match the the color
> > +     devtools/client/animationinspector/utils.js */
> 
> No big deal, but we do have a way to share these colors by specifying it in
> the theme variables and then using this module:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/theme.
> js#78.
> 
> Right now it's limited to colors specified in the main variables file and
> since this is limited to a tool maybe we don't do it.  But worth thinking
> more about this use case if we end up doing it in other places.
Ah, nice. I didn't know about theme.js (or forgot about it).
Looking at that file, I see that it loads the theme variables from disk and somewhat parses it to get colors from there.
Wouldn't it be easier to use computed styles instead? Something like this would work fine I think:

var style = getComputedStyle(doc.documentElement);
for(var i = 0; i < style.length; i ++) {
  if (style[i].substring(0, 2) === "--") {
    var name = style[i];
    var value = style.getPropertyValue(style[i]));
  }
}

where 'doc' is the document object of any of the frames we load our theme in.

So, in fact, I could get rid of this comment in animationinspector.css and instead change devtools/client/animationinspector/utils.js to read the color value from computed styles.

> @@ +259,5 @@
> >    height: var(--timeline-animation-height);
> >    position: relative;
> >  }
> >  
> > +.animation-timeline .animation:nth-child(4n+1) {
> 
> Why this?
Ok, so that's because animated properties are now in a new element that's a sibling of the animation element, so you end up with something like this:

<animation timeline 1>
<animated properties for animation 1>
<animation timeline 2>
<animated properties for animation 2>
<animation timeline 3>
<animated properties for animation >
...

and I wanted to preserve the alternated background on animations, so that means changing the background on every 4 elements:
http://jsbin.com/jamobijega/edit?html,css,output

Comment 21

[Patrick Brosset <:pbro>]

Filed bug 1231347 for retrieving css variables from JS more easily.
About the 4n+1, I'll add a comment in the CSS file to make this clearer.

Comment 22

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fe20c78dc26

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2a266b600cef
https://hg.mozilla.org/integration/fx-team/rev/973264cc53e4

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2a266b600cef
https://hg.mozilla.org/mozilla-central/rev/973264cc53e4

Comment 25

[Patrick Brosset <:pbro>]

Release Note Request (optional, but appreciated)
[Why is this notable]: This feature provides a lot more info for people inspecting/debugging css animations in firefoxdevtools.
[Suggested wording]: The list of animated properties and keyframes is now displayed when you click on an animation in the animation-inspector's timeline.
[Links (documentation, blog post, etc)]: https://www.youtube.com/watch?v=Un3u4wuGT8Q

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes "In the animation-inspector's timeline, when clicking on an animation the list of animated properties and keyframes is now displayed"
with a link to the video