1232681 - Display script-generated animations correctly

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Show script-generated animations in the timeline.

By default they'll be displayed, but we need to introduce a new animation type in devtools/server/actors/animation.js and make sure that nothing at actor levels except animations to be CSS-only.

Also, we should choose a different color for those animations in the timeline, and make sure the tooltip with the animation type is displayed correctly.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Hi Patrick, we landed support for Element.animate to m-c last week and we'd like to ship it in Firefox 47. Any chance we could get DevTools to use animation.id as the label for script-generated animations in the Firefox 47 timeframe?

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #1)
> Hi Patrick, we landed support for Element.animate to m-c last week and we'd
> like to ship it in Firefox 47. Any chance we could get DevTools to use
> animation.id as the label for script-generated animations in the Firefox 47
> timeframe?
animation.id in devtools is bug 1231945 and, yes, landing that bug in 47 seems very reasonable. I'll take it on.
I assume we should also land a fix for this bug too.

Comment 3

[Patrick Brosset <:pbro>]

As discussed on IRC, Nicolas said he'd be interested to work on this.
Nicolas, don't hesitate to ask as many questions as needed. And feel free to un-assign yourself if can't/don't want to work on this. We'd like to ship this in the 47 timeframe, which means landing this on nightly at the end of February maximum.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: screenshot.png

Hi there,
I started working on it, and I think i know how to take care of this, at least visually. See the screenshot attached.

I have a few questions though : 

1. How should we name the type of those animations ? I opted for 'Script Animation', but maybe it should only be 'Animation' ?

2. If I animate an element without an id, what should we display here ? In my screenshot, I put 'animated', but that's definitely not a good label.

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #4)
> I started working on it, and I think i know how to take care of this, at
> least visually. See the screenshot attached.
Nice, this screenshot looks great.
> 1. How should we name the type of those animations ? I opted for 'Script
> Animation', but maybe it should only be 'Animation' ?
Script Animation sounds good to me. I'll needinfo Brian who can probably help take a decision here. Brian: this is the name that will appear in a tooltip when you hover over one of these animations in the timeline. Today we have "CSS Animation" and "CSS Transition" only. Nicolas is proposing to add "Script Animation".
> 2. If I animate an element without an id, what should we display here ? In
> my screenshot, I put 'animated', but that's definitely not a good label.
I recently fixed bug 1231945 to display the id when there is one, but indeed, if there is none (and I'm guessing that's going to be most of the cases), currently the empty string "" appears.
We could display something like "untitled", but that doesn't help more than having no name at all, so I don't like this.
Maybe when there is no name we display the type instead? So in that case "Script Animation"?

There are 2 places where the name is displayed:
- the tooltip: I think we should just omit it in that case, if the anmimation has no name/id, just don't display anything there (which means we need to make a simple change to remove the empty line that's currently displayed),
- on the animation block: I'd vote for "Script Animation" here, or nothing, just leave it empty and we can change that later if we find a better idea.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: screenshot_v02.png

