Closed Bug 1309468 Opened 8 years ago Closed 7 years ago

CSS Animation's animation-timing-function curve should be visualized in summary graph

Categories

(DevTools :: Inspector: Animations, enhancement, P2)

Product:
DevTools
Component:
Inspector: Animations
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(13 files)

proto1.png
633.48 KB, image/png
Details
Bug 1309468 - Part 1: Display effect easing curve and keyframes easing curve in summary graph.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1309468 - Part 2: Change animation height to taller.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1309468 - Part 3: Add tests for summary graph.
59 bytes, text/x-review-board-request
pbro
: review+
Details
compare-graph-opacity.png
77.11 KB, image/png
Details
Bug 1309468 - Part 4: Make ProgressGraphHelper's usage to fit to SummaryGraphHelper.
59 bytes, text/x-review-board-request
pbro
: review+
Details
name-label.jpeg
578.12 KB, image/jpeg
Details
with-stroke.png
69.10 KB, image/png
Details
with-stroke-2.png
70.38 KB, image/png
Details
stroke-and-end.png
70.37 KB, image/png
Details
Bug 1309468 - Part 5: Change the name label design.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1309468 - Part 6: Disappear horizontal scrollbar of animation-timeline.
59 bytes, text/x-review-board-request
pbro
: review+
Details
dark-theme.png
61.43 KB, image/png
Details 
We are implementing to display animation timing-function in bug 1210795.
In the bug, we display the result of effect easing as the graph. However, the graph shows 'linear' lines because animation-timing-function is executed as keyframe easing.
Inspector bug triage (filter on CLIMBING SHOES).
Severity: normal → enhancement
Priority: -- → P2
Blocks: 1280937
Depends on: 1210795
Assignee: nobody → daisuke
Just some notes from today's discussion with Daisuke.

* We want to basically sample keyframe easing and have it show up in the summary
  graph.
* 99% of the time, keyframe easing will be the same for all properties but
  sometimes it will differ by property.
* For the differing case, we overlay the different property curves and make each
  semi-transparent.
  e.g. if we have three curves, we make all three have opacity: 0.33. That way,
       in places where the curves are equal, the graph is completely solid.
* We *don't* want to reflect the actual keyframe property values or their
  distances in this view, it's just timing--that way if multiple different
  properties have the same easing, their curves will overlap precisely even if
  their values are quite different.

One idea for doing this is to take something like:

  @keyframes hoge {
    0% {
      opacity: 0;
      transform: scale(0.5);
    }
    0% {
      animation-timing-function: steps(2);
      width: 200px;
    }
    50% {
      opacity: 0.7;
    }
    100% {
      opacity: 1;
      transform: scale(1);
      width: 300px;
    }
  }

And sample the result of getComputedStyle(elem).opacity with the following
different animations:

  // Opacity
  { offset: 0, opacity: 0, easing: 'ease' }
  { offset: 0.5, opacity: 0.5, easing: 'ease' }
  { offset: 1, opacity: 1 }

  // Width
  { offset: 0, opacity: 0, easing: 'steps(2)' }
  { offset: 1, opacity: 1 }

  // Transform
  { offset: 0, opacity: 0, easing: 'ease' }
  { offset: 1, opacity: 1 }

Notice how the opacity value is equal to the offset, not the actual property
value since we're just interested in time.

Perhaps using the getProperties() API will make this easier?

Regarding the number of curves, I think we'd probably want to sort through and
combine properties that have identical easing and only generate one curve for
them. This is especially true if we use getProperties() since it will expand all
longhands.

We'd want to set the above keyframes on the same sort of animation as we
currently use for generating the summary view so that we get direction, effect
easing etc. applied.

That's the rough idea anyway. I'm not really sure if it works yet or not.
Oh, one other thing I forgot. Currently we show effect easing in the summary view. In this bug we want to show keyframe easing for CSS animations/transitions. That presents the question: what if an animation has both? This could happen for script-animations or even for CSS animations/transitions (you can use the script API to add effect easing to a CSS animation/transition).

We both agreed that we should show both in the graph (i.e. sample the curve with both set on the animation); that it would be confusing if one type of graph showed one type of easing only.

