1180134 - Indicate if an animation displayed in the animation-inspector comes from a CSS transition or a CSS animation

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

I think bug 1179111 gives us the ability to know, so it'd be easy enough to indicate this in the UI somehow.

Comment 1

[Patrick Brosset <:pbro>]

Going to work on this on top of bug 1169563

Comment 2

[Patrick Brosset <:pbro>]

Attachment: 1 - Color code animations and transitions so they look different

Comment 3

[Patrick Brosset <:pbro>]

Attachment: color-coded-animations.PNG

Here's what animations look like with the patch applied.
Other options are:
- adding the words "Transition" and "Animation" in the timeline, but that takes a lot of space and we're already using ellipsis if the animation names are too long,
- using icons, but coming up with good icons will be hard,
- separating animations and transitions in 2 sections with section headers.

Note that in the future, there will be SVG SMIL animations in here too, as well as script-generated web animations.

Comment 4

[Patrick Brosset <:pbro>]

Brian, any ideas on this? Do you think colors are enough at this stage?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #4)
> Brian, any ideas on this? Do you think colors are enough at this stage?

Looks fine to me - you might combine that with tooltiptext that indicates "opacity - transition" and "anim open - animation"

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug1180134.patch

Comment 7

[Patrick Brosset <:pbro>]

Removing this flag, the platform work for this has already been done before.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: Bug_1180134_-_1_-_Color_code_animations_and_transi.diff

This patch contains:
- a new 'type' property in each animation actor's state object
- some cleanup in the css and 2 new css classes for transitions and animations to look different
- the ui component responsible for displaying animations uses the actor's state to use the new css classes
- new l10n strings that are used as tooltips on animations in the ui
- some tests

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

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7e4a530f2fb
Damn, I forgot to update one test. I will cancel this try push and will push again later.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8653940 [details] [diff] [review]
Bug_1180134_-_1_-_Color_code_animations_and_transi.diff

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

::: browser/devtools/animationinspector/components.js
@@ +939,5 @@
>      createNode({
>        parent: iterations,
>        attributes: {
>          "class": "name",
> +        "title": state.type

Here there is a falsy check for state.type but above we string concat state.type + " iterations".  What's the case where it's falsy?  It appears it would end up adding "null" as a class name.

::: browser/locales/en-US/chrome/browser/devtools/animationinspector.properties
@@ +65,5 @@
>  timeline.timeGraduationLabel=%Sms
> +
> +# LOCALIZATION NOTE (timeline.CSSAnimation.nameLabel):
> +# This string is displayed in a tooltip of the animation panel that is shown
> +# when hovering over the name of a CSS Animation in the timeline UI.

Worth adding a note of what the %S is going to be here

@@ +70,5 @@
> +timeline.cssanimation.nameLabel=%S - CSS Animation
> +
> +# LOCALIZATION NOTE (timeline.CSSAnimation.nameLabel):
> +# This string is displayed in a tooltip of the animation panel that is shown
> +# when hovering over the name of a CSS Transition in the timeline UI.

And here

::: browser/themes/shared/devtools/animationinspector.css
@@ +273,5 @@
>    top: unset;
>  }
>  
>  .animation-timeline .animation .name {
> +  color: white;

Nit: var(--theme-selection-color) is pretty much white and will match up with other colors seen in the toolbox

::: toolkit/devtools/server/actors/animation.js
@@ +43,5 @@
> +// Types of animations.
> +const TYPES = {
> +  CSS_ANIMATION: "cssanimation",
> +  CSS_TRANSITION: "csstransition",
> +  UNKNOWN: "unknown"

This type doesn't seem to be used at all, but null is returned instead.  This should either be removed from the const, or we should be returning TYPES.UNKNOWN and looking for that in the tool frontend.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: Bug_1180134_-_Color_code_animations_and_transition.diff

Thanks for the review. This should address your comments.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: Bug_1180134_-_Color_code_animations_and_transition.diff

Attached the wrong patch.

Comment 13

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92a5f2d08c77

Comment 14

[Pulsebot]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9bb65cd568c3

Comment 16

[Alfred Kayser]

Spot the little typo: timelime --> timeline

Comment 17

[Alfred Kayser]

This typo causes the dots to miss the border color...

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Alfred Kayser from comment #17)
> This typo causes the dots to miss the border color...
Thanks, filed bug 1233363 to fix this.