Updated screenshot with all animation types, details expanded.
I quite look how it looks when the animation has no id ( well, it's empty ).

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1232681_v_01.patch

Hi, this is where I am so far, everything seems to work fine. Tests all succeed, so at least I did not break anything.

I guess the next step is to create the same tests we have for CSS animations with Scripts animations.
I wanted to know if I'm on the right way before doing it.

Thank you

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8716027 [details] [diff] [review]
Bug_1232681_v_01.patch

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

::: devtools/client/animationinspector/components/animation-time-block.js
@@ +160,3 @@
>    // Older servers don't send the type.
> +  if (state.type) {
> +    if (state.name === "") {

So, CSSAnimations and CSSTransitions can't be unnamed. They'll either have a name that comes from CSS or a custom id that the developer added from javascript after the fact.

So, timeline.CSSTransition.unnamedLabel and timeline.CSSAnimation.unnamedLabel aren't needed.
Maybe we could re-structure the code to make the intent a little clearer:

getFormattedAnimationTitle({state}) {
  // Older servers don't send a type, and only know about
  // CSSAnimations and CSSTransitions, so it's safe to use
  // just the name.
  if (!state.type) {
    return state.name;
  }

  // Script-generated animations may not have a name.
  if (state.type === "scriptanimation" && !state.name) {
    return L10N.getStr(`timeline.scriptanimation.unnamedLabel`);
  }
  
  return L10N.getFormatStr(`timeline.${state.type}.nameLabel`, state.name);
}

::: devtools/client/locales/en-US/animationinspector.properties
@@ +95,5 @@
> +
> +# LOCALIZATION NOTE (timeline.cssanimation.nameLabel):
> +# This string is displayed in a tooltip of the animation panel that is shown
> +# when hovering over an unnamed CSS Animation in the timeline UI.
> +timeline.cssanimation.unnamedLabel=CSS Animation

Based on my previous comment, you can remove this string.

@@ +100,5 @@
> +
> +# LOCALIZATION NOTE (timeline.csstransition.nameLabel):
> +# This string is displayed in a tooltip of the animation panel that is shown
> +# when hovering over an unnamed CSS Transition in the timeline UI.
> +timeline.csstransition.unnamedLabel=CSS Transition

And this one.

::: devtools/server/actors/animation.js
@@ +128,5 @@
>      return player instanceof this.tabActor.window.CSSTransition;
>    },
>  
> +  isScriptAnimation: function(player = this.player) {
> +    return player instanceof this.tabActor.window.Animation;

Unfortunately, CSSTransition and CSSAnimation inherit from Animation.
So even if player is a CSSAnimation, this will return true.
In practice this is fine because this is called as the last 'else if' in the getType function below.
But still, it would be better if this function only returned true for script animations.
Can you needinfo Brian Birtles and ask what's the proper way to do this? (:birtles)

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #5)
> (In reply to Nicolas Chevobbe from comment #4)
> > I started working on it, and I think i know how to take care of this, at
> > least visually. See the screenshot attached.
> Nice, this screenshot looks great.
> > 1. How should we name the type of those animations ? I opted for 'Script
> > Animation', but maybe it should only be 'Animation' ?
> Script Animation sounds good to me. I'll needinfo Brian who can probably
> help take a decision here. Brian: this is the name that will appear in a
> tooltip when you hover over one of these animations in the timeline. Today
> we have "CSS Animation" and "CSS Transition" only. Nicolas is proposing to
> add "Script Animation".

That sounds fine to me.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Hi Brian,

How can I check if an animation is a Script animation.
I logged player.constructor.name and it outputs "Animation", and I did not knew that CSSAnimation and CSSTransition inherit from it. 
So, is there a better way to check if an animation is a script animation than : 

    player instanceof this.tabActor.window.Animation && ! (
      player instanceof this.tabActor.window.CSSAnimation ||
      player instanceof this.tabActor.window.CSSTransition 
    )

?

Thank you

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Nicolas Chevobbe from comment #10)
> How can I check if an animation is a Script animation.
> I logged player.constructor.name and it outputs "Animation", and I did not
> knew that CSSAnimation and CSSTransition inherit from it. 
> So, is there a better way to check if an animation is a script animation
> than : 
> 
>     player instanceof this.tabActor.window.Animation && ! (
>       player instanceof this.tabActor.window.CSSAnimation ||
>       player instanceof this.tabActor.window.CSSTransition 
>     )
> 
> ?

That seems right to me. I checked with Cameron and he agrees that it's ok.

Comment 12

[Patrick Brosset <:pbro>]

Note that in bug 1245849, I'm adding 2 new (xpcshell) tests that will need to be expanded before this lands. This should be pretty easy and I can help you with this.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Attachment: MozReview Request: Bug 1232681 - Display script-generated animations correctly. r?pbro

Review commit: https://reviewboard.mozilla.org/r/34253/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34253/

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8717646 [details]
MozReview Request: Bug 1232681 - Display script-generated animations correctly. r?pbro

https://reviewboard.mozilla.org/r/34253/#review31035

This looks really good. No comments about the code changes. I do have a few comments about the tests. But I don't need to re-review this after you've done the changes. So R+.
Please push to try.

::: devtools/client/animationinspector/test/doc_multiple_animation_types.html:18
(Diff revision 1)
> +      transition: background-color 10s;

Just to be extra safe, please make this last 20s instead. Some test machines are really slow (especially in debug) builds, and tests run therefore really slowly. So, in theory, if things are slow enough, we could have this test fail intermittently if it takes more than 10s to open the toolbox/animation-inspector and the transition is already done.

The css animation and script animation are both fine because the fill forwards, so they will always appear in the timeline, even if they're done.

::: devtools/server/tests/unit/test_animation_name.js:25
(Diff revision 1)
>    };

I think we should add this here:

window.CSSAnimation.prototype = Object.create(window.Animation.prototype);
window.CSSTransition.prototype = Object.create(window.Animation.prototype);

So that CSSAnimation and CSSTransition both inherit from Animation, because it's true in real life, and the code in isScriptAnimation sort of depends on that.

::: devtools/server/tests/unit/test_animation_type.js:25
(Diff revision 1)
>    };

I think we should add this here:

window.CSSAnimation.prototype = Object.create(window.Animation.prototype);
window.CSSTransition.prototype = Object.create(window.Animation.prototype);

So that CSSAnimation and CSSTransition both inherit from Animation, because it's true in real life, and the code in isScriptAnimation sort of depends on that.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1232681_v_03.patch

Edited patch with review comments. TRY server closed for now, I'll push later

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Job is over. There are some errors but don't seems to be related to this patch

Comment 18

[Pulsebot]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61089f9d1f6d

Comment 20

[Patrick Brosset <:pbro>]

Release Note Request (optional, but appreciated)
[Why is this notable]: Because the WebAnimations API is new and this tool will help with awareness and adoption.
[Suggested wording]: Starting with FF47, script-generated animations that are created using the Web Animations API's `element.animate()` (which also should land in FF47) are displayed in the animation inspector timeline.
[Links (documentation, blog post, etc)]: https://twitter.com/brianskold/status/694795163437109248/video/1

Note that Brian Birtles is the person to contact for the implementation of the Web Animations API. He's targeting FF47 for element.animate() but I'm not sure what's the latest status about this. Also, I hear a blog post is going to be written about this.

@Brian: do you think we should rel-note this change?

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I think shipping Element.animate in Firefox 47 is at risk at this stage. There are a couple of significant bugs we need to fix first and at this rate we'll only likely get one of them fixed in time for the next merge.

If we're not shipping Element.animate then I wonder if it makes sense to rel-note the DevTools support for it?