In the case where we have a non-linear effect easing and at least one keyframe with non-linear easing, I suggest we add a dotted line to represent the effect easing. Then, when we come to editing, any controls we surface for adjusting the effect easing (in the summary view) could be easily understood as adjusting that dotted line. (I assume we'd adjust keyframe easing in the keyframe view.)
Attached image proto1.png — 
I have made a prototype.

The summary graph displays as following.
* Display ffect easing as stroke.
* Display keyframe's easing as semitransparent fill.

This change can be not only for CSS Animation, but Script Animation and Transition.

Thanks.
Depends on: 1210796
Hi Patrik,
I wrote some patches for this.
These patches assume on the codes of bug 1210796.
Although the bug is not landed yet, could you review these patches?

Thanks.
Comment on attachment 8854655 [details]
Bug 1309468 - Part 1: Display effect easing curve and keyframes easing curve in summary graph.

https://reviewboard.mozilla.org/r/126594/#review131358

Looks good. I have a few comments, and I haven't yet tested this locally. So here's a first review at least.

::: devtools/client/animationinspector/components/animation-time-block.js:24
(Diff revision 1)
>  // Show max 10 iterations for infinite animations
>  // to give users a clue that the animation does repeat.
>  const MAX_INFINITE_ANIMATIONS_ITERATIONS = 10;
>  
> +// Minimum opacity for semitransparent fill color for keyframes's easing graph.
> +const MIN_KEYFRAMES_EASING_OPACITY = .5;

Can you explain why .5 please?
If there are 3 graphs, then they will each be 50% transparent, instead of 33%. Is this wanted?

::: devtools/client/animationinspector/components/animation-time-block.js:294
(Diff revision 1)
> +  graphHelper.setClosePathNeeded(false);
> +  renderGraph(parentEl, state, totalDisplayedDuration, "effect-easing", graphHelper);
> +}
> +
> +/**
> + * Render effect easing graph.

This comment is a copy of the `renderEffectEasingGraph`. Please explain here exactly what graph gets rendered.

::: devtools/client/animationinspector/components/animation-timeline.js:493
(Diff revision 1)
> +      const tracks = yield this.getTracks(animation);
>        let timeBlock = new AnimationTimeBlock();
>        timeBlock.init(timeBlockEl);
> -      timeBlock.render(animation);
> +      timeBlock.render(animation, tracks);
>        this.timeBlocks.push(timeBlock);
> +      this.tracksMap[animation] = tracks;

`animation` is an object, right? If so, it can't be used as a key for `this.tracksMap` since it's just a plain old object, it can only have string keys.
Unless, I guess, it has a toString that generates something unique.

You could otherwise make `this.tracksMap` a WeakMap instead.

::: devtools/client/animationinspector/components/animation-timeline.js:517
(Diff revision 1)
> -    // Display animation's detail if there is only one animation,
> -    // or the previously displayed animation is included in timeline list.
>      if (this.animations.length === 1) {
> -      this.onAnimationSelected(null, this.animations[0]);
> -      return;
> -    }
> +      // Display animation's detail if there is only one animation,
> +      yield this.onAnimationSelected(null, this.animations[0]);
> +    } else if (!this.animationRootEl.classList.contains("animation-detail-visible")) {

It's a little weird having an empty `else if` in the middle here. I think it would be better to move it before this if/elseif/else block as an early return thing.

::: devtools/client/animationinspector/graph-helper.js:248
(Diff revision 1)
>  /**
> + * Get a helper function which returns the segment coord from given time.
> + * @return {Object} A segmentHelper object that has the following properties:
> + * - animation: The script animation used to get the progress
> + * - endTime: The end time of the animation
> + * - getSegment: Helper function that, given a time,
> + *               will calculate the progress through the dummy animation.
> + */

This comment is out of date now and should be removed.

