Closed Bug 1383974 Opened 3 years ago Closed 2 years ago

Animation inspector doesn't show easing in tooltip for steps timing function

Categories

(DevTools :: Inspector: Animations, defect, P3)

defect

Tracking

(firefox57 fix-optional, firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: birtles, Assigned: daisuke)

Details

Attachments

(6 files)

STR:
1. Load http://msuja.ws/svg.html
2. Inspect one of the animations
3. Hover over the top graph

Expected results:
The tooltip says, "Overall easing: steps(34)"

Actual results:
There is no mention of the easing in the tooltip.

This bug reproduces for me with or without Stylo.
I see below exception on a debug build with stylo on inbox.google.com.

Full message: TypeError: easing is undefined
Full stack: getPreferredProgressThreshold@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/animationinspector/graph-helper.js:715:9
(In reply to Brian Birtles (:birtles) from comment #0)
> STR:
> 1. Load http://msuja.ws/svg.html
> 2. Inspect one of the animations
> 3. Hover over the top graph
> 
> Expected results:
> The tooltip says, "Overall easing: steps(34)"
> 
> Actual results:
> There is no mention of the easing in the tooltip.
> 
> This bug reproduces for me with or without Stylo.

Sorry for late replying.
The easing which displays in tooltip is getting from effect timing.
In this case, because these animations are CSS Animations, the inputting easing is applied to the keyframes and effect timing easing is 'linear'.
Also, we avoid to display default value (easing: linear).
Due to those reason, there is no information at the tooltip in summary graph.

However, there is no way to know the easing as text for CSS Animation and keyframe's easing. 
Let me think about this. maybe,,
* tooltip on keyframe (small circle) in detail pane?
* tooltip on detail graph?
Or, we may need to handle CSS Animations easing as special to display in the tooltip of summary graph.

Thank you very much!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I see below exception on a debug build with stylo on inbox.google.com.
> 
> Full message: TypeError: easing is undefined
> Full stack:
> getPreferredProgressThreshold@resource://gre/modules/commonjs/toolkit/loader.
> js -> resource://devtools/client/animationinspector/graph-helper.js:715:9

This looks like another bug.
AnimationInspector refreshes most of UIs at the various timing.
It contains that starting the animations as well.
This looks like could reproduce when we are getting many timings like those, but I'm not sure.
I'll investigate this.

Thank you very much!
Attached image easing.gif
This is a prototype to display the easing on graph.
Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
Priority: -- → P3
I wanted to start testing this locally but I can't apply the patch. I think it might depend on bug 1366989. If so, could you please mark this bug as blocked by bug 1366989? 
Also, I tried to apply the patches from bug 1366989 but they also did not apply cleanly. Is there something else I'm missing or have they just been rebased?
Flags: needinfo?(dakatsuka)
Oh, I'm sorry, indeed, could not apply the patch..
I fixed and re-uploaded.
Flags: needinfo?(dakatsuka)
(In reply to Daisuke Akatsuka (:daisuke) from comment #2)
> The easing which displays in tooltip is getting from effect timing.
> In this case, because these animations are CSS Animations, the inputting
> easing is applied to the keyframes and effect timing easing is 'linear'.
> Also, we avoid to display default value (easing: linear).
Right, I remember that, this was done in bug 1330538 I think.

> Or, we may need to handle CSS Animations easing as special to display in the
> tooltip of summary graph.
I think we need to do that in fact. I tend to think that the vast majority of people are not setting an easing per keyframe, and do not know the different between effect and keyframe easing. 
So, I think our tool should be smart enough to detect that the same keyframe easing is applied throughout, and display it in the summary graph tooltip too. Maybe we need to prefix it with "keyframes easing: ..."  to make it clear.
This could be done in another bug I guess.

I like the approach you're taking in your patches here, but I think having the info (when possible) also in the summary graph tooltip would be good.
Comment on attachment 8905990 [details]
Bug 1383974 - Part 1: Display easing in keyframes.

https://reviewboard.mozilla.org/r/177750/#review186386

Looking good, thanks Daisuke! See my comments below.
I'll take one more look at the patch fter you've responded to them.

::: devtools/client/animationinspector/components/keyframes.js:59
(Diff revision 2)
> -    const strokeHeightForViewBox = 0.5 / this.containerEl.clientHeight;
> +    // NOTE: Maximum stroke width is specified
> +    // as ".keyframes svg path.hint" in animationinspector.css.
> +    const maxStrokeWidth = 5;

Are you saying that both the value in the CSS and in the JS need to be kept in sync?
If so, that's a bit unfortunate because they will tend to be hard to maintain. We might change the CSS and forget about the JS part.
If you can't find any way to avoid this, then please do the following:

- move this const at the top of the file, name it like we normally name module constants: MAX_STROKE_WIDTH
- add a visible comment above it like: /!\ keep in sync with the stroke width in animationinspector.css for .keyframes svt path.hint
- then in the CSS file, add a similar comment, explaining that this stroke width should be kept in sync with MAX_STROKE_WIDTH in keyframes.js

But if you can find a way to just have it in one place, that would be best.

::: devtools/client/animationinspector/components/keyframes.js:63
(Diff revision 2)
> -    // Calculate stroke height in viewBox to display stroke of path.
> -    const strokeHeightForViewBox = 0.5 / this.containerEl.clientHeight;
> +    // Maximum stroke width for Path element.
> +    // NOTE: Maximum stroke width is specified
> +    // as ".keyframes svg path.hint" in animationinspector.css.
> +    const maxStrokeWidth = 5;
> +    // Calculate stroke width in viewBox to display stroke of path.
> +    const strokeHeightForViewBox = maxStrokeWidth / 2 / this.containerEl.clientHeight;

Why do you divide by 2 here? Can you move this 2 into a module constant that has a name and a comment, so it's easier to understand.

::: devtools/client/animationinspector/components/keyframes.js:155
(Diff revision 2)
> + * Render easing hint. This method render two things.
> + * One is as text, another one is emphasis path which display when mouse is over the path.

Some rephrasing suggestions:

Renders the easing hint.
This method renders an emphasized path over the easing path for a keyframe. It appears when hovering over the easing.
It also renders a tooltip that appears when hovering.

::: devtools/client/animationinspector/components/keyframes.js:159
(Diff revision 2)
> +/**
> + * Render easing hint. This method render two things.
> + * One is as text, another one is emphasis path which display when mouse is over the path.
> + * @param {Element} parentEl - Parent element of this appended path element.
> + * @param {Array} path segments - [{x: {Number} time, y: {Number} progress}, ...]
> + * @param {ProgressGraphHelper} graphHelper - The object of ProgressGraphHalper.

ProgressGraphHelper, not Halper

::: devtools/client/animationinspector/components/keyframes.js:168
(Diff revision 2)
> +    if (startKeyframe.easing === "linear") {
> +      // Ignore to display the hint.
> +      continue;
> +    }

Why not? I know it's a straight line, but I still see the value of being consistent here. So that wherever you hover, the hint and tooltip will be displayed.

::: devtools/client/animationinspector/components/keyframes.js:227
(Diff revision 2)
> +    if (startKeyframe.easing === "linear") {
> +      // Ignore to display the hint.
> +      continue;
> +    }

Same comment here. Consistently showing easing would make it more predictable for users, and avoid them thinking that perhaps the thing is broken.

::: devtools/client/animationinspector/graph-helper.js:286
(Diff revision 2)
>     * @param {Array} pathSegments - Path segments. Please see createPathSegments.
>     * @param {String} cls - Class name.
>     * @return {Element} path element.
>     */
>    appendPathElement: function (parentEl, pathSegments, cls) {
> -    return appendPathElement(parentEl, pathSegments, cls);
> +    return appendPathElement(parentEl, pathSegments, cls, false /* isClosePathNeeded */);

nit: we don't usually have inline comments like this.
If you feel like the false parameter needs to be explained here, then add the comment above, as a separate line.

::: devtools/client/animationinspector/graph-helper.js:530
(Diff revision 2)
> + * Append path element. Also, this method appends two segment
> + * that are {start x, 0} and {end x, 0} to make shape.
> + * But does not affect given pathSegments.

I'm not sure about the name of this function. The function is still appending a path element. It's just that it's changing the path at the same time. But I don't understand the "AsShape" suffix.

Or maybe its the comment that needs to be made clearer. What do you mean exactly when you say "to make a shape"?

::: devtools/client/animationinspector/graph-helper.js:540
(Diff revision 2)
> + * @param {String} cls - Class name.
> + * @param {bool} isClosePathNeeded - Set true if need to close the path. (default true)
> + * @return {Element} path element.
> + */
> +function appendPathElementAsShape(parentEl, pathSegments, cls, isClosePathNeeded = true) {
> +  const segments = pathSegments.concat();

If I understand correctly, you're only using concat to make a new array without modifying the old one, so that you don't have side effects outside of this function. Right?
If so, then please add a comment above this line, otherwise it will make it hard for people to understand the empty concat().

::: devtools/client/animationinspector/graph-helper.js:540
(Diff revision 2)
> +  const segments = pathSegments.concat();
> +  segments.unshift(Object.assign({}, pathSegments[0], { y: 0 }));
> +  segments.push(Object.assign({}, pathSegments[pathSegments.length - 1], { y: 0 }));

I think you could simplify all of this and make it more readable this way:

```
const segments = [
  { x: pathSegments[0].x, y:0 },
  ...pathSegments,
  { x: pathSegments[pathSegments.length - 1].x, y: 0 }
];
```

::: devtools/client/animationinspector/graph-helper.js:551
(Diff revision 2)
> +/**
>   * Append path element.
>   * @param {Element} parentEl - Parent element of this appended path element.
>   * @param {Array} pathSegments - Path segments. Please see createPathSegments.
>   * @param {String} cls - Class name.
>   * @param {bool} isClosePathNeeded - Set true if need to close the path. (default true)

The parameter does not default to true anymore, so you need to change this comment.
Attachment #8905990 - Flags: review?(pbrosset)
Comment on attachment 8905991 [details]
Bug 1383974 - Part 2: Add tests for easing hint in keyframes.

https://reviewboard.mozilla.org/r/177752/#review186432
Attachment #8905991 - Flags: review?(pbrosset) → review+
Attached image easing-tooltip.png
(In reply to Patrick Brosset <:pbro> from comment #12)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #2)
> > The easing which displays in tooltip is getting from effect timing.
> > In this case, because these animations are CSS Animations, the inputting
> > easing is applied to the keyframes and effect timing easing is 'linear'.
> > Also, we avoid to display default value (easing: linear).
> Right, I remember that, this was done in bug 1330538 I think.
> 
> > Or, we may need to handle CSS Animations easing as special to display in the
> > tooltip of summary graph.
> I think we need to do that in fact. I tend to think that the vast majority
> of people are not setting an easing per keyframe, and do not know the
> different between effect and keyframe easing. 
> So, I think our tool should be smart enough to detect that the same keyframe
> easing is applied throughout, and display it in the summary graph tooltip
> too. Maybe we need to prefix it with "keyframes easing: ..."  to make it
> clear.
> This could be done in another bug I guess.
> 
> I like the approach you're taking in your patches here, but I think having
> the info (when possible) also in the summary graph tooltip would be good.

How about is attachment 8910147 [details]?
Use "animation-timing-function" instead of "keyframe easing" as label. Because user must specify using animation-timing-function in case of CSS Animation. Also, may confuse if use "keyframe timing" since we can specify the easing to each keyframe.
(In reply to Daisuke Akatsuka (:daisuke) from comment #16)
> How about is attachment 8910147 [details]?
> Use "animation-timing-function" instead of "keyframe easing" as label.
> Because user must specify using animation-timing-function in case of CSS
> Animation. Also, may confuse if use "keyframe timing" since we can specify
> the easing to each keyframe.
Yeah, that sounds ok to me.
(In reply to Patrick Brosset <:pbro> from comment #17)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #16)
> > How about is attachment 8910147 [details]?
> > Use "animation-timing-function" instead of "keyframe easing" as label.
> > Because user must specify using animation-timing-function in case of CSS
> > Animation. Also, may confuse if use "keyframe timing" since we can specify
> > the easing to each keyframe.
> Yeah, that sounds ok to me.

Thanks, I'll add that patch as well.
Comment on attachment 8905990 [details]
Bug 1383974 - Part 1: Display easing in keyframes.

https://reviewboard.mozilla.org/r/177750/#review186386

> Are you saying that both the value in the CSS and in the JS need to be kept in sync?
> If so, that's a bit unfortunate because they will tend to be hard to maintain. We might change the CSS and forget about the JS part.
> If you can't find any way to avoid this, then please do the following:
> 
> - move this const at the top of the file, name it like we normally name module constants: MAX_STROKE_WIDTH
> - add a visible comment above it like: /!\ keep in sync with the stroke width in animationinspector.css for .keyframes svt path.hint
> - then in the CSS file, add a similar comment, explaining that this stroke width should be kept in sync with MAX_STROKE_WIDTH in keyframes.js
> 
> But if you can find a way to just have it in one place, that would be best.

Agree. I'll refer the CSS value from JS.

> Why do you divide by 2 here? Can you move this 2 into a module constant that has a name and a comment, so it's easier to understand.

If we could support stroke-alignment[1] property and specify 'inner', we don't need this related codes.
Currently, it is the same state as 'center'. For example, if we create rect along maximum value of viewBox, half of stroke is hidden. To avoid this, we enlarge the SVG viewBox by that amount. The 2 is for that. (So, we can handle y axis in 0 - 1 for generating the graph.)

And, yes, I will add the explanation.

https://svgwg.org/specs/strokes/#SpecifyingStrokeAlignment

> Why not? I know it's a straight line, but I still see the value of being consistent here. So that wherever you hover, the hint and tooltip will be displayed.

The default value is avoiding to display in tooltip in summary graph. I made this easing hint consistent with that. But as you said, I agree the consistency for the another aspect as well.
Okay, I'll change to show whenever in this time.

> I'm not sure about the name of this function. The function is still appending a path element. It's just that it's changing the path at the same time. But I don't understand the "AsShape" suffix.
> 
> Or maybe its the comment that needs to be made clearer. What do you mean exactly when you say "to make a shape"?

This method appends start / end point of graph into given segments, then generates the path which includes closepath sign by this. As the result, the shape will be along the bottom axis. ( for example, triangle will be made in case of 'linear' )
Another method appendPathElement, it converts the path from given segments as it is. That is, it becomes a line. I meant like this, but how should I say??

> If I understand correctly, you're only using concat to make a new array without modifying the old one, so that you don't have side effects outside of this function. Right?
> If so, then please add a comment above this line, otherwise it will make it hard for people to understand the empty concat().

Yes, right. Okay, I'll add.

> I think you could simplify all of this and make it more readable this way:
> 
> ```
> const segments = [
>   { x: pathSegments[0].x, y:0 },
>   ...pathSegments,
>   { x: pathSegments[pathSegments.length - 1].x, y: 0 }
> ];
> ```

Oh, thanks!
(In reply to Daisuke Akatsuka (:daisuke) from comment #19)
> > I'm not sure about the name of this function. The function is still appending a path element. It's just that it's changing the path at the same time. But I don't understand the "AsShape" suffix.
> > 
> > Or maybe its the comment that needs to be made clearer. What do you mean exactly when you say "to make a shape"?
> 
> This method appends start / end point of graph into given segments, then
> generates the path which includes closepath sign by this. As the result, the
> shape will be along the bottom axis. ( for example, triangle will be made in
> case of 'linear' )
> Another method appendPathElement, it converts the path from given segments
> as it is. That is, it becomes a line. I meant like this, but how should I
> say??
Then you could rename appendPathElement to appendLinePath and appendPathElementAsShape to appendShapePath.
(In reply to Patrick Brosset <:pbro> from comment #20)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #19)
> > > I'm not sure about the name of this function. The function is still appending a path element. It's just that it's changing the path at the same time. But I don't understand the "AsShape" suffix.
> > > 
> > > Or maybe its the comment that needs to be made clearer. What do you mean exactly when you say "to make a shape"?
> > 
> > This method appends start / end point of graph into given segments, then
> > generates the path which includes closepath sign by this. As the result, the
> > shape will be along the bottom axis. ( for example, triangle will be made in
> > case of 'linear' )
> > Another method appendPathElement, it converts the path from given segments
> > as it is. That is, it becomes a line. I meant like this, but how should I
> > say??
> Then you could rename appendPathElement to appendLinePath and
> appendPathElementAsShape to appendShapePath.

OK, thank you very much!
Failed a test. The cause is patch 3.
Comment on attachment 8905990 [details]
Bug 1383974 - Part 1: Display easing in keyframes.

https://reviewboard.mozilla.org/r/177750/#review187900

::: devtools/client/animationinspector/components/keyframes.js:63
(Diff revision 3)
> -                         `0 -${ 1 + strokeHeightForViewBox }
> -                          ${ totalDuration }
> -                          ${ 1 + strokeHeightForViewBox * 2 }`);
> -
>      // Create graph helper to render the animation property graph.
> +    const win = this.containerEl.ownerDocument.defaultView;

I think we have an ESLint rule that suggests rewriting this as this.containerEl.ownerGlobal;
Attachment #8905990 - Flags: review?(pbrosset) → review+
Comment on attachment 8910700 [details]
Bug 1383974 - Part 3: Display animation-timing-function if CSS Animations.

https://reviewboard.mozilla.org/r/182156/#review187902

::: commit-message-7d6df:1
(Diff revision 2)
> +Bug 1383974 - Part 3: Display animation-timing-functin if CSS Animations. r?pbro

nit: function, not functin

::: devtools/client/locales/en-US/animationinspector.properties:84
(Diff revision 2)
>  
> +# LOCALIZATION NOTE (player.animationTimingFunctionLabel):
> +# This string is displayed in a tooltip that appears when hovering over
> +# animations in the timeline. It is the label displayed before the
> +# animation-timing-function for CSS Animations.
> +player.animationTimingFunctionLabel=animation-timing-function:

Other labels start with an uppercase letter, and are actual sentences, not keywords.
So, I would change this to be "Aniation timing function:"
Attachment #8910700 - Flags: review?(pbrosset) → review+
Comment on attachment 8910701 [details]
Bug 1383974 - Part 4: Modify test for tooltip for animation-timing-function.

https://reviewboard.mozilla.org/r/182158/#review187906
Attachment #8910701 - Flags: review?(pbrosset) → review+
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/693f32f563d2
Part 1: Display easing in keyframes. r=pbro
https://hg.mozilla.org/integration/autoland/rev/ac3850461834
Part 2: Add tests for easing hint in keyframes. r=pbro
https://hg.mozilla.org/integration/autoland/rev/f951257a6856
Part 3: Display animation-timing-function if CSS Animations. r=pbro
https://hg.mozilla.org/integration/autoland/rev/588aa1f227f1
Part 4: Modify test for tooltip for animation-timing-function. r=pbro
Backed out for failing browser_discovery.js:

https://hg.mozilla.org/integration/autoland/rev/22e4997cd797715f1ff4221aeff5f5ace94bf9de

Push which ran failing test: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7bcda8f31f98a0d8c7be8e42f6a05336225f8401&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132716979&repo=autoland

[task 2017-09-22T15:59:50.778Z] 15:59:50     INFO - Test emphasis path
[task 2017-09-22T15:59:50.784Z] 15:59:50     INFO - TEST-PASS | devtools/client/animationinspector/test/browser_animation_detail_easings.js | stroke-opacity of hintEl for "opacity" of "css-animations" should be 0 while mouse is out from the element - 
[task 2017-09-22T15:59:50.786Z] 15:59:50     INFO - TEST-PASS | devtools/client/animationinspector/test/browser_animation_detail_easings.js | stroke-opacity of hintEl for "opacity" of "css-animations" should be 1 while mouse is over the element - 
[task 2017-09-22T15:59:50.788Z] 15:59:50     INFO - Click on animation 1 in the timeline
[task 2017-09-22T15:59:50.791Z] 15:59:50     INFO - Buffered messages logged at 15:56:50
[task 2017-09-22T15:59:50.793Z] 15:59:50     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 4
[task 2017-09-22T15:59:50.794Z] 15:59:50     INFO - Buffered messages logged at 15:57:35
[task 2017-09-22T15:59:50.796Z] 15:59:50     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 3
[task 2017-09-22T15:59:50.797Z] 15:59:50     INFO - Buffered messages logged at 15:58:20
[task 2017-09-22T15:59:50.799Z] 15:59:50     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 2
[task 2017-09-22T15:59:50.800Z] 15:59:50     INFO - Buffered messages logged at 15:59:05
[task 2017-09-22T15:59:50.801Z] 15:59:50     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
[task 2017-09-22T15:59:50.802Z] 15:59:50     INFO - Buffered messages finished
[task 2017-09-22T15:59:50.804Z] 15:59:50     INFO - TEST-UNEXPECTED-FAIL | devtools/client/animationinspector/test/browser_animation_detail_easings.js | Test timed out -
Flags: needinfo?(dakatsuka)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/af2062b5e308
Backed out changeset 588aa1f227f1 
https://hg.mozilla.org/integration/autoland/rev/93156e5811ec
Backed out changeset f951257a6856 
https://hg.mozilla.org/integration/autoland/rev/5a9b45a4a60d
Backed out changeset ac3850461834 
https://hg.mozilla.org/integration/autoland/rev/c2bbc9f00c63
Backed out changeset 693f32f563d2 for failing devtools' devtools/client/animationinspector/test/browser_animation_detail_easings.js. r=backout
I'll re-land if this try succeeded. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4849a663d829690af71307fddbc80b2d88dcd714
Flags: needinfo?(dakatsuka)
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/019aa83403f4
Part 1: Display easing in keyframes. r=pbro
https://hg.mozilla.org/integration/autoland/rev/72188596d6db
Part 2: Add tests for easing hint in keyframes. r=pbro
https://hg.mozilla.org/integration/autoland/rev/d568686e828c
Part 3: Display animation-timing-function if CSS Animations. r=pbro
https://hg.mozilla.org/integration/autoland/rev/eb050d2f2790
Part 4: Modify test for tooltip for animation-timing-function. r=pbro
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.