::: devtools/client/animationinspector/graph-helper.js:257
(Diff revision 1)
> + * - endTime: The end time of the animation
> + * - getSegment: Helper function that, given a time,
> + *               will calculate the progress through the dummy animation.
> + */
> +/**
> + * @param {Object} win - window object.

Instead, you should add a first comment line here that describes what this class is about, what it does, how it is intended to be used.

::: devtools/client/animationinspector/graph-helper.js:259
(Diff revision 1)
> + *               will calculate the progress through the dummy animation.
> + */
> +/**
> + * @param {Object} win - window object.
> + * @param {Object} state - animation state.
> + * @param {Object} keyframes - @see setKeyframes.

Is this parameter ever non null? Looking at the only place where you instantiate a `SummaryGraphHelper`, I see it is null. And then later you call `setKeyframes`.
If this is the case, then, you can remove the parameter altogether and just call `setKeyframes`.

::: devtools/client/animationinspector/graph-helper.js:271
(Diff revision 1)
> +  this.progressValueHelperFunction = this.getProgressValueHelperFunction();
> +  this.opacityValueHelperFunction = this.getOpacityValueHelperFunction();

Setting these 2 properties seems unnecessarily complex. In fact, even setting `this.valueHelperFunction` doesn't seem needed.

You could, at the end of `setKeyframes`, do something like `this.hasFrames = !!frames;`.

Then, inside `getSegment`, you could simply do:

`const value = this.hasFrames ? this.getOpacityValue : this.getProgressValue();`

You would also need to make `getProgressValueHelperFunction` like this instead:

```
getProgressValue: function () {
  return Math.max(this.animation.effect.getComputedTimeing().progress, 0);
}
```

Same for `getOpacityValueHelperFunction`.

This way, you can delete `this.valueHelperFunction`, `this.opacityValueHelperFunction` and `this.progressValueHelperFunction`.
Seems easier to understand to me.

::: devtools/client/animationinspector/graph-helper.js:325
(Diff revision 1)
> +  /*
> +   * Set animation behavior.
> +   * In Animation::SetCurrentTime spec, even if current time of animation is over
> +   * endTime, the progress is changed. Likewise, in case the time is less than 0.
> +   * If set true, we prevent the time to make the same animation behavior as the original.
> +   * @param {bool} isOriginalBehavior - true: original behavior
> +   *                                    false: according to spec.
> +   */
> +  setOriginalBehavior: function (isOriginalBehavior) {
> +    this.isOriginalBehavior = isOriginalBehavior;
> +  },

If I understand correctly, this class is used in just one place, and isOriginalBehavior is set to true. So we maybe don't need to expose this at all. Maybe let's just always behave as if isOriginalBehavior was true, but remove the property altogether, the if/else further below, the setOriginalBehavior, and the extra parameter in the constructor.

::: devtools/client/animationinspector/graph-helper.js:488
(Diff revision 1)
>   * @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)
>   * @return {Element} path element.
>   */
> -function appendPathElement(parentEl, pathSegments, cls) {
> +function appendPathElement(parentEl, pathSegments, cls, isClosePathNeeded = true) {

Is this function used in other places than inside `SummaryGraphHelper` now? 
If not, then you could move the code inside `SummaryGraphHelper.appendPathElement` instead of having a separate function here.
Attachment #8854655 - Flags: review?(pbrosset)
Comment on attachment 8854657 [details]
Bug 1309468 - Part 3: Add tests for summary graph.

https://reviewboard.mozilla.org/r/126598/#review131382

Looks good to me.

::: devtools/client/animationinspector/test/browser_animation_summarygraph_for_multiple_easings.js:126
(Diff revision 1)
> +    if (!testcase) {
> +      return;
> +    }
> +
> +    info(`Test effect easing graph of ${ state.name }`);
> +    const effectGEl = timeBlock.containerEl.querySelector(".effect-easing");

typo here? `effectEl` instead of `effectGEl`?

::: devtools/client/animationinspector/test/browser_animation_timeline_takes_rate_into_account.js:35
(Diff revision 1)
> +    const dur = getDuration(groupEl.querySelector("path"));
> +    if (!duration) {
> +      duration = dur;
> +      return;
> +    }
> +    is(duration, dur, "The durations shuld be same at all pathes in one group");

nit: plural of path is paths, not pathes.

::: devtools/client/animationinspector/test/browser_animation_timeline_takes_rate_into_account.js:49
(Diff revision 1)
> +    const dur = getDuration(groupEl.querySelector("path"));
> +    if (!duration2) {
> +      duration2 = dur;
> +      return;
> +    }
> +    is(duration2, dur, "The durations shuld be same at all pathes in one group");

same here.
Attachment #8854657 - Flags: review?(pbrosset) → review+
Comment on attachment 8854655 [details]
Bug 1309468 - Part 1: Display effect easing curve and keyframes easing curve in summary graph.

https://reviewboard.mozilla.org/r/126594/#review131358

> Can you explain why .5 please?
> If there are 3 graphs, then they will each be 50% transparent, instead of 33%. Is this wanted?

Thank you for your review, Patrick!

Yeah, at first although I also thought so, if more than 3 graphs, hard to see each graph shape since too pale.
I will attach the screenshot.

> `animation` is an object, right? If so, it can't be used as a key for `this.tracksMap` since it's just a plain old object, it can only have string keys.
> Unless, I guess, it has a toString that generates something unique.
> 
> You could otherwise make `this.tracksMap` a WeakMap instead.

Thanks, will use WeakMap.

> If I understand correctly, this class is used in just one place, and isOriginalBehavior is set to true. So we maybe don't need to expose this at all. Maybe let's just always behave as if isOriginalBehavior was true, but remove the property altogether, the if/else further below, the setOriginalBehavior, and the extra parameter in the constructor.

This is used to create a graph shape for negative delay and negative endDelay in end of renderGraph.
However, same to setKeyframes, we can remove a parameter from the constructor.
Thanks!

> Is this function used in other places than inside `SummaryGraphHelper` now? 
> If not, then you could move the code inside `SummaryGraphHelper.appendPathElement` instead of having a separate function here.

We are using this function in keyframes.js to create detail graphs.
However, I am thinking ProgressGraphHelper should have appendPathElement as same to SummaryGraphHelper for consistency of how to use.
I'll do this in another patch.
I attach a screenshots to compare the opacity.
One is divided by length of type of keyframe's easing.
Another one is minimum 0.5
How do you think?

The screenshots were taken using following JS code.
div.animate([
  {
    offset: 0,
    opacity: 0,
    easing: "steps(5, start)"
  },
  {
    offset: 0,
    backgroundColor: "red",
    easing: "steps(2)"
  },
  {
    offset: 0,
    width: "300px",
    easing: "steps(3)"
  },
  {
    offset: 0,
    color: "red",
    easing: "steps(4)"
  },
  {
    offset: 0,
    height: "300px",
    easing: "steps(5)"
  },
  {
    offset: 0,
    marginTop: "300px",
    easing: "steps(6)"
  },
  {
    offset: 0,
    marginRight: "300px",
    easing: "steps(7)"
  },
  {
    offset: 0,
    marginBottom: "300px",
    easing: "steps(8)"
  },
  {
    offset: 0,
    marginLeft: "300px",
    easing: "steps(9)"
  },
], 1000 * 10);
Comment on attachment 8854657 [details]
Bug 1309468 - Part 3: Add tests for summary graph.

https://reviewboard.mozilla.org/r/126598/#review131382

> typo here? `effectEl` instead of `effectGEl`?

I had wanted to describe <g> :)
But, I will remove G. Also, keyframeGEls too.
Comment on attachment 8854655 [details]
Bug 1309468 - Part 1: Display effect easing curve and keyframes easing curve in summary graph.

https://reviewboard.mozilla.org/r/126594/#review132902

A few additional comments. And I haven't tested this yet. I'll do this early next week, as soon as the dependency has landed. I'll give you better feedback then.

::: devtools/client/animationinspector/components/animation-timeline.js:351
(Diff revision 2)
>      selectedAnimationEl.classList.add("selected");
>      this.animationRootEl.classList.add("animation-detail-visible");
> +    // Don't render if the detail displays same animation already.
>      if (animation !== this.details.animation) {
>        this.selectedAnimation = animation;
> -      // Don't render if the detail displays same animation already.
> +      yield this.details.render(animation, this.tracksMap[animation]);

`this.tracksMap` is a WeakMap now, so you can't access its values using `[animation]`. You need to use `this.tracksMap.get(animation);`

::: devtools/client/animationinspector/components/animation-timeline.js:493
(Diff revision 2)
> +      const tracks = yield this.getTracks(animation);
>        let timeBlock = new AnimationTimeBlock();
>        timeBlock.init(timeBlockEl);
> -      timeBlock.render(animation);
> +      timeBlock.render(animation, tracks);
>        this.timeBlocks.push(timeBlock);
> +      this.tracksMap[animation] = tracks;

Same for setting values, you need to use the WeakMap's API:
`this.tracksMap.set(animation, tracks);`

::: devtools/client/animationinspector/graph-helper.js:249
(Diff revision 2)
>  };
>  
>  exports.ProgressGraphHelper = ProgressGraphHelper;
>  
>  /**
> + * This class is to use for making the summary graph in animation-timeline.

nit: rephrasing: "This class is used for creating thne summary graph in animation-timeline"

::: devtools/client/animationinspector/graph-helper.js:250
(Diff revision 2)
>  
>  exports.ProgressGraphHelper = ProgressGraphHelper;
>  
>  /**
> + * This class is to use for making the summary graph in animation-timeline.
> + * The graph shape changes by the following methods.

nit: rephrasing "The shape of the graph can be changed by using the following methods:"
Attachment #8854655 - Flags: review?(pbrosset)
I rebased this patches to m-c since bug 1210796 was landed.
Also, fixed according to your review.
Comment on attachment 8854655 [details]
Bug 1309468 - Part 1: Display effect easing curve and keyframes easing curve in summary graph.

https://reviewboard.mozilla.org/r/126594/#review135672

I've applied this commit locally and tested it a little bit.
First of all: wow! Really cool to see curves and steps instead of lines. I think this helps a lot.

Here are a few comments about the UI:

- I tested with an animation that used steps(): http://s.codepen.io/rachelnabors/debug/zHeir and the summary graph looked a bit weird. I think it showed only 1 third of the steps as real "steps" in the graph, the other steps were straight lines. See here: https://www.dropbox.com/s/cqdmfik8j7pxjcn/steps-function-summary-graph.PNG?dl=0
--> It would be good to fix this in this patch if possible.

- Now that the summary graph is becoming even more important than before (its shape conveys a lot of information), the animation name is getting in the way. It is displayed on top of the graph and could hide some important information. You are making the row height bigger now, so I would like to suggest a change in this bug. Something like this: https://www.dropbox.com/s/0iee418ub50fvx1/move-animation-name-above-target.PNG?dl=0 This way, the animation name just sits inside the sidebar, next to the target (where there is room, because we're making each row taller). Doing this will require to make sure there's an ellipsis if the name is wider than the sidebar.
--> This can be done in another patch

- There's often a horizontal scrollbar in the animation panel that shouldn't be there. https://www.dropbox.com/s/e5nrx638eqqvo62/horizontal-scroll-bar-animation-panel.gif?dl=0 . To reproduce this, you can go to apple.com., and just move your mouse over the top menubar. This triggers a lot of transitions, and quickly ends up creating a scrollbar in the animation panel.
I don't think this is related to this commit, this might actually have been there from before. Feel free to file another bug for this if you prefer.
Attachment #8854655 - Flags: review?(pbrosset)
Comment on attachment 8854656 [details]
Bug 1309468 - Part 2: Change animation height to taller.

https://reviewboard.mozilla.org/r/126596/#review135698

::: devtools/client/themes/animationinspector.css:67
(Diff revision 3)
>    /* How wide should the sidebar be (should be wide enough to contain long
>       property names like 'border-bottom-right-radius' without ellipsis) */
>    --timeline-sidebar-width: 200px;
>    /* How high should animations displayed in the timeline be */
> -  --timeline-animation-height: 20px;
> +  --timeline-animation-height: 30px;
> +  /* How high should animations displayed in the detail be */

nit: some rephrasing needed:

How high should animated properties displayed in the details view be

::: devtools/client/themes/animationinspector.css:315
(Diff revision 3)
> -  min-height: var(--timeline-animation-height);
> +  min-height: var(--detail-animation-height);
>    cursor: col-resize;
>    -moz-user-select: none;
> +  height: var(--detail-animation-height);

The time-header is more a toolbar than an animation, right? So I think we should use --toolbar-height here rather than --detail-animation-height.
Also, why do we have both a min-height and a height? I don't remember why we had min-height before, but it seems to me that we should only have height, and nothing else.

::: devtools/client/themes/animationinspector.css:335
(Diff revision 3)
>    position: sticky;
>    top: 0;
>    background-color: var(--theme-body-background);
>    border-bottom: 1px solid var(--time-graduation-border-color);
>    z-index: 3;
> -  height: var(--timeline-animation-height);
> +  height: var(--toolbar-height);

In fact, we already have a height on this wrapper element, so the .time-header child could have height:100% only, instead of --toolbar-height.

::: devtools/client/themes/animationinspector.css:341
(Diff revision 3)
>    width: 100%;
>    overflow: hidden;
>  }
>  
>  .animation-timeline .time-body {
> -  top: var(--timeline-animation-height);
> +  top: var(--detail-animation-height);

This should become --toolbar-height if you do the changes explained just before.
Attachment #8854656 - Flags: review?(pbrosset) → review+
Comment on attachment 8857382 [details]
Bug 1309468 - Part 4: Make ProgressGraphHelper's usage to fit to SummaryGraphHelper.

https://reviewboard.mozilla.org/r/129372/#review135704
Attachment #8857382 - Flags: review?(pbrosset) → review+
Comment on attachment 8854655 [details]
Bug 1309468 - Part 1: Display effect easing curve and keyframes easing curve in summary graph.

https://reviewboard.mozilla.org/r/126594/#review135672

Thank you for your kindly feedback!
(And, I'm sorry for my late replying..)

1. steps() problem
Okay, I'll fix in this patch.

2. Animation name label
I see, will try to put above the target object in another patch.
(Or, it may work just put after the taget?)

3. Horizontal scrollbar
It could reproduce in Nightly, but not Release(53.02)
The cause may be bug 1210796.
I'm sorry, will fix in this bug.

Thanks!
Attached image name-label.jpeg — 
I taken some screenshots for name label.

Above the target:
I feel it is crowded a bit. I wonder we may need more height?

After the target:
Looks not so bad for me. However, it might be vanished since depends on the label width. If the boundary of between right pane (graph) and left pane (labels) is movable, it might be resolved. 

Center on Graph:
Also, not so bad, I think.
This similar to Chrome one.

How do you think??
Also, this design might depend on bug 1210363 ( animation grouping ).
Flags: needinfo?(pbrosset)
Attached image with-stroke.png — 
Brian and I discussed about the name label design.
We attach another screenshot.
It is similar to type of "center of graph", but the label has outline stroke.
Attached image with-stroke-2.png — 
Minor change:
stroke-opacity: 1 -> 0.5
stroke-width: 3 -> 4
Attached image stroke-and-end.png — 
And another one, put the label to end of the graph.
I'll write a patch with this prototype.
(In reply to Daisuke Akatsuka (:daisuke) from comment #28)
> Created attachment 8865350 [details]
> name-label.jpeg
> 
> I taken some screenshots for name label.
> 
> Above the target:
> I feel it is crowded a bit. I wonder we may need more height?
Yeah I agree, it feels a bit crowded.
> After the target:
> Looks not so bad for me. However, it might be vanished since depends on the
> label width. If the boundary of between right pane (graph) and left pane
> (labels) is movable, it might be resolved. 
Your screenshot looks good, but in many cases target DOM elements and animation names will be long so we won't see much of it. Indeed, making the left panel resizable would solve this.
> Center on Graph:
> Also, not so bad, I think.
> This similar to Chrome one.
I'm still afraid that this might hide important visual information, but it does look good in your screenshot.
> How do you think??
Let's not worry about this now. As I said before, we can do this in another bug. And we can work with UX a bit to iron the details out. So, if you want to move the label to the middle, and use a stroke like comment 29, that's fine with me. We can then change it again later if we think it's necessary.

(In reply to Daisuke Akatsuka (:daisuke) from comment #29)
> Created attachment 8866183 [details]
> with-stroke.png
> 
> Brian and I discussed about the name label design.
> We attach another screenshot.
> It is similar to type of "center of graph", but the label has outline stroke.
Looks good to me.
One idea though: what about making it centered in the timeline. So that all labels are always aligned at the center of the whole timeline, not just inside their own graphs, which may vary in size.
Flags: needinfo?(pbrosset)
(In reply to Daisuke Akatsuka (:daisuke) from comment #31)
> Created attachment 8867608 [details]
> stroke-and-end.png
> 
> And another one, put the label to end of the graph.
> I'll write a patch with this prototype.
Ah sorry, I hadn't seen this one. Looks good to me. Either that or centered in the timeline would be OK to me.
Comment on attachment 8867612 [details]
Bug 1309468 - Part 5: Change the name label design.

https://reviewboard.mozilla.org/r/139178/#review143928

::: devtools/client/animationinspector/components/animation-time-block.js:117
(Diff revision 1)
> -        attributes: {
> +      attributes: {
> -          "class": "name",
> +        "class": "name",
> -          "title": this.getTooltipText(state)
> +        "title": this.getTooltipText(state)
> -        },
> +      }
> +    });
> +    createSVGNode({

nit: please add an empty line between these 2 node creation.

::: devtools/client/themes/animationinspector.css:447
(Diff revision 1)
> -  max-width: 50%;
> +  fill: black;
> +  stroke: white;

Did you test this with the dark theme?
Maybe we should invert this depending on which theme is chosen. We have plenty of CSS variables for our themes that you can use here instead of hard coding colors: \devtools\client\themes\variables.css
Attachment #8867612 - Flags: review?(pbrosset) → review+
Comment on attachment 8867613 [details]
Bug 1309468 - Part 6: Disappear horizontal scrollbar of animation-timeline.

https://reviewboard.mozilla.org/r/139180/#review143932
Attachment #8867613 - Flags: review?(pbrosset) → review+
Comment on attachment 8854655 [details]
Bug 1309468 - Part 1: Display effect easing curve and keyframes easing curve in summary graph.

https://reviewboard.mozilla.org/r/126594/#review145398

::: devtools/client/animationinspector/graph-helper.js:266
(Diff revisions 3 - 4)
>   * setOriginalBehavior:
>   *   In Animation::SetCurrentTime spec, even if current time of animation is over
>   *   the endTime, the progress is changed. Likewise, in case the time is less than 0.
>   *   If set true, prevent the time to make the same animation behavior as the original.
> + * setMinProgressThreshold:
> + *   SummaryGraphHelper searches and creates the summary graph untill the progress

s/untill/until

::: devtools/client/animationinspector/graph-helper.js:268
(Diff revisions 3 - 4)
> + *   So, if the threshold is too small, although can render the graph smoothly,
> + *   but the rendering performance will be down.

Some rephrasing needed I think. I suggest: "So, while setting a low threshold produces a smooth graph, it will have an effect on performance"

::: devtools/client/animationinspector/graph-helper.js
(Diff revisions 3 - 4)
>    this.animation = this.targetEl.animate(null, effectTiming);
>    this.animation.pause();
>    this.endTime = this.animation.effect.getComputedTiming().endTime;
>  
>    this.minSegmentDuration = minSegmentDuration;
> -  this.minProgressThreshold = minProgressThreshold;

Could we have a default value for this property now that you've moved it to a setter?
This would avoid having a nasty error is someone forgets to call the setter.

::: devtools/client/animationinspector/graph-helper.js:646
(Diff revisions 3 - 4)
>    const diffB = startB - endB;
>    return Math.sqrt(diffA * diffA + diffR * diffR + diffG * diffG + diffB * diffB);
>  }
> +
> +/**
> + * Return preferred progress threshold for given keyframes.

Can you add something like this to this comment:

"See the documentation of DURATION_RESOLUTION and DEFAULT_MIN_PROGRESS_THRESHOLD for more information regarding this"

::: devtools/client/animationinspector/test/browser_animation_refresh_on_added_animation.js:39
(Diff revisions 3 - 4)
>    }, panel, inspector);
>  
>    assertAnimationsDisplayed(panel, 0);
>  });
>  
> -function* changeElementAndWait(options, panel, inspector) {
> +function* changeElementAndWait(options, panel, inspector, isDetailDisplayed) {

Why did you add `isDetailDisplayed`? I don't see it being used in this test.
Attachment #8854655 - Flags: review?(pbrosset) → review+
Thank you for your review, Patrick!
I'll fix your points and land it!
Attached image dark-theme.png — 
Attached screenshot is for dark theme.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24136f1d3b32
Part 1: Display effect easing curve and keyframes easing curve in summary graph. r=pbro
https://hg.mozilla.org/integration/autoland/rev/4692fed20d4e
Part 2: Change animation height to taller. r=pbro
https://hg.mozilla.org/integration/autoland/rev/7d2a1c747e25
Part 3: Add tests for summary graph. r=pbro
https://hg.mozilla.org/integration/autoland/rev/b36e1c61812d
Part 4: Make ProgressGraphHelper's usage to fit to SummaryGraphHelper. r=pbro
https://hg.mozilla.org/integration/autoland/rev/05d113655441
Part 5: Change the name label design. r=pbro
https://hg.mozilla.org/integration/autoland/rev/03a6c5ed7602
Part 6: Disappear horizontal scrollbar of animation-timeline. r=pbro
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: