Closed Bug 1210796 Opened 9 years ago Closed 7 years ago

Display keyframes' timing-functions in the animation-inspector

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
enhancement

Tracking

(firefox55 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: pbro, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(30 files)

keyframe-timingfunction.png
33.33 KB, image/png
Details
prototype2.png
53.86 KB, image/png
Details
prototype3.png
233.37 KB, image/png
Details
prototype4.png
206.28 KB, image/png
Details
prototype5.png
286.13 KB, image/png
Details
prototype6.png
289.12 KB, image/png
Details
prototype6-dark.png
293.57 KB, image/png
Details
Bug 1210796 - Part 1: Add GetAnimationTypeForLonghand into nsIDOMWindowUtils to use in animationinspector of devtools.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1210796 - Part 2: Visualize each properties.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 3: Isolated timeline.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 4: Add animated property header.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 5: Add aniamted property's progress line tick.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 6: Fixed animation detail panel.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 7: Change selection logic.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 8: Add selection color to selected animation element.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 9: Add progress indicator.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 12: Unite common codes into utils.js and use.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 13: Add tests.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 10: Display animation's detail if the animation is only one in the list.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 11: Disable clicking on keyframes because we can’t calculate correct current time since the animation detail handles the animation progress not time.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 14: Add close button.
59 bytes, text/x-review-board-request
pbro
: review+
Details
detail-testing.png
505.68 KB, image/png
Details
detail-testing-dark.png
506.23 KB, image/png
Details
Bug 1210796 - Part 15: Displays the animation detail if the previously displayed animation is included in timeline list after refresh all UI.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1210796 - Part 16: Move unchanged properties to bottom in the list.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Screen Shot 2017-03-13 at 22.09.39.png
674.81 KB, image/png
Details
primalscreen-test.html
753 bytes, text/html
Details
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.
59 bytes, text/x-review-board-request
birtles
: review+
heycam
: review+
Details
test2.html
560 bytes, text/html
Details
devtools-test-sample.html
1.29 KB, text/html
Details 
Bug 1197100 adds a keyframes and animated properties panel in the animation-inspector, when an animation is selected.

When a keyframe gets selected, we should allow users to view/edit its timing-function. We could reuse the cubic-bezier editor tooltip for this.

See also bug 1210795 which is about showing the timing-function for the overall animation.
Depends on: 1228005
No longer depends on: 1197100
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
No longer blocks: 1201279
Bug triage (filter on CLIMBING SHOES).
Severity: normal → enhancement
Priority: -- → P2
Blocks: 1280937
I added some comments to bug 1210795 which overlap with this significantly.
Assignee: nobody → daisuke
I attach a screen shot of first prototype.
The test keyframes are below.
[ { offset: 0, opacity: 0, easing: "ease-in-out" },
  { offset: 0.5, opacity: 0.5, easing: "steps(3)" },
  { offset: 1, opacity: 1 } ]

I'd like to confirm and discuss about following points.
1. Which should we display the progress or the computed value? ( our first prototype is computed value https://bugzilla.mozilla.org/attachment.cgi?id=8770434 )
2. The time axis should be same to animation's time line ( bug 1210795 ) or isolate to focus keyframes more? ( this prototype is same to animation's. Our first prototype is isolation https://bugzilla.mozilla.org/attachment.cgi?id=8770434 )

Thanks!
(In reply to Daisuke Akatsuka (:daisuke) from comment #3)
> Created attachment 8789697 [details]
> keyframe-timingfunction.png
> 
> I attach a screen shot of first prototype.
> The test keyframes are below.
> [ { offset: 0, opacity: 0, easing: "ease-in-out" },
>   { offset: 0.5, opacity: 0.5, easing: "steps(3)" },
>   { offset: 1, opacity: 1 } ]

Nice. (Nit: I wonder if the grey is a bit dark?)

> I'd like to confirm and discuss about following points.
> 1. Which should we display the progress or the computed value? ( our first
> prototype is computed value
> https://bugzilla.mozilla.org/attachment.cgi?id=8770434 )

Good questions. I think progress is simpler but when we discussed this in London, I think most people felt that using the computed value was more intuitive? The difficulty with using the computed value is that for multi-dimensional values it is hard to know what value to use.

* For color values, we could use the hue as the vertical axis? (Perhaps in future, that could even be configurable by clicking it somehow?)
* For transform values, I think long-term we talked about splitting out the transform components into separate graphs? That would be more work that should probably happen in a different bug, however. Perhaps for now, we could use the notion of 'distance' from bug 1272549.
* For other list values, I think there's probably some way of exploiting the distance calculations to generate a suitable scale (for types that don't have a notion of distance, I think it will degenerate to being the same as progress).

> 2. The time axis should be same to animation's time line ( bug 1210795 ) or
> isolate to focus keyframes more? ( this prototype is same to animation's.
> Our first prototype is isolation
> https://bugzilla.mozilla.org/attachment.cgi?id=8770434 )

I lean towards isolating this because I think it will make it easier to edit/read the graph when the iteration duration is short?

However, if the size of the keyframe graph is nearly the same size as the summary graph, maybe it will be confusing?
Thanks, Brian!
I'll try to do that (1. computed value, 2. isolation)
Attached image prototype2.png — 
I attached second prototype screenshot.
The test keyframes are;

[ { offset: 0, easing: "ease-in",
    backgroundColor: "rgb(255, 255, 0)",
    opacity: 0,
    transform: "translate(0px, 0px) rotate(0deg)",
    width: "0px"
  },
  { offset: 0.5, easing: "steps(3)",
    backgroundColor: "blue",
    opacity: 0.5,
    transform: "translate(100px, 100px) rotate(90deg)",
    width: "100px"
  },
  { offset: 1,
    backgroundColor: "cyan",
    opacity: 1,
    transform: "translate(150px, 150px) rotate(90deg)",
    width: "150px"
  }]

I'm using nsDOMWindowUtils.computeAnimationDistance to calculate the graph height of all values. 
The "transform" graph is making flat since this method does not support "transform" yet.
Attached image prototype3.png — 
This screenshot is a prototype version 3.
The differences are,
* The keyframe graph isolates as one iteration timeline.
* Height of the 'color' animation type changes to 100%.
* Colors of 'coord' and except 'color', 'coord', 'float' type are changed.

Also, we should think about the scrubber.
Spoke with Daisuke about the scrubber, and there are a few issues we covered:

* Although the keyframes view shows the keyframe values for a single iteration, the width of the graphs in the keyframes view does not match the width of a single iteration in the summary view. This is on purpose: the keyframes view needs to represent more information and if we condense it to the width of an iteration it will be too small to see. As a result, the position of the scrubber doesn't line up with the progress within the keyframes view.

* We could try to break the scrubber so it actually jumps to the correct position in the keyframe view but it would look weird. Also, you probably want to drag the scrubber within the keyframes view, but doing that is somewhat difficult.[1]

Daisuke raised the question of whether you really want to expand more than one animation at a time? If not, it seems like we could fix these problems (and possibly some of the visual awkwardness of having all these colors mixed in together) by having a separate fixed panel at the bottom of the view that pops up with the keyframe information when you select an animation. If you select a different animation, the panel contents are replaced with that animation's keyframes, i.e. you can only inspect the keyframes of one animation at a time.

If we do that, we don't need to break the scrubber line and there's only one (obvious) place where you can scrub. We'd still show the playback position within the keyframes view, but in a way that doesn't suggest you can alter it from there.

Daisuke is going to work on a prototype of this.

[1] As for why this is difficult there are a few reasons. Reason 1: the original animation might not run for a full iteration so what do we do if you drag somewhere in the keyframes view that isn't part of the original animation?

Reason 2: the time within the keyframes view is adjusted based on the effect easing and these easing functions are not always invertible, e.g. step timing functions. If you have a step timing function applied to the effect, there are places in the keyframes you'll never hit. If we allow scrubbing within the keyframes view and you position it on one of those times you never hit--what time do we set on the animation?

For easing functions that are not monotonically increasing you have a similar problem where you might visit the same point in the keyframes twice or more. So which time do you use for the animation in that case?
(Also, I think another advantage of having a separate keyframes panel is that we could collapse shorthands there when all the longhands have the same animations values, and then make expandable. Trying to do that with the current setup would mean there would be two levels of expansion which somehow feels clunky to me.)
Attached image prototype4.png — 
This screenshot is a prototype version 4.

The differences are,
* Animated properties panel is stored in an accordion UI.
* Add background color to selected .animation element.
* Add a scrubber( blue one ) to keyframes timeline.

Thanks.
Hi Helen!
Could you give advices to us about UI?

color:
 Is it too much color?

layout:
 Currently, I stored animated properties pane into an accordion UI.
 But we should probably use fixed position pane? (and close button).

scrubber:
 We should use another design?

Thanks.
Flags: needinfo?(hholmes)
Attached image prototype5.png — 
I discussed with Helen in all hands.
As her answer, the layout, color of each properties, color and style of progress indicator is no problem ( attached screenshot ).
I'll implement toward this.

Also, will modify the following points:
* Change the visualize to progress based ( not time based ).
* Make user don't scrub the progress indicator on this component.
* Change the tick labels to 0.0, 0.5, 1.0
* Unite the similar codes in animation-timeline-block.js as common function.
Flags: needinfo?(hholmes)
Attached image prototype6.png — 
We changed properties color.
* opacity: pink
* transform: orange
* other: gray
Attached image prototype6-dark.png — 
The attachment is a screenshot in dark theme.
Comment on attachment 8823596 [details]
Bug 1210796 - Part 1: Add GetAnimationTypeForLonghand into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/102134/#review102468

It's not clear to me why the function returns 'none' for shorthand properties.  I think that's because I don't quite understand what the purpose of this function is.  Could you please add a comment to explain the purpose in the commit message?

I think all of test cases here do not need Web Animations API at all, so we can run it without the pref. It's a bit strange to me that the test file in dom/animation/test even though the implementation is in dom/base/nsDOMWindowUtils.cpp.

::: dom/animation/test/mozilla/file_get-animation-type.html:3
(Diff revision 1)
> +<!doctype html>
> +<meta charset=utf-8>
> +<script src="../testcommon.js"></script>

I think testcommon.js can be dropped because functions in testcommon.js are not used at all.

::: dom/animation/test/mozilla/file_get-animation-type.html:103
(Diff revision 1)
> +  try {
> +    SpecialPowers.DOMWindowUtils.getAnimationType("invalid");
> +  } catch (e) {
> +    assert_equals(e.result,
> +                  SpecialPowers.Cr.NS_ERROR_ILLEGAL_VALUE,
> +                  "Invalid property should throw as NS_ERROR_ILLEGAL_VALUE");

Can't we use assert_throws?

::: dom/animation/test/mozilla/file_get-animation-type.html:112
(Diff revision 1)
> +test(t => {
> +  try {
> +    SpecialPowers.DOMWindowUtils.getAnimationType();
> +  } catch (e) {
> +    assert_equals(e.result,
> +                  SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS,
> +                  "No parameter should throw as NS_ERROR_XPC_NOT_ENOUGH_ARGS");
> +  }
> +}, "Test for no parameter");

I don't think this test case is useful.

::: dom/animation/test/mozilla/file_get-animation-type.html:122
(Diff revision 1)
> +test(t => {
> +  try {
> +    SpecialPowers.DOMWindowUtils.getAnimationType(null);
> +  } catch (e) {
> +    assert_equals(e.result,
> +                  SpecialPowers.Cr.NS_ERROR_ILLEGAL_VALUE,
> +                  "Null parameter should throw as NS_ERROR_ILLEGAL_VALUE");
> +  }
> +}, "Test for null parameter");

Likewise.

::: dom/base/nsDOMWindowUtils.cpp:2735
(Diff revision 1)
> +    case eStyleAnimType_Custom: {
> +      aResult.AssignLiteral("custom");
> +      break;
> +    }

Nit: These curly brackets are unncessary.

::: dom/base/nsDOMWindowUtils.cpp:2775
(Diff revision 1)
> +    default: {
> +      aResult.AssignLiteral("none");
> +    }

We should definetly drop this "default", so that "-Wswitch" option can catch the case that we forget to add a new animation type here once we have the new animation type.

::: dom/interfaces/base/nsIDOMWindowUtils.idl:1564
(Diff revision 1)
>                                    in AString property,
>                                    in AString value1,
>                                    in AString value2);
>  
>    /**
> +   * Returns animation type of the given parameters.

We should document |aProperty| is a CSS property name not IDL attribute.
Comment on attachment 8823596 [details]
Bug 1210796 - Part 1: Add GetAnimationTypeForLonghand into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/102134/#review102468

Thanks Hiro!

I'd like to use this animation type to decide how to calculate the graph shape and color of each property in devtools. The shorthand properties are not necessary since they are extracted to thire longhand properties in devtool. So I returned 'none' for shorthands.
However, I have re-thought that we should return actual animation type even if property is shorthand property, if we add the function as name is GetAnimationType to DOMUtilWindow.
Thank you very much.

Also, I'll move the test to dom/base/test.
Comment on attachment 8823596 [details]
Bug 1210796 - Part 1: Add GetAnimationTypeForLonghand into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/102134/#review102468

I found the test file for DOMWindowUtils.
dom/base/test/test_domwindowutils.html
I'll add our test to here.
Really sorry about the delay Daisuke. I was back last week after a couple of weeks of break, and didn't have time to address review requests then. I have started reviewing code this week again, and I will start reviewing your patches tomorrow.
(In reply to Patrick Brosset <:pbro> from comment #38)
> Really sorry about the delay Daisuke. I was back last week after a couple of
> weeks of break, and didn't have time to address review requests then. I have
> started reviewing code this week again, and I will start reviewing your
> patches tomorrow.

Thank you very much, Patrick!
Comment on attachment 8823596 [details]
Bug 1210796 - Part 1: Add GetAnimationTypeForLonghand into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/102134/#review102468

I re-re-think about GetAnimationType.
I'd like to ignore shorthand property since we have no plan to use shorthand property.
To clarify the function, I want to rename GetAnimationType to GetAnimationTypeForLonghand etc.
Comment on attachment 8823596 [details]
Bug 1210796 - Part 1: Add GetAnimationTypeForLonghand into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/102134/#review104028

::: dom/base/test/test_domwindowutils.html:144
(Diff revision 2)
> +    const propertyName = testcase.propertyName;
> +    const expectedType = testcase.expectedType;
> +    is(utils.getAnimationTypeForLonghand(propertyName), expectedType,
> +       `Animation type should be ${ expectedType }`);
> +  });
> +  SimpleTest.executeSoon(next);

Do we really need executeSoon() instead of next()?
Attachment #8823596 - Flags: review?(hikezoe) → review+
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review104320

::: devtools/client/animationinspector/components/keyframes.js:22
(Diff revision 2)
>  /**
>   * UI component responsible for displaying a list of keyframes.
>   */
>  function Keyframes() {

This UI component now does more than show keyframes only. It also shows a graph for the progression of a property through one iteration of the animation. So I think the comment and the name of the class need to be changed to reflect this.

::: devtools/client/animationinspector/components/keyframes.js:59
(Diff revision 2)
>  
>      let iterationStartOffset =
>        animation.state.iterationStart % 1 == 0
>        ? 0
>        : 1 - animation.state.iterationStart % 1;
> +    // Create graph element.

nit: Please add an empty line before this comment

::: devtools/client/animationinspector/components/keyframes.js:149
(Diff revision 2)
>      });
>    }
>  };
> +
> +/**
> + * Render keyframes.

nit: this comment shouldn't only restate the function name, but instead bring precisions about what it does.

::: devtools/client/animationinspector/components/keyframes.js:156
(Diff revision 2)
> + * @param {Number} duration - Duration of one iteration.
> + * @param {Number} minSegmentDuration - Minimum segment duration.
> + * @param {Number} minProgressThreshold - Minimum progress threshold.
> + * @param {Object} graphHelper - The object returned by getGraphHelper.
> + */
> +function renderKeyframes(parentEl, duration, minSegmentDuration,

I think we should change the name of this function to something like renderPropertyGraph.

::: devtools/client/animationinspector/components/keyframes.js:161
(Diff revision 2)
> +function renderKeyframes(parentEl, duration, minSegmentDuration,
> +                         minProgressThreshold, graphHelper) {
> +  const segments = createPathSegments(0, duration, minSegmentDuration,
> +                                      minProgressThreshold, graphHelper);
> +  const propertyIDL = graphHelper.getPropertyIDL();
> +  const pathClass = propertyIDL === "opacity" || propertyIDL === "transform"

I feel like these 2 specific cases should be encoded inside getAnimationType, so that setting pathClass can be done like `const pathClass = graphHelper.getAnimationType();`

::: devtools/client/animationinspector/components/keyframes.js:206
(Diff revision 2)
> +/**
> + * Get a graph helper object which returns the segment coord from given time and
> + * animation type.
> + * @param {Element} targetEl - element which is animated in this helper.
> + * @param {String} propertName - CSS property name (e.g. background-color).
> + * @param {String} propertIDLName - CSS IDL name (e.g. backgroundColor).

What does IDL mean in this context? Could we maybe use something a little less cryptic?

::: devtools/client/animationinspector/components/keyframes.js:220
(Diff revision 2)
> +                        propertyIDLName, keyframes, effectTiming) {
> +  // Animation type
> +  const win = targetEl.ownerDocument.defaultView;
> +  const DOMWindowUtils = win.QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIDOMWindowUtils);
> +  const animationType = DOMWindowUtils.getAnimationTypeForLonghand(propertyName);

Ah, unfortunately we can't do this. We've just gone through an entire project that aimed at removing XUL and Chrome JS from our front-end so that the toolbox would work in a content tab (and possibly in other browsers too).
Here you are re-introducing a new Chrome API call that won't work when the toolbox runs in content.

Is there any way we can replace this with standard JS? Or else, get the same information from the animation actor (which runs in a chrome process).

::: devtools/client/animationinspector/components/keyframes.js:348
(Diff revision 2)
> +        continue;
> +      }
> +      const value2 = keyframes[j].value;
> +      try {
> +        const distance =
> +          DOMWindowUtils.computeAnimationDistance(targetEl, propertyName, value1, value2);

Same comment about using chrome-only APIs (here and below). We'll unfortunately need another approach to doing this.
Is the WebAnimations API going to have a standard API for doing this?

::: devtools/client/animationinspector/components/keyframes.js:370
(Diff revision 2)
> +/**
> + * Convert given CSS property name to IDL name.
> + * @param {String} CSS property name (e.g. background-color).
> + * @return {String} IDL name (e.g. backgroundColor).
> + */
> +function propertyToIDL(property) {

This sounds familiar, I think we might have code that does this already in DevTools. Unfortunately I couldn't find it.
I did however found getCssPropertyName in \devtools\client\animationinspector\components\animation-details.js which does the opposite. I think it would make sense to put these 2 functions close to each other in a shared file.

::: devtools/client/animationinspector/components/keyframes.js:390
(Diff revision 2)
> + * @param {Number} minProgressThreshold - Minimum progress threshold.
> + * @param {Object} segmentHelper - The object of getSegmentHelper.
> + * @return {Array} path segments -
> + *                 [{x: {Number} time, y: {Number} progress}, ...]
> + */
> +function createPathSegments(startTime, endTime, minSegmentDuration,

This function is almost exactly the same as the one in \devtools\client\animationinspector\components\animation-time-block.js
Please reuse the code as much as possible.
In this case, you could move createPathSegments to a utils.js file and use it in both places.

::: devtools/client/animationinspector/components/keyframes.js:441
(Diff revision 2)
> + * @param {Element} parentEl - Parent element of this appended path element.
> + * @param {Array} pathSegments - Path segments. Please see createPathSegments.
> + * @param {String} cls - Class name.
> + * @return {Element} path element.
> + */
> +function appendPathElement(parentEl, pathSegments, cls) {

Same comment here, there's the same exact function in \devtools\client\animationinspector\components\animation-time-block.js
Please put code in common in a shared file.

::: devtools/client/themes/animationinspector.css:621
(Diff revision 2)
>  .keyframes .frame::before {
>    content: "";
>    display: block;
>    transform:
>      translateX(calc(var(--keyframes-marker-size) * -.5))
>      /* The extra pixel on the Y axis is so that markers are centered on the
>         horizontal line in the keyframes diagram. */
>      translateY(calc(var(--keyframes-marker-size) * -.5 + 1px));
>    width: var(--keyframes-marker-size);
>    height: var(--keyframes-marker-size);
>    border-radius: 100%;
> +  border: 1px solid lightGray;
>    background-color: inherit;
>  }

Design note: I feel like visually, the big circles that show where frames are don't work very well anymore when overlayed on top of the new property graphs.
I think we might need to revisit the design for them. This can be done in a later patch though. Let's keep it in mind though so we don't forget at the end.

::: devtools/client/themes/variables.css
(Diff revision 2)
>    --theme-highlight-gray: #dde1e4;
>  
>    /* Colors used in Graphs, like performance tools. Similar colors to Chrome's timeline. */
>    --theme-graphs-green: #85d175;
>    --theme-graphs-blue: #83b7f6;
> -  --theme-graphs-bluegrey: #0072ab;

Why remove this variable, it appears to still be used in some other parts of the code.
Attachment #8823597 - Flags: review?(pbrosset)
Comment on attachment 8823598 [details]
Bug 1210796 - Part 3: Isolated timeline.

https://reviewboard.mozilla.org/r/102138/#review104346

::: devtools/client/animationinspector/components/keyframes.js:113
(Diff revision 2)
>      this.keyframesEl.removeChild(targetEl);
>  
>      // Append elements to display keyframe values.
>      this.keyframesEl.classList.add(animation.state.type);
>      for (let frame of this.keyframes) {
> -      let offset = frame.offset + iterationStartOffset;
> +      const offset = frame.offset;

nit: no need to initialize the extra `offset` here.

::: devtools/client/animationinspector/components/keyframes.js:118
(Diff revision 2)
> -      let offset = frame.offset + iterationStartOffset;
> +      const offset = frame.offset;
>        createNode({
>          parent: this.keyframesEl,
>          attributes: {
>            "class": "frame",
>            "style": `left:${offset * 100}%;`,

just use `frame.offset` here

::: devtools/client/animationinspector/components/keyframes.js:119
(Diff revision 2)
>        createNode({
>          parent: this.keyframesEl,
>          attributes: {
>            "class": "frame",
>            "style": `left:${offset * 100}%;`,
> -          "data-offset": frame.offset,
> +          "data-offset": offset,

and here.
Attachment #8823598 - Flags: review?(pbrosset) → review+
Hi Patrick, thank you for your review!

I'd like to know about the way to use no-shipped method.
Currently, we use getComputedTiming method to make the summary graph shape( bug 1210795 ). Likewise, we can calculate the graph shape of this bug with spacing keyframes[1] instead of DOMWindowUtils.computeAnimationDistance .
Those are not need Chrome privilege, but they do not ship yet. That means the animationinspector on beta edition can't work since no those methods.
Are there any ways to use them in beta or release?

[1] https://w3c.github.io/web-animations/#spacing-keyframes
Flags: needinfo?(pbrosset)
(In reply to Daisuke Akatsuka (:daisuke) from comment #55)
> Hi Patrick, thank you for your review!
> 
> I'd like to know about the way to use no-shipped method.
> Currently, we use getComputedTiming method to make the summary graph shape(
> bug 1210795 ).
Right, this is the one we discussed when we met in Hawaii. I didn't remember exactly which method it was and then I forgot to file a bug for it.
Here is where we use it:
http://searchfox.org/mozilla-central/search?q=getComputedTiming&path=devtools

> Likewise, we can calculate the graph shape of this bug with
> spacing keyframes[1] instead of DOMWindowUtils.computeAnimationDistance .
> Those are not need Chrome privilege, but they do not ship yet. That means
> the animationinspector on beta edition can't work since no those methods.
> Are there any ways to use them in beta or release?
So the animationinspector will start failing as soon as the next merge happens. 
I don't know of any way to use getComputedTiming or spacing keyframes on beta or release if they don't ship by default. I'm guessing they are behind a pref that's set to true by default on nightly and aurora, and false on beta and release. DevTools can't force those prefs to true.

When are these things going to ship to all channels? Do we already have a timeline estimate for this?

The only thing I can think of is: we file a bug to take care of getComputedTiming, in which we land patch that does something like this: the code test for the existence of getComputedTiming, if it exists it uses it, otherwise it provides a fallback function that always returns 1 (or something) so instead of nice easing graphs, we have rectangles (so we have something that's roughly what we had before).

We would need to do this quickly so we can uplift this all the way.

Now, for this current bug, it depends whether we can ship it after spacing keyframes ships or not.
Flags: needinfo?(pbrosset)
Wait, I'm not sure I understand the problem after all. Looking at the history of the code, it seems like we have been using getComputedTiming for some time already. In fact it's used in release without any problems.
For example, see bug 1231688 landed in FF46.
Maybe only the getComputedTiming().progress property is not shipping yet, is that it?
Can you clarify before I file the bug?
Flags: needinfo?(daisuke)
Comment on attachment 8823599 [details]
Bug 1210796 - Part 4: Add animated property header.

https://reviewboard.mozilla.org/r/102140/#review104582

::: devtools/client/animationinspector/components/animation-details.js:160
(Diff revision 2)
>  
>      // Useful for tests to know when the keyframes have been retrieved.
>      this.emit("keyframes-retrieved");
>  
> +    // Add keyframe's progress line.
> +    renderKeyframesProgressLine(this.containerEl, animation);

I think you should move this function to inside the component's prototype. So you execute it with `this.renderKeyframesProgressLine(animation)`;
(no need to pass arguments the `this.containerEl` argument then).

Also, I think we should rename progressLine to something else.
For instance, the header at the top of the animation-inspector is called the time-header. So maybe we should also use the word header here too, for consistency. KeyframesHeader, or AnimatedPropertyHeader. Something like this.

::: devtools/client/animationinspector/components/animation-details.js:162
(Diff revision 2)
>      this.emit("keyframes-retrieved");
>  
> +    // Add keyframe's progress line.
> +    renderKeyframesProgressLine(this.containerEl, animation);
> +
>      for (let propertyName in this.tracks) {

Because you have one part of the render function in another function, I think it makes sense to move this other part of the render function in another function too. This way it's easier to follow what's happening:

```
this.renderKeyframesProgressLine(animation);
this.renderAnimatedProperties(animation);
```

::: devtools/client/animationinspector/components/animation-details.js:231
(Diff revision 2)
> +  // Add 0.0 label
> +  createNode({
> +    parent: progressLineHeaderLabelEl,
> +    nodeType: "label",
> +    attributes: { "class": "header-item" },
> +    textContent: "0.0"
> +  });
> +
> +  // Add 0.5 label
> +  createNode({
> +    parent: progressLineHeaderLabelEl,
> +    nodeType: "label",
> +    attributes: { "class": "header-item" },
> +    textContent: "0.5"
> +  });
> +
> +  // Add 1.0 label
> +  createNode({
> +    parent: progressLineHeaderLabelEl,
> +    nodeType: "label",
> +    attributes: { "class": "header-item" },
> +    textContent: "1.0"
> +  });

I want to make sure I understand, here we're not dealing with time, right? 0, 0.5, 1 only represents one iteration of the animation. Similar to how you can use 0% 50% 100% in CSS @keyframes? This header doesn't show actual time, right?

Also, what's the logic behind the labels 0.0 and 1.0? Why not simply 0 and 1? Or why not use percentages?

In any case, you could refactor this code to use a loop:

```
for (let label of [0, .5, 1]) {
  createNode({
    parent: progressLineHeaderLabelEl,
    nodeType: "label",
    attributes: { "class": "header-item" },
    textContent: label
  });
}
```
Attachment #8823599 - Flags: review?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #56)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #55)
> > Likewise, we can calculate the graph shape of this bug with
> > spacing keyframes[1] instead of DOMWindowUtils.computeAnimationDistance .
> > Those are not need Chrome privilege, but they do not ship yet. That means
> > the animationinspector on beta edition can't work since no those methods.
> > Are there any ways to use them in beta or release?
> So the animationinspector will start failing as soon as the next merge
> happens. 
> I don't know of any way to use getComputedTiming or spacing keyframes on
> beta or release if they don't ship by default. I'm guessing they are behind
> a pref that's set to true by default on nightly and aurora, and false on
> beta and release. DevTools can't force those prefs to true.

We also turn these things on for Chrome callers. That's why they currently work even on beta/release channels. But I believe we were going to drop chrome privileges for client-side code? And if so, when does that ship?

I believe we periodically do simulated merges to check that tests will pass after the merge so I'm surprised nothing has come up from that if we have already dropped chrome privileges for client-side DevTools code and that is set to ride the trains.

> When are these things going to ship to all channels? Do we already have a
> timeline estimate for this?

No, it's really gated on the standards process -- we'll probably fix a few more things in the spec, then coordinate with Google about a shipping date. I expect we'll ship in the first half of this year, however.

> The only thing I can think of is: we file a bug to take care of
> getComputedTiming, in which we land patch that does something like this: the
> code test for the existence of getComputedTiming, if it exists it uses it,
> otherwise it provides a fallback function that always returns 1 (or
> something) so instead of nice easing graphs, we have rectangles (so we have
> something that's roughly what we had before).

Is there no other way to treat client-side DevTools code differently?
Flags: needinfo?(daisuke) → needinfo?(pbrosset)
Thanks Brian. That clarify things. So getComputedTiming and spacing keyframes features work for chrome and content on nightly and aurora, and only for chrome on beta and release.
I guess that means we can use them in the inspector's UI now, because they will eventually ship by default for content in all releases.

Let me clarify the devtools.html project: originally, the goal was to remove all XUL from the devtools UI. That project ended up also encompassing removing all chrome-privileged code from our UI. See [1] for motivations.

During this project, we went through a very long period of refactoring of the inspector panel code to remove all the XUL code we could find and all the usages of APIs that wouldn't work in content.
At the end of the project, we managed to set up a local development workflow where one can open the inspector in a content tab in Firefox, make changes and just reload the page to see them (no build required). Bug 1291049 is where that work happened.

Now, that being said, we haven't *shipped* this in any sort of official way. I mean, all code changes have landed, but we do not run any tests in content to make sure we don't re-introduce XUL or chrome APIs.
It is our goal, in 2017, to put something like that in place (as well as doing other things like moving the code to GitHub eventually).

That means that, today, devtools just only runs in a chrome context. And that's why getComputedTiming just works.

During the devtools.html project, we also worked on the debugger panel. And we took a very different approach. That panel was rewritten from scratch, with the code on GitHub, with tests running in content, and with developers working with the debugger in content tabs of Firefox and other browsers daily.
So, for that panel, there's absolutely no way we could re-introduce chrome APIs by mistake. Even if eventually, the debugger runs inside the devtools toolbox in Firefox, where chrome APIs would work.

I hope this makes things clearer.
To summarize: we are already using getComputedTiming, it won't cause any problems on beta and release, until we start running tests in content privileged contexts. And since, eventually, this API will be available in content contexts too, I think we can safely continue to use it and not worry about it.

If you're confident that this will ship in the first half of the year, then that sounds great to me.
In Q1 (probably Q2), we said we'd focus on the console panel first. So the inspector can come next, which gives us the time it will take for these APIs to ship.

[1] https://docs.google.com/document/d/1_5aerWTN_GVofr6YQVjmJlaGfZ4nv5YKZmdGHewfTpE/edit#heading=h.dw3amfbdp0lh
Flags: needinfo?(pbrosset)
Comment on attachment 8823600 [details]
Bug 1210796 - Part 5: Add aniamted property's progress line tick.

https://reviewboard.mozilla.org/r/102142/#review104870

::: devtools/client/animationinspector/components/animation-details.js:255
(Diff revision 2)
> +  // Add progress line tick body background.
> +  const progressLineBodyBackgroundEl = createNode({
> +    parent: keyframesProgressLineEl,
> +    attributes: { "class": "track-container progress-body" }
> +  });
> +
> +  // Add progress tick of 0.0.
> +  createNode({
> +    parent: progressLineBodyBackgroundEl,
> +    nodeType: "span",
> +    attributes: { "class": "progress-tick" }
> +  });
> +
> +  // Add progress tick of 0.5.
> +  createNode({
> +    parent: progressLineBodyBackgroundEl,
> +    nodeType: "span",
> +    attributes: { "class": "progress-tick" }
> +  });
> +
> +  // Add progress tick of 1.0.
> +  createNode({
> +    parent: progressLineBodyBackgroundEl,
> +    nodeType: "span",
> +    attributes: { "class": "progress-tick" }
> +  });

I think you should fold this commit with the one just before, where you added the code dealing with the header.
They are dealing exactly with the same code and it would make reviews a bit easier.

I would make the same comment here: please generate these in the same loop as you generate the headers.
Attachment #8823600 - Flags: review?(pbrosset)
Comment on attachment 8823601 [details]
Bug 1210796 - Part 6: Fixed animation detail panel.

https://reviewboard.mozilla.org/r/102144/#review105128

Here are a few things I found while testing. Some of them may be due to this specific commit, others to previous commits.

- The zebra striping pattern in the list of animations doesn't work anymore, probably because the animation details was move to the accordion. Search for `.animation-timeline .animation:nth-child(4n+1)` in the CSS file, and replace `4n+1` with `2n+1`. I think that should fix it.
- Even when no animations are selected, the accordion header appears and the arrow can be clicked and it animates when clicked. This is a bit confusing because no accordion content actually appears in this case. In fact, I'm not sure we do need an expandable/collapsible accordion. If you click on an animation, it's most probably because you want more information about it, so it's safe to show the panel below with the keyframes. When no animations are selected, then not showing the panel at all seems like the right thing to do. So maybe we could just have a panel that appears and disappears when animations get selected/deselected instead of something collapsible?
- When an animation is selected, there are no visual clues indicating which one is selected. So if you have many animations you may loose track of which one you selected before easily (maybe part 8 solves this though, haven't tested yet). Selection should work like in the inspector for example I think. Also the accordion header could have its text change to repeat the name and/or DOM target of the animation too.
- Sometimes, when selecting another animation, there's a visible glitch where the accordion closes and then re-opens quickly to show the new animation. This is very fast, but still noticeable. We should investigate a way to avoid this and only have the content refresh but the accordion stay open. Maybe you are re-rendering the whole component? If so, I would advise bringing the accordion one level up in the hierarchy of components and have it be a child of the panel, just like the timeline is. I think your approach was to make the accordion a child of the timeline instead. Which means that any time its refreshed, then the accordion gets refreshed too. I think the accordion could be a top level thing instead.
- This is sort of random, but sometimes when I click on another animation, it doesn't get selected right away. I sometimes have to click 2 or 3 times to select it. Other times, just clicking once on a new animation does work immediately.
- If you have, say, 3 animations displayed, and you make the panel tall enough that there is space below these animations. Then 2 weird things happen (at least for me on Windows): the scrollbar in the timeline appears. It should not because there's no overflow, all animations are displayed fine. And the 2nd thing is if you try to scroll with the wheel and with the mouse pointer over the empty area, then that doesn't work. If instead the pointer is over one of the animations, then the mouse wheel does work.

::: devtools/client/animationinspector/components/animation-timeline.js:135
(Diff revision 2)
> +    this.propertiesAccordionEl = createNode({
> +      parent: this.rootWrapperEl,
> +      attributes: {
> +        "class": "accordion"
> +      }
> +    });

Hm, is there a way we can reuse the accordion component from the debugger? You're re-creating an accordion widget here that will have to be maintained and may differ in terms of look and feel with the one in the debugger. It'd be really great if we could avoid having to do this.
Now, I know the one in the debugger is React. But integrating React components in a non-React UI is possible, especially using the lifecycle methods like componentDidMount, etc.
So I would really advise looking in how to do this. Feel free to ping :jlast for help on this.

It would be really cool if we could avoid duplicating the HTML, JS and CSS for something we already have.

Note: if you want to take this opportunity to make the whole animation details panel React, then go for it!

::: devtools/client/animationinspector/components/animation-timeline.js:242
(Diff revision 2)
> -    this.destroySubComponents("details", [{
> -      event: "frame-selected",
> +    this.details.off("frame-selected", this.onFrameSelected);
> +    this.detailsEl.innerHTML = "";

I think you should instead call `this.details.unrender();`
that's what this function is for.

::: devtools/client/animationinspector/components/animation-timeline.js:278
(Diff revision 2)
> +        this.detailsEl.classList.remove("cssanimation");
> +        this.detailsEl.classList.remove("csstransition");
> +        this.detailsEl.classList.remove("scriptanimation");

The list of classes comes from the different values that `animation.state.type` can have, so it might be better not to hard code these values here.

Instead, you could maybe replace the whole if block with:

```
this.detailsEl.className = `animated-properties ${animation.state.type}`;
```
Attachment #8823601 - Flags: review?(pbrosset)
Comment on attachment 8823602 [details]
Bug 1210796 - Part 7: Change selection logic.

https://reviewboard.mozilla.org/r/102146/#review105212

I noticed that one of the problem I reported in the previous commit is gone with this commit: when clicking on animations, now the accordion updates correctly every time. Thanks!

::: devtools/client/animationinspector/components/animation-timeline.js:268
(Diff revision 2)
>      let index = this.animations.indexOf(animation);
>      if (index === -1) {
>        return;
>      }
>  
> -    let el = this.rootWrapperEl;
> +    // Unselect an animation which has selected.

nit: rephrase to  Unselect the animation which was selected.
Attachment #8823602 - Flags: review?(pbrosset) → review+
Hi Patrick,
Thank you for your review!
I'm sorry for my delay, had fixed other bugs.

I'll fix this bug from this week.
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review104320

> This function is almost exactly the same as the one in \devtools\client\animationinspector\components\animation-time-block.js
> Please reuse the code as much as possible.
> In this case, you could move createPathSegments to a utils.js file and use it in both places.

Yeah, I thought so.
I had fixed in patch 10 ( https://reviewboard.mozilla.org/r/102152/diff/2#index_header )
But, should we unite createPathSegments, appendPathElement functions in this bug?

> Why remove this variable, it appears to still be used in some other parts of the code.

I'm sorry, this is my mistake..
(In reply to Daisuke Akatsuka (:daisuke) from comment #65)
> Comment on attachment 8823597 [details]
> Bug 1210796 - Part 2: Visualize each properties.
> 
> https://reviewboard.mozilla.org/r/102136/#review104320
> 
> > This function is almost exactly the same as the one in \devtools\client\animationinspector\components\animation-time-block.js
> > Please reuse the code as much as possible.
> > In this case, you could move createPathSegments to a utils.js file and use it in both places.
> 
> Yeah, I thought so.
> I had fixed in patch 10 (
> https://reviewboard.mozilla.org/r/102152/diff/2#index_header )
> But, should we unite createPathSegments, appendPathElement functions in this
> bug?
> 

No..
Should we unite in this patch( part 2 )?
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review104320

> What does IDL mean in this context? Could we maybe use something a little less cryptic?

I'm sorry I could not understand this question..
However the reason that I added the parameter for IDL is just wanted to avoid to re-create new string.

> This sounds familiar, I think we might have code that does this already in DevTools. Unfortunately I couldn't find it.
> I did however found getCssPropertyName in \devtools\client\animationinspector\components\animation-details.js which does the opposite. I think it would make sense to put these 2 functions close to each other in a shared file.

I'll move this function too in patch 10.

> Design note: I feel like visually, the big circles that show where frames are don't work very well anymore when overlayed on top of the new property graphs.
> I think we might need to revisit the design for them. This can be done in a later patch though. Let's keep it in mind though so we don't forget at the end.

Thanks!
I filed the Bug 1334001
(In reply to Daisuke Akatsuka (:daisuke) from comment #66)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #65)
> > Comment on attachment 8823597 [details]
> > Bug 1210796 - Part 2: Visualize each properties.
> > 
> > https://reviewboard.mozilla.org/r/102136/#review104320
> > 
> > > This function is almost exactly the same as the one in \devtools\client\animationinspector\components\animation-time-block.js
> > > Please reuse the code as much as possible.
> > > In this case, you could move createPathSegments to a utils.js file and use it in both places.
> > 
> > Yeah, I thought so.
> > I had fixed in patch 10 (
> > https://reviewboard.mozilla.org/r/102152/diff/2#index_header )
> > But, should we unite createPathSegments, appendPathElement functions in this
> > bug?
> > 
> 
> No..
> Should we unite in this patch( part 2 )?
Yes I think that would make reviewing easier. When I review a part in a patch series, I don't usually look ahead at other parts that might refactor/clean the code that was changed.
I mean, I'm fine with that, as long as I know.
(In reply to Patrick Brosset <:pbro> from comment #68)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #66)
> > (In reply to Daisuke Akatsuka (:daisuke) from comment #65)
> > > Comment on attachment 8823597 [details]
> > > Bug 1210796 - Part 2: Visualize each properties.
> > > 
> > > https://reviewboard.mozilla.org/r/102136/#review104320
> > > 
> > > > This function is almost exactly the same as the one in \devtools\client\animationinspector\components\animation-time-block.js
> > > > Please reuse the code as much as possible.
> > > > In this case, you could move createPathSegments to a utils.js file and use it in both places.
> > > 
> > > Yeah, I thought so.
> > > I had fixed in patch 10 (
> > > https://reviewboard.mozilla.org/r/102152/diff/2#index_header )
> > > But, should we unite createPathSegments, appendPathElement functions in this
> > > bug?
> > > 
> > 
> > No..
> > Should we unite in this patch( part 2 )?
> Yes I think that would make reviewing easier. When I review a part in a
> patch series, I don't usually look ahead at other parts that might
> refactor/clean the code that was changed.
> I mean, I'm fine with that, as long as I know.

Okay thanks!
I'll move getCssPropertyName and propertyToIDL as well.
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review104320

> I'm sorry I could not understand this question..
> However the reason that I added the parameter for IDL is just wanted to avoid to re-create new string.

I'm fine with adding a parameter. I just thought that the word `IDL` might not be the easiest one for people maintaining this code to understand.
Maybe these 2 parameters should be named `propertyCSSName` and `propertyJSName` instead? I think that would be easier to understand.
Comment on attachment 8823599 [details]
Bug 1210796 - Part 4: Add animated property header.

https://reviewboard.mozilla.org/r/102140/#review104582

> I think you should move this function to inside the component's prototype. So you execute it with `this.renderKeyframesProgressLine(animation)`;
> (no need to pass arguments the `this.containerEl` argument then).
> 
> Also, I think we should rename progressLine to something else.
> For instance, the header at the top of the animation-inspector is called the time-header. So maybe we should also use the word header here too, for consistency. KeyframesHeader, or AnimatedPropertyHeader. Something like this.

Okay, thanks!

> Because you have one part of the render function in another function, I think it makes sense to move this other part of the render function in another function too. This way it's easier to follow what's happening:
> 
> ```
> this.renderKeyframesProgressLine(animation);
> this.renderAnimatedProperties(animation);
> ```

Okay, I'll do that.

> I want to make sure I understand, here we're not dealing with time, right? 0, 0.5, 1 only represents one iteration of the animation. Similar to how you can use 0% 50% 100% in CSS @keyframes? This header doesn't show actual time, right?
> 
> Also, what's the logic behind the labels 0.0 and 1.0? Why not simply 0 and 1? Or why not use percentages?
> 
> In any case, you could refactor this code to use a loop:
> 
> ```
> for (let label of [0, .5, 1]) {
>   createNode({
>     parent: progressLineHeaderLabelEl,
>     nodeType: "label",
>     attributes: { "class": "header-item" },
>     textContent: label
>   });
> }
> ```

Thanks the advice!
Yeah, this is not the time. I'll use percentages since we can specify percentages in @keyframes.
Comment on attachment 8823601 [details]
Bug 1210796 - Part 6: Fixed animation detail panel.

https://reviewboard.mozilla.org/r/102144/#review105128

Thanks!
I'll check these behaviors.

> Hm, is there a way we can reuse the accordion component from the debugger? You're re-creating an accordion widget here that will have to be maintained and may differ in terms of look and feel with the one in the debugger. It'd be really great if we could avoid having to do this.
> Now, I know the one in the debugger is React. But integrating React components in a non-React UI is possible, especially using the lifecycle methods like componentDidMount, etc.
> So I would really advise looking in how to do this. Feel free to ping :jlast for help on this.
> 
> It would be really cool if we could avoid duplicating the HTML, JS and CSS for something we already have.
> 
> Note: if you want to take this opportunity to make the whole animation details panel React, then go for it!

Thanks again!
Will ask to :jlast on IRC.
(I'd like to move to React after this bug.)
(In reply to Patrick Brosset <:pbro> from comment #62)
> Comment on attachment 8823601 [details]
> Bug 1210796 - Part 6: Fixed animation detail panel.
>
> https://reviewboard.mozilla.org/r/102144/#review105128
>
> Here are a few things I found while testing. Some of them may be due to this
> specific commit, others to previous commits.

Thank you very much!
I replay inline.

> - The zebra striping pattern in the list of animations doesn't work anymore,
> probably because the animation details was move to the accordion. Search for
> `.animation-timeline .animation:nth-child(4n+1)` in the CSS file, and
> replace `4n+1` with `2n+1`. I think that should fix it.

I fixed this.
Will add this test in patch 11.

> - Even when no animations are selected, the accordion header appears and the
> arrow can be clicked and it animates when clicked. This is a bit confusing
> because no accordion content actually appears in this case. In fact, I'm not
> sure we do need an expandable/collapsible accordion. If you click on an
> animation, it's most probably because you want more information about it, so
> it's safe to show the panel below with the keyframes. When no animations are
> selected, then not showing the panel at all seems like the right thing to
> do. So maybe we could just have a panel that appears and disappears when
> animations get selected/deselected instead of something collapsible?

Made the ui to hide at first.
Then, display when user click on animation.

> - When an animation is selected, there are no visual clues indicating which
> one is selected. So if you have many animations you may loose track of which
> one you selected before easily (maybe part 8 solves this though, haven't
> tested yet). Selection should work like in the inspector for example I
> think. Also the accordion header could have its text change to repeat the
> name and/or DOM target of the animation too.

Yes, added the background-color which is sign for selected on .animations in part 8.
Also, added AnimationTargetNode to the detail to show the selected dom name.

> - Sometimes, when selecting another animation, there's a visible glitch
> where the accordion closes and then re-opens quickly to show the new
> animation. This is very fast, but still noticeable. We should investigate a
> way to avoid this and only have the content refresh but the accordion stay
> open. Maybe you are re-rendering the whole component? If so, I would advise
> bringing the accordion one level up in the hierarchy of components and have
> it be a child of the panel, just like the timeline is. I think your approach
> was to make the accordion a child of the timeline instead. Which means that
> any time its refreshed, then the accordion gets refreshed too. I think the
> accordion could be a top level thing instead.

It may be caused by refreshing the content of the accordion.
(The content is empty at once (0px), then display. It may be cause.)
I am trying to implement resizable panel instead of the accordion.
Please confirm after did.

> - This is sort of random, but sometimes when I click on another animation,
> it doesn't get selected right away. I sometimes have to click 2 or 3 times
> to select it. Other times, just clicking once on a new animation does work
> immediately.

Yes, this was fixed in part 7.

> - If you have, say, 3 animations displayed, and you make the panel tall
> enough that there is space below these animations. Then 2 weird things
> happen (at least for me on Windows): the scrollbar in the timeline appears.
> It should not because there's no overflow, all animations are displayed
> fine. And the 2nd thing is if you try to scroll with the wheel and with the
> mouse pointer over the empty area, then that doesn't work. If instead the
> pointer is over one of the animations, then the mouse wheel does work.

Yeah, I fixed this one.

Also, I did refactoring my codes a bit especially the structure of dom and the naming.
Thank you very much!
Hi Patrick,
I’m sorry for delay. It was taken time a bit..

Let me describe the changes:
* Use resizable panel by mouse instead of accordion.
* Content of animation detail is scrollable since change to resizable.
* Display animation detail from first time if there is only one animation at the list.
* Disable clicking on keyframes since we handle the progress now.
* Also, dom structure and naming.

I paste the structure as reference:
#player
- .animation-root
  - .animation-timeline : scrollable
    - Display animations
  - .animation-detail
    - .animation-detail-header 
      - Display "Animated properties” text and hidden resizer
    - .animation-detail-body
      - .animated-properties
        - .animated-properties-header
          - Display labels and ticks
        - .animated-properties-body : scrollable
          - .property *
        - .progress-indicator-wrapper
Comment on attachment 8823605 [details]
Bug 1210796 - Part 12: Unite common codes into utils.js and use.

https://reviewboard.mozilla.org/r/102152/#review112422

Nice clean up. Thank you.
Attachment #8823605 - Flags: review?(pbrosset) → review+
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review112426

Just a few more comments on this part.
I think the only part that I'm not very comfortable with is where we have all the helpers and the way they share the test DOM element. I've added a comment about this.
Let me know what you think. The rest is fine to me.

::: devtools/client/animationinspector/components/keyframes.js:145
(Diff revision 3)
>      });
>    }
>  };
> +
> +/**
> + * Render visuali graph for animation progress in one iteration.

Some rephrasing needed: "Render a graph representing the progress of the animation over one iteration"

::: devtools/client/animationinspector/components/keyframes.js:250
(Diff revision 3)
> + */
> +function getValueHelperFunction(animationType, targetEl, propertyCSSName,
> +                                propertyJSName, keyframes, DOMWindowUtils) {
> +  switch (animationType) {
> +    case "none": {
> +      return noneValueHelperFunction();

No need to define a new function here. Let's just return this:

`return () => 1;`

::: devtools/client/animationinspector/components/keyframes.js:267
(Diff revision 3)
> +}
> +
> +/**
> + * Return value helper function of animation type 'none' or 'color'.
> + */
> +function noneValueHelperFunction() {

as commented above, no need for this function

::: devtools/client/animationinspector/components/keyframes.js:276
(Diff revision 3)
> +}
> +
> +/**
> + * Return value helper function of animation type 'float'.
> + * @param {Object} keyframes - This object shoud be same as
> + *                             the parameter of getGraphHelper.

Missing @return jsdoc

::: devtools/client/animationinspector/components/keyframes.js:299
(Diff revision 3)
> + *                             the parameter of getGraphHelper.
> + * @param {String} propertyCSSName - CSS property name (e.g. background-color).
> + * @param {String} propertyJSName - CSS property name for JS (e.g. backgroundColor).
> + * @param {Object} keyframes - This object shoud be same as
> + *                             the parameter of getGraphHelper.
> + * @param {DOMWindowUtils} DOMWindowUtils - nsDOMWindowUtils object.

Missing @return jsdoc

::: devtools/client/animationinspector/components/keyframes.js:327
(Diff revision 3)
> + * @param {Element} targetEl - This element shoud be same as
> + *                             the parameter of getGraphHelper.
> + * @param {String} propertyCSSName - CSS property name (e.g. background-color).
> + * @param {String} propertyJSName - CSS property name for JS (e.g. backgroundColor).
> + * @param {Object} keyframes - This object shoud be same as
> + *                             the parameter of getGraphHelper.

missing @return jsdoc

::: devtools/client/animationinspector/components/keyframes.js:340
(Diff revision 3)
> +  // 2. Set the two styles into new keyframes as ‘from' and ‘to’.
> +  // 3. Set the style which calculates into between those.
> +  // 4. Return the computedOffset which is gotten by getKeyframes() as y-axis value
> +
> +  const win = targetEl.ownerDocument.defaultView;
> +  const dummyEffect = new win.KeyframeEffect(targetEl, null,

Does the `targetEl` really need to be the exact same element as the one used in other helper functions?

It feels a bit weird having to pass it around all of these functions.

Looking at the code in this particular function, it feels to me like we could use any DOM element here.
In fact, I tested this quickly, and this seems to work for me. But maybe we need the same element because it has the right timing informtion like the easing.

If we need the same targetEl to be used in all of these helper functions (coordinateValueHelperFunction, distanceValueHelperFunction, getValueHelperFunction, getGraphHelper), then I'd like to suggest we rework the code a bit.

I think a class would work better. Maybe a class called GraphHelpers or something.
You would instantiate once only: 

const graphHelpers = new GraphHelpers(win);

using the given win, it would create the targetEl you need.
And then it would expose the 4 functions. This way, they can share the targetEl (via this.targetEl) rather than having to ask for it everytime.

::: devtools/client/themes/animationinspector.css:59
(Diff revision 3)
>    /* The size of a keyframe marker in the keyframes diagram */
>    --keyframes-marker-size: 10px;
>    /* The color of the time graduation borders */
>    --time-graduation-border-color: rgba(128, 136, 144, .5);
> +  /* The color for other animation type */
> +  --other-border-color: #999999;

Maybe use the shorter form `#999`

::: devtools/client/themes/animationinspector.css:60
(Diff revision 3)
>    --keyframes-marker-size: 10px;
>    /* The color of the time graduation borders */
>    --time-graduation-border-color: rgba(128, 136, 144, .5);
> +  /* The color for other animation type */
> +  --other-border-color: #999999;
> +  --other-background-color: #99999966;

And here too `#9996`

::: devtools/client/themes/animationinspector.css:632
(Diff revision 3)
>         horizontal line in the keyframes diagram. */
>      translateY(calc(var(--keyframes-marker-size) * -.5 + 1px));
>    width: var(--keyframes-marker-size);
>    height: var(--keyframes-marker-size);
>    border-radius: 100%;
> +  border: 1px solid lightGray;

You should try to reuse a color from our theme instead of introducing a hard coded one here.
See the list of colors in /devtools/client/themes/variables.css

::: devtools/client/themes/variables.css
(Diff revision 3)
>    --theme-highlight-gray: #dde1e4;
>  
>    /* Colors used in Graphs, like performance tools. Similar colors to Chrome's timeline. */
>    --theme-graphs-green: #85d175;
>    --theme-graphs-blue: #83b7f6;
> -  --theme-graphs-bluegrey: #0072ab;

Why remove this? It appears to still be used.

::: devtools/server/actors/animation.js:471
(Diff revision 3)
>        return {name: property.property, values: property.values};
>      });
> +  },
> +
> +  /**
> +   * Get animation type of given CSS property name.

Rephrase to "Get the animation types for a given list of CSS property names"

::: devtools/shared/specs/animation.js:81
(Diff revision 3)
>          properties: RetVal("array:json")
>        }
> +    },
> +    getAnimationTypes: {
> +      request: {
> +        propertyNames: Arg(0, "array:json")

This should be "array:string" instead.
Attachment #8823597 - Flags: review?(pbrosset)
Comment on attachment 8823599 [details]
Bug 1210796 - Part 4: Add animated property header.

https://reviewboard.mozilla.org/r/102140/#review112458

r=me but please use localized strings for the percentages as explained below.

::: devtools/client/animationinspector/components/animation-details.js:189
(Diff revision 3)
> +      parent: headerEl,
> +      attributes: { "class": "track-container" }
> +    });
> +
> +    // Add labels
> +    for (let label of ["0%", "50%", "100%"]) {

I talked to flod on IRC and he told me these need to be localized. So these should be strings in a properties file instead.

There are 2 examples of this in devtools already:

https://hg.mozilla.org/releases/mozilla-aurora/file/default/devtools/client/locales/en-US/performance.properties#l89
https://hg.mozilla.org/releases/mozilla-aurora/file/default/devtools/client/locales/en-US/memory.properties#l202

So we should do the same here. Either putting these strings in a common properties file so we can reuse, or adding new ones in the animation inspector's properties file.
Attachment #8823599 - Flags: review?(pbrosset) → review+
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review112426

Thanks Patrick,
I will make GraphHelpers class according to your advice!

> No need to define a new function here. Let's just return this:
> 
> `return () => 1;`

Definitely!
Comment on attachment 8833906 [details]
Bug 1210796 - Part 11: Disable clicking on keyframes because we can’t calculate correct current time since the animation detail handles the animation progress not time.

https://reviewboard.mozilla.org/r/110036/#review112808

Code changes look good to me. But I'm a bit sad to see this feature go away. I think it was useful to see the state of the animated element at specific keyframes. But I'm not a real user and never got specific feedback about this feature. I suspect that not a lot of people knew you could even click on animations to see their keyframes, so we might not be losing a very used feature anyway.
Do you have plans to implement this feature elsewhere/differently?
Attachment #8833906 - Flags: review?(pbrosset) → review+
Comment on attachment 8823606 [details]
Bug 1210796 - Part 13: Add tests.

https://reviewboard.mozilla.org/r/102154/#review112816

A few typos and maybe some test spliting would be nice, but other than this this looks good, thanks for adding tests!

::: devtools/client/animationinspector/test/browser_animation_animated_properties_path.js:204
(Diff revision 3)
> +add_task(function* () {
> +  yield addTab(URL_ROOT + "doc_multiple_property_types.html");
> +  const {panel} = yield openAnimationInspector();
> +  const timelineComponent = panel.animationsTimelineComponent;
> +
> +  for (let index in TEST_CASES) {

nit: iterating over an array with for..in feels a bit weird to me. I think a simple `for (let i = 0; i < TEST_CASES.length; i++)`, even if not as compact, would feel more natural.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_path.js:214
(Diff revision 3)
> +    const properties = TEST_CASES[index];
> +    for (let property in properties) {
> +      const testcase = properties[property];
> +      info(`Test path of ${ property }`);
> +      const className = testcase.expectedClass;
> +      const pathEl = timelineComponent.rootWrapperEl.querySelector(`path.${ className }`);

Can't there be cases where there are several paths with the same classe in the timeline?
Maybe in your test case this doesn't happen, but what if both `color` and `background-color` were animated at the same time? Then you'd have 2 `path.color` and the test might fail.
It's probable that someone changes the test case in the future, so it might be safer to do `querySelectorAll` and define an index in the TEST_CASES, or something like that.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_path.js:215
(Diff revision 3)
> +    for (let property in properties) {
> +      const testcase = properties[property];
> +      info(`Test path of ${ property }`);
> +      const className = testcase.expectedClass;
> +      const pathEl = timelineComponent.rootWrapperEl.querySelector(`path.${ className }`);
> +      ok(pathEl, `Path element which has '${ className }' class should be exist`);

Please replace "should be exist" with "should exist" (here and in other places below).

::: devtools/client/animationinspector/test/browser_animation_animated_properties_progress_indicator.js:27
(Diff revision 3)
> +  is(progressIndicatorEl.style.left, "0%",
> +     "The left style of progress indicator element should be 0% at 0ms");
> +  yield clickOnTimelineHeader(panel, 0.5);
> +  apploximate(progressIndicatorEl.style.left, "50%",
> +              "The left style of progress indicator element should be "
> +              + "apploximately 50% at 500ms");

I think you mean "approximately"

::: devtools/client/animationinspector/test/browser_animation_animated_properties_progress_indicator.js:82
(Diff revision 3)
> +  yield clickOnAnimation(panel, 1);
> +  is(progressIndicatorEl.style.left, originalIndicatorPosition,
> +     "The animation time should be continued even if another animation selects");
> +});
> +
> +function apploximate(percentageString1, percentageString2, message) {

s/apploximate/approximate

::: devtools/client/animationinspector/test/browser_animation_detail_displayed.js:12
(Diff revision 3)
> +// 4. Display from first time if displayed animation is only one.
> +// 5. Resize the displaying area if user dragged on top edge of this pane.

We have many problems with mochitests taking too long to run and causing intermittent timeouts, especially on slower test environments like linux/debug.

So, I usually try to create small/fast tests when I can.
This one seems like a good candidate for this.

I would do one test for (1), (2) and (3), another one for (4), and yet another one for (5).

It also makes maintenance easier.

::: devtools/client/animationinspector/test/browser_animation_detail_displayed.js:22
(Diff revision 3)
> +  const {inspector, panel} = yield openAnimationInspector();
> +  const animationDetailEl =
> +    panel.animationsTimelineComponent.rootWrapperEl.querySelector(".animation-detail");
> +
> +  // 1. Existance of animation-detail element.
> +  ok(animationDetailEl, "The animation-detail element should be exist");

Same comment as earlier "Should exist" instead of "Should be exist"

::: devtools/client/animationinspector/test/head.js:378
(Diff revision 3)
>  /**
> + * Click the timeline header to update the animation current time.
> + * @param {AnimationsPanel} panel
> + * @param {Number} x position rate on timeline header.
> + *                 This method calculates
> + *                 `xPositionRate * offsetWidth + offsetWidth of timeline header`

I think you meant `xPositionRate * offsetWidth + offsetLeft`, not `offsetWidth`.

::: devtools/client/animationinspector/test/head.js:382
(Diff revision 3)
> + *                 This method calculates
> + *                 `xPositionRate * offsetWidth + offsetWidth of timeline header`
> + *                 as the clientX of MouseEvent.
> + *                 This parameter should be from 0.0 to 1.0.
> + */
> +function* clickOnTimelineHeader(panel, xPositionRate) {

Maybe just `position` instead of `xPositionRate` would be a better name. Especially because you have the nice explaination in the jsdoc above.
Attachment #8823606 - Flags: review?(pbrosset)
Comment on attachment 8833906 [details]
Bug 1210796 - Part 11: Disable clicking on keyframes because we can’t calculate correct current time since the animation detail handles the animation progress not time.

https://reviewboard.mozilla.org/r/110036/#review112808

Yeah, I want to add a new feature 'edit the keyframe values' instead of 'indicate the time' in case of clicking.
How do you think about this?

Also, it is difficult to convert the progress to the time (time to progress is easy,,) in case of the effect easing is 'steps' or such the 'cubic-bezier(0,1.64,1,-0.69)'.
http://cubic-bezier.com/#0,1.64,1,-0.69
The last bezier goes through the progress of 0.5 three times. So, we can't specific the time,,
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review112426

Also, I'd like to change a bit.
* Make new js file graph-helper.js. This file exports all functions and variables that related to make graph such the GraphHelpers( maybe PropertyGraphHelpers or just getPropertyGraphHeler ), createPathSegments, DEFAULT_MIN_PROGRESS_THRESHOLD and so on.
Comment on attachment 8823606 [details]
Bug 1210796 - Part 13: Add tests.

https://reviewboard.mozilla.org/r/102154/#review112816

> Can't there be cases where there are several paths with the same classe in the timeline?
> Maybe in your test case this doesn't happen, but what if both `color` and `background-color` were animated at the same time? Then you'd have 2 `path.color` and the test might fail.
> It's probable that someone changes the test case in the future, so it might be safer to do `querySelectorAll` and define an index in the TEST_CASES, or something like that.

The animation detail (property graphs) displays for only one animation in this revised.
So if we get from the detail pane not timeline, it shoud be unique.
I'll change the test as above.
Thanks!

> We have many problems with mochitests taking too long to run and causing intermittent timeouts, especially on slower test environments like linux/debug.
> 
> So, I usually try to create small/fast tests when I can.
> This one seems like a good candidate for this.
> 
> I would do one test for (1), (2) and (3), another one for (4), and yet another one for (5).
> 
> It also makes maintenance easier.

Oh, I see.
I'll separate the test!
And I talked with Brian.
We want to add one more patch to add a button which closes the animation detail pane.
Hi Patrick,

The spacing keyframe feature will drop soon.
https://bugzilla.mozilla.org/show_bug.cgi?id=1339690
I'll re-write ProgressGraphHelper::getDistanceValueHelperFunction of graph-helper.js ( patch 2 ) as soon as possible.

Thanks.
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review114094

I see all my comments have been addressed, thanks!
I have 2 remaining comments: something about backward compatibility, and something about using `class`.
Please address them and then feel free to R+ the new patch yourself. Unless you feel like you've made some substantial changes that you want me to review again.

::: devtools/client/animationinspector/components/animation-details.js:156
(Diff revision 4)
> -    // Useful for tests to know when the keyframes have been retrieved.
> -    this.emit("keyframes-retrieved");
> +    // Get animation type for each CSS properties.
> +    const animationTypes =
> +      yield this.animation.getAnimationTypes(Object.keys(this.tracks));
>  

I completely forgot to mention this in a previous review, but now that this has moved to the server-side, it means that we need to handle backward compatibility for it. Indeed, this method does not exist on older debugging targets, so if you connect the toolbox to an older firefox, this code will fail.

So, we need some fallback mechanism.
The first thing you need to do is add a trait to know if this method exist. See `getServerTraits` in animation-controller.js
You'll need to add a new entry in the `config` array like:

```
    { name: "hasGetAnimationTypes", actor: "animationplayer",
      method: "getAnimationTypes" },
```

And then you can wrap this call in `if (this.serverTraits.hasGetAnimationTypes) {...}`

Now, if the target does *not* have the method, we need a fallback.
Either we need to completely disable the new feature, so that clicking on the animation doesn't do anything. Or we need to provide some degraded version of it. Like maybe defaulting all animation types to `none`.

::: devtools/client/animationinspector/components/keyframes.js:83
(Diff revision 4)
> +    // Create keyframe object to make dummy animation.
> +    const propertyJSName = getJsPropertyName(propertyName);
> +    const keyframesObject = keyframes.map(keyframe => {
> +      const keyframeObject = Object.assign({}, keyframe);
> +      keyframeObject[propertyJSName] = keyframe.value;
> +      return keyframeObject;
> +    });
> +
> +    // Create effect timing object to make dummy animation.
> +    const effectTiming = {
> +      duration: totalDuration,
> +      fill: "forwards"
> +    };

Now that you have created this new ProgressGraphHelper class, it seems to me that this entire block of code should go into the class instead of being here.

::: devtools/client/animationinspector/graph-helper.js:31
(Diff revision 4)
> +
> +/**
> + * This helper return the segment coordinates and style for property graph,
> + * also return the graph type.
> + */
> +class ProgressGraphHelper {

I like ES classes myself, unfortunately we haven't yet started to use them in the rest of the DevTools code (apart from a couple of tests). So I prefer if we refrain from introducing classes before we've had a chance to discuss them as a team and taken a decision about them.
This way we have a chance of having a more consistent code.

So, please replace with:

```
function ProgressGraphHelper() {
  ... the constructor code here
}

ProgressGraphHelper.prototype = {
  ... all other methods here
};
```
Attachment #8823597 - Flags: review?(pbrosset) → review+
I have done some testing with all commits applied, here are some things I found that we may want to fix:

- I don't know if repeating the target DOM node in the properties panel at the bottom is required. Why don't we take the full width of the panel instead?

+----+----+----+-----------+-----------------------------------+
| <| | |> | 1x | 00:00.550 |                                   |
+----+----+----+-----------+---+-------------------------------+
|                              | 0ms    100ms   200ms   300ms  |
+------------------------------+-------------------------------+
| div.class                    | [__animation name_______]     |
| span#id.class1.class2        |       [__animation name____]  |
+------------------------------+-------------------------------+
| Animated properties for <animation name>                     |
+--------------------------------------------------------------+
|          0%                   50%                       100% |
| opacity  O-------------------------------O----------------O  |
+--------------------------------------------------------------+

This gives us more space to look at things in details. And make it clearer that the 2 timelines aren't synchronized.
Also, if we do this, it would be nice to repeat the animation name in the bottom panel's header.

- The cross icon to close the properties panel is always black, even in the dark theme, which makes it hard to see.

- The properties panel sometimes stays open even when it is empty. To see this, do the following: click on an animation, you then see the properties listed below, now click on the rewind button. The properties panel becomes empty because the whole thing is refreshed, but it stays open.
I think we should do either of these 2 things:
  - either close the panel
  - or preserve the state of which animation is currently opened and automatically select it again on refresh
We have no state right now in the animation panel, so when you rewind, the whole thing gets refreshed and we lose the state of what was previously selected. It would be great to change this. Store somewhere on the animation-panel instance the name/id of the animation currently selected, and whether or not the properties panel is opened. So that whenever there's a refresh, if the selected animation still exists, then we could automatically select it, and toggle the properties panel accordingly.
This way, rewinding would be transparent to users, they wouldn't lose the context of whatever they were doing before.

- The round keyframes in the properties panel appear clickable. They have a cursor:pointer applied to them, so it feels like something will happen if you click them. But nothing does. So we should remove this style.

- The graph in the properties panel sometimes does not span the whole 100% width of the container.
On windows for instance, with the light theme, there's a scrollbar that appears to the right of the panel (probably because of the overflow:scroll css rule). And this scrollbar takes a bit of space, which messes up with the width calculations. So the graph and the progress header are not in sync.

- The properties progress indicator (the 1px vertical line) is really hard to see (at least to me). It is grey and by default is at the 100% position, so it overlaps the 100% vertical line. So at first, I didn't see it at all.
Then I started to move the scrubber and noticed that it was moving at the same time in the properties panel.
So I think we need to work on a better design for this indicator.
I would suggest using the same style as the scrubber (solid line, small triangle at the top), but use a different color.
Comment on attachment 8823600 [details]
Bug 1210796 - Part 5: Add aniamted property's progress line tick.

https://reviewboard.mozilla.org/r/102142/#review114144
Attachment #8823600 - Flags: review?(pbrosset) → review+
Comment on attachment 8823601 [details]
Bug 1210796 - Part 6: Fixed animation detail panel.

https://reviewboard.mozilla.org/r/102144/#review114148

Please see comment 114, I have commented about various UI issues I found, and I think some of them need to be fixed here.
Please fine further comments below, relative to this patch in particular.

::: devtools/client/animationinspector/components/animation-timeline.js:563
(Diff revision 4)
> +  onAnimationDetailResizerMouseDown: function (e) {
> +    this.resizeAnimationDetail(e.pageY);
> +    this.win.addEventListener("mouseup", this.onAnimationDetailResizerMouseUp);
> +    this.win.addEventListener("mouseout", this.onAnimationDetailResizerMouseOut);
> +    this.win.addEventListener("mousemove", this.onAnimationDetailResizerMouseMove);
> +    e.preventDefault();
> +  },
> +
> +  onAnimationDetailResizerMouseUp: function (e) {
> +    this.cancelAnimationDetailResizerDragging();
> +  },
> +
> +  onAnimationDetailResizerMouseOut: function (e) {
> +    if (!this.win.document.contains(e.relatedTarget)) {
> +      this.cancelAnimationDetailResizerDragging();
> +    }
> +  },
> +
> +  onAnimationDetailResizerMouseMove: function (e) {
> +    this.resizeAnimationDetail(e.pageY);
> +  },
> +
> +  cancelAnimationDetailResizerDragging: function () {
> +    this.win.removeEventListener("mouseup", this.onAnimationDetailResizerMouseUp);
> +    this.win.removeEventListener("mouseout", this.onAnimationDetailResizerMouseOut);
> +    this.win.removeEventListener("mousemove", this.onAnimationDetailResizerMouseMove);
> +  },

We have very similar code in devtools\client\shared\components\splitter\split-box.js. This is a shared React component that we use for things that need to be resizable either vertically or horizontally.

It would be great to use this instead of adding more code to handle the resizing yourself here.
This is better for maintenance because then you don't have to fix potential bugs in 2 places.

Now, the Split-box thing is a React component, and I know the animation-inspector does not use React, so it may be a bit hard to use it here. 
But the inspector uses it too, and it (also) is not React, so there is surely a way

This might be out of the scope of this bug though. So I would be OK landing a non-resizable panel and then using the split-box component later when we migrate this thing to React.

::: devtools/client/themes/animationinspector.css:282
(Diff revision 4)
>  .animation-timeline .scrubber .scrubber-handle {
>    position: absolute;
>    height: 100%;
>    /* Make it thick enough for easy dragging */
>    width: 6px;
> +  top: 5px;

I don't really understand why top needs to be 5px here. I've tried setting it to 0px and the scrubber little triangle appears outside the header. I don't understand why.
Can you explain?
Also, for me on Windows, top:4px works better, the triangle is exactly at the right position.

::: devtools/client/themes/animationinspector.css:323
(Diff revision 4)
>    height: var(--timeline-animation-height);
>    overflow: hidden;
>  }
>  
>  .animation-timeline .time-body {
> -  height: 100%;
> +  position: fixed;

This breaks the alignment between the vertical grey lines in the header-items and the ones in the time-body time-ticks.
Try this:
- display some animations
- notice that the vertical grey lines look fine, they look like continuous lines, because the ones in the body area are aligned with the ones in the header area
- click on an animation
- resize the animation details area to force a scrollbar to appear in the timeline area (on windows, I have real scrollbars (non-floating) that take space)
==> when this happens, the time tick lines are not continuous anymore.

::: devtools/client/themes/animationinspector.css:670
(Diff revision 4)
> +.animation-detail .animation-detail-header {
> +  position: relative;
> +  background-color: var(--theme-toolbar-background);
> +  border-top: 1px solid var(--theme-splitter-color);
> +  border-bottom: 1px solid var(--theme-splitter-color);
> +  font-size: 12px;
> +  padding: 5px;
> +}

There are probably css classes you can reuse instead of definining some of these properties.
Please try devtools-toolbar, or theme-toolbar. They contain common styles that can be useful here.

Also, there's a css variable in this stylesheet named --toolbar-height that you should probably use to get a height that is consistent with the other toolbars in this panel.

Also, we shouldn't need to define a font-size here. Font-size is defined at the global devtools level so it is consistent throughout the tools. So, no need to hard code a size here.
Attachment #8823601 - Flags: review?(pbrosset)
Comment on attachment 8823603 [details]
Bug 1210796 - Part 8: Add selection color to selected animation element.

https://reviewboard.mozilla.org/r/102148/#review114864
Attachment #8823603 - Flags: review?(pbrosset) → review+
Thank you for your kindly review, Patrick.
I'll fix according to your advice.
Thanks!
Attached image detail-testing.png — 
(In reply to Patrick Brosset <:pbro> from comment #114)
> I have done some testing with all commits applied, here are some things I
> found that we may want to fix:
> 

I attache a screeshot which is trying above points.

1. width of detail pane
“border-bottom-right-raidus” property is probably longest name.  It is not so bad, if make this property to be displayed for now. ( Although we can expand the width a bit, it probably about a few 'em'. If so, may better we fit to the timeline tick, I think. )

2. target DOM node
Will drop that.

3. Animated properties label
CSS Animation has the name, however, Script Animation may not have the name. So, how is this?
* Animated properties for anim2 - CSS Animation
* Animated properties for Script Animation

4. Indicator
I tried indicators color and style. I think, gray is not so bad since we use many colors in the detail pane if we append top triangle.
Attached image detail-testing-dark.png — 
Also, regarding to indicator color, lightgray may be better for Dark theme?
Brian and I talked about how to create the graph using distance instead of 'spacing'.

1. Push the distance information using nsDOMWindowUtil into getFrames of animation actor.
2. Use the distance to decide the coordinates of each keyframe.
3. Connect the points using bezier of specified easing.

I will try this.
I have changed following points.
Please review patch2 again as well.

patch2:
* Change the graph color of 'other' animation type.
* Change the way to create graph by distance.
* Change the way to create graph for discretable animation
* Implement color distance calculation in JavaScript.

patch 6:
* Use React component.
* Remove animation target component.
* Add animation name and type to header title.

patch 9:
* Change the color and style of progress indicator.

patch 11:
* Change cursor style on keyframe circle.

patch 14: 
* Change icon color of close button.

patch 15:
* Show the animation detail if the previously displayed animation is included in timeline list after refresh all UI. If not include, hide the component.

Thanks!
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review118164

Thanks for those changes, they look good.
I have made a few comments about reusing some helpful existing devtools code (for timing-function and color parsing).

::: devtools/client/animationinspector/components/animation-details.js:113
(Diff revisions 4 - 5)
>          if (!tracks[name]) {
>            tracks[name] = [];
>          }
>  
> -        for (let {value, offset, easing} of values) {
> -          tracks[name].push({value, offset, easing});
> +        for (let {value, offset, easing, distance} of values) {
> +          tracks[name].push({value, offset, easing, distance});

In this patch you changed `getProperties` so it would also return the distance. But, this is a breaking change from what the method returned before. So we have to make sure the code also works if we are connecting to an older server that does not return distances yet.
So we need to default to some distance value here in case it is missing.

::: devtools/client/animationinspector/components/animation-details.js:152
(Diff revisions 4 - 5)
> +  getAnimationTypes: Task.async(function* (propertyNames) {
> +    if (this.serverTraits.hasGetAnimationTypes) {
> +      return yield this.animation.getAnimationTypes(propertyNames);
> +    }
> +    // Set animation type 'none' since does not support getAnimationTypes.
> +    const animationTypes = {};
> +    propertyNames.forEach(propertyName => {
> +      animationTypes[propertyName] = "none";
> +    });
> +    return animationTypes;
> +  }),

It makes it easier if functions always return the same type. Here there are 2 cases: either the function returns a promise, or an object.
So, callers of this function don't know if they're supposed to use `.then` or not.

I suggest always returning a promise.

In the second case, where you already have the object, then you can return a promise that resolves immediately with the object.

`return new Promise(resolve => resolve(animationTypes));`

::: devtools/client/animationinspector/graph-helper.js:356
(Diff revisions 4 - 5)
> +  const pathSegments = [];
> +  keyframes.forEach(keyframe => {
> +    pathSegments.push({ x: keyframe.offset * duration,

Instead of creating an second empty array, then looping on the first array and pushing items to the 2nd, you can use `map`:

```
return keyframes.map(keyframe => {
  return {
    x: keyframe.offset * duration,
    y: keyframe.distance,
    easing: keyframe.easing,
    style: keyframe.value
  };
});
```

::: devtools/client/animationinspector/graph-helper.js:367
(Diff revisions 4 - 5)
> +  });
> +  return pathSegments;
> +}
> +
> +/**
> + * Create line path string which uses in path element.

"which is used" instead of "which uses"

::: devtools/client/animationinspector/graph-helper.js:376
(Diff revisions 4 - 5)
> +function createLinePathString(segment) {
> +  return ` L${ segment.x },${ segment.y }`;
> +}
> +
> +/**
> + * Create steps path string which uses in path element.

Similar comment here. "Create a steps path string used in path element"
Having said this, I don't know if this sentence makes sense in fact. Maybe simply: "Create a path string to represents a step function"

::: devtools/client/animationinspector/graph-helper.js:403
(Diff revisions 4 - 5)
> +  }
> +  return path;
> +}
> +
> +/**
> + * Create bezier curve path string which uses in path element.

Same comment here.

::: devtools/client/animationinspector/graph-helper.js:411
(Diff revisions 4 - 5)
> +    case "ease": {
> +      cp1x = .25;
> +      cp1y = .1;
> +      cp2x = .25;
> +      cp2y = 1;
> +      break;
> +    }
> +    case "ease-in": {
> +      cp1x = .42;
> +      cp1y = 0;
> +      cp2x = 1;
> +      cp2y = 1;
> +      break;
> +    }
> +    case "ease-out": {
> +      cp1x = 0;
> +      cp1y = 0;
> +      cp2x = .58;
> +      cp2y = 1;
> +      break;
> +    }
> +    case "ease-in-out": {
> +      cp1x = .42;
> +      cp1y = 0;
> +      cp2x = .58;
> +      cp2y = 1;
> +      break;
> +    }

We have another module in the tree that contains those values:

devtools\client\shared\widgets\CubicBezierPresets.js

You could import it here like so:

```
const {PREDEFINED} = require("devtools/client/shared/widgets/CubicBezierPresets");
```

And then get the numbers with:

```
const cp1x = PREDEFINED[currentSegment.easing][0];
const cp1y = PREDEFINED[currentSegment.easing][1];
const cp2x = PREDEFINED[currentSegment.easing][2];
const cp2y = PREDEFINED[currentSegment.easing][3];
```

::: devtools/client/animationinspector/graph-helper.js:440
(Diff revisions 4 - 5)
> +      // parse custom cubic bezier.
> +      const matches =
> +        currentSegment.easing.match(/^cubic-bezier\((.+),\s*(.+),\s*(.+),\s*(.+)\)/);

And we also have a timing-function parsing helper somewhere too:

devtools\client\shared\widgets\CubicBezierWidget.js

You can use the `parseTimingFunction` from this module.

Right now, this function is exported as `_parseTimingFunction` because the only external use for it is inside a test. But since you might use it too, you could rename the exported symbol to just `parseTimingFunction` and change all instances of `_parseTimingFunction` you can find in tests.

::: devtools/client/animationinspector/graph-helper.js:468
(Diff revisions 4 - 5)
> +function getRGBA(colorString) {
> +  const matches = colorString.match(/rgba?\((\d+),\s(\d+),\s(\d+)(,\s(.+))?\)/);
> +  const r = parseInt(matches[1], 10);
> +  const g = parseInt(matches[2], 10);
> +  const b = parseInt(matches[3], 10);
> +  const a = matches[5] ? parseFloat(matches[5]) : 1;
> +  return { r: r, g: g, b: b, a: a };
> +}

We already have a color utility in the tree: devtools/shared/css/color.js

There's an example of it being used here:
http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js#182

So, instead of adding this function, you could just use the utility instead. Something like this:

```
const color = new colorUtils.CssColor(colorString);
return color._getRGBATuple();
```

I think you should rename `_getRGBATuple` to `getRGBATuple` by the way. There is no reason this function should be marked as private. It is very useful.

::: devtools/server/actors/animation.js:538
(Diff revisions 4 - 5)
> +      // computeAnimationDistance could't compute this property name and values.
> +      console.warn(e);

Is this warning going to be helpful to someone? Do we actually need to find it in the log to fix `computeAnimationDistance`? 
If yes, then fine, let's leave it in. If `computeAnimationDistance` is expected to fail in some cases, then we might want to remove the warning.
Attachment #8823597 - Flags: review+
Comment on attachment 8823601 [details]
Bug 1210796 - Part 6: Fixed animation detail panel.

https://reviewboard.mozilla.org/r/102144/#review118184

Thanks for using the SplitBox component!

I have some UI comments:
- the issue I reported in comment 116 is still present. It might be hard to understand from that comment, so I recorded a gif: https://dl.dropboxusercontent.com/u/714210/mis-alignment-timeline.gif As you can see, when I make the timeline smaller, a scrollbar appears (expected) and the animations get smaller (also expected), but the header, ticks, and scrubber don't move. They should also be moving to adapt to the new timeline size.
- Let's say you have 2 animations displayed. You click on animation 1 (the properties panel opens up), then you click on animation 2 (the properties panel is updated), then you click again on animation 1: at this point the panel gets closed. This should not happen. In fact, I'm not sure clicking on animations should ever close the properties panel. It should just open it or change its content. But closing the properties panel would be better done with a close icon inside the panel's header instead.
- I noticed something weird with the splitbox too: click on an animation, the panel opens, now make the panel taller by dragging the splitter, now click on the same animation again: the panel gets closed, but a 1px horizontal line stays where the splitter was.

Apart from this, I have a couple of comments in the code.

::: devtools/client/animationinspector/components/animation-timeline.js:77
(Diff revision 5)
> +    const { Cu } = require("chrome");
> +    const { BrowserLoader } =
> +      Cu.import("resource://devtools/client/shared/browser-loader.js", {});

Please move this to animationinspector.xhtml just like we did in devtools\client\inspector\inspector.xhtml
This way, only the xhtml we load in the toolbox needs to be privileged.

::: devtools/client/animationinspector/components/animation-timeline.js:190
(Diff revision 5)
> +      }
> +    });
> +
> +    createNode({
> +      parent: headerTitleEl,
> +      textContent: "Animated properties for"

This string needs to be localized in devtools\client\locales\en-US\animationinspector.properties
Attachment #8823601 - Flags: review?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #139)
> - the issue I reported in comment 116 is still present. It might be hard to
> understand from that comment, so I recorded a gif:
> https://dl.dropboxusercontent.com/u/714210/mis-alignment-timeline.gif As you
> can see, when I make the timeline smaller, a scrollbar appears (expected)
> and the animations get smaller (also expected), but the header, ticks, and
> scrubber don't move. They should also be moving to adapt to the new timeline
> size.
I just tested with all commits applied, and I see this problem still exists. It also exists in the properties panel.
> - Let's say you have 2 animations displayed. You click on animation 1 (the
> properties panel opens up), then you click on animation 2 (the properties
> panel is updated), then you click again on animation 1: at this point the
> panel gets closed. This should not happen. In fact, I'm not sure clicking on
> animations should ever close the properties panel. It should just open it or
> change its content. But closing the properties panel would be better done
> with a close icon inside the panel's header instead.
> - I noticed something weird with the splitbox too: click on an animation,
> the panel opens, now make the panel taller by dragging the splitter, now
> click on the same animation again: the panel gets closed, but a 1px
> horizontal line stays where the splitter was.
However, these 2 last things are fixed when I test with all commits applied. So please ignore them.
Comment on attachment 8823604 [details]
Bug 1210796 - Part 9: Add progress indicator.

https://reviewboard.mozilla.org/r/102150/#review118212
Attachment #8823604 - Flags: review?(pbrosset) → review+
Comment on attachment 8833906 [details]
Bug 1210796 - Part 11: Disable clicking on keyframes because we can’t calculate correct current time since the animation detail handles the animation progress not time.

https://reviewboard.mozilla.org/r/110036/#review118214
Comment on attachment 8833905 [details]
Bug 1210796 - Part 10: Display animation's detail if the animation is only one in the list.

https://reviewboard.mozilla.org/r/110034/#review118216
Attachment #8833905 - Flags: review?(pbrosset) → review+
Comment on attachment 8837016 [details]
Bug 1210796 - Part 14: Add close button.

https://reviewboard.mozilla.org/r/112280/#review118218

Some CSS cleanup needed here.

::: devtools/client/animationinspector/components/animation-timeline.js:115
(Diff revision 2)
> +
> +    this.splitboxControlledEl = this.rootWrapperEl.querySelector(".controlled");
> +    this.splitboxControlledEl.dataset.defaultDisplayStyle =
> +      this.win.getComputedStyle(this.splitboxControlledEl).display;
> +    this.splitboxControlledEl.style.display = "none";
> +    this.splitboxControlledEl.style.height = "50%";

Why not use the initialHeight property of the SplitBox config object for this?

::: devtools/client/animationinspector/components/animation-timeline.js:209
(Diff revision 2)
> +    this.animationDetailCloseButton.addEventListener("click",
> +                                                     this.onDetailCloseButtonClick);

This event listener should be removed on destroy of the component too.

::: devtools/client/animationinspector/components/animation-timeline.js:619
(Diff revision 2)
>      this.currentTime = time;
>      this.details.indicateProgress(this.currentTime);
> +  },
> +
> +  onDetailCloseButtonClick: function (e) {
> +    this.splitboxControlledEl.style.display = "none";

I just thought of an easier way to hide the panel.
Instead of changing the inline display style here, you could simply toggle a class on one of the container elements.
The class could be "animation-details-visible" or something like that.
When you need to show the panel, you would just add this class with `classList.add()` and here, in this function you would just `classList.remove()`.

This way, you can just add a rule in the CSS file that applies when `:not(.animation-details-visible)` and defines `display:none` on the splitbox.

That way, you can remove `this.splitboxControlledEl.dataset.defaultDisplayStyle` in the code.

::: devtools/client/themes/animationinspector.css:708
(Diff revision 2)
> +.animation-detail .animation-detail-header .animation-inspector-close-button {
> +  position: absolute;
> +  top: 0;
> +  right: 0;
> +  border: none;
> +  margin: 1px 0;
> +  padding: 0 4px;
> +  width: 16px;
> +  height: 16px;
> +  background-color: var(--theme-body-background);
> +}
> +
> +.animation-inspector-close-button::before {
> +  content: "";
> +  position: absolute;
> +  top: 0;
> +  left: 0;
> +  width: 16px;
> +  height: 16px;
> +  background-image: var(--close-button-image);
> +  filter: var(--icon-filter);
> +}

You can get rid of almost everything here if you apply the class `devtools-button` to the element.

Also, you can get rid of the absolute positions and just let the button display itself in the flex layout. And then by using `flex:1` on the animation name, that will push the button all the way to the right.
Attachment #8837016 - Flags: review?(pbrosset)
Comment on attachment 8842651 [details]
Bug 1210796 - Part 15: Displays the animation detail if the previously displayed animation is included in timeline list after refresh all UI.

https://reviewboard.mozilla.org/r/116200/#review118226
Attachment #8842651 - Flags: review?(pbrosset) → review+
Thank you for quickly review, Patrick!
Will fix those!
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review118972

::: devtools/client/animationinspector/components/animation-details.js:162
(Diff revision 6)
> +    // Set animation type 'none' since does not support getAnimationTypes.
> +    const animationTypes = {};
> +    propertyNames.forEach(propertyName => {
> +      animationTypes[propertyName] = "none";
> +    });
> +    return new Promise(resolve => resolve(animationTypes));

Can't we just use `return Promise.resolve(animationTypes)`?
(In reply to Brian Birtles (:birtles) from comment #161)
> Comment on attachment 8823597 [details]
> Bug 1210796 - Part 2: Visualize each properties.
> 
> https://reviewboard.mozilla.org/r/102136/#review118972
> 
> ::: devtools/client/animationinspector/components/animation-details.js:162
> (Diff revision 6)
> > +    // Set animation type 'none' since does not support getAnimationTypes.
> > +    const animationTypes = {};
> > +    propertyNames.forEach(propertyName => {
> > +      animationTypes[propertyName] = "none";
> > +    });
> > +    return new Promise(resolve => resolve(animationTypes));
> 
> Can't we just use `return Promise.resolve(animationTypes)`?

Thanks, Brian,
We could use that!
Comment on attachment 8823597 [details]
Bug 1210796 - Part 2: Visualize each properties.

https://reviewboard.mozilla.org/r/102136/#review119966

Great work. Thanks for addressing my comments.

::: devtools/client/animationinspector/graph-helper.js:411
(Diff revision 7)
> + * @param {Object} currentSegment - e.g. { x: 0, y: 0, easing: "ease" }
> + * @param {Object} nextSegment - e.g. { x: 1, y: 1 }
> + * @return {String} path string - e.g. "C 0.25 0.1, 0.25 1, 1 1"
> + */
> +function createCubicBezierPathString(currentSegment, nextSegment) {
> +  const controllPoints = parseTimingFunction(currentSegment.easing);

nit: controlPoints instead of controllPoints
Attachment #8823597 - Flags: review?(pbrosset) → review+
Comment on attachment 8823606 [details]
Bug 1210796 - Part 13: Add tests.

https://reviewboard.mozilla.org/r/102154/#review119970

I have a few comments about these tests, but I don't think I need to re-review after you address them. So r=me here. Thanks!

::: devtools/client/animationinspector/test/browser.ini:26
(Diff revision 7)
>    !/devtools/client/inspector/test/head.js
>    !/devtools/client/inspector/test/shared-head.js
>    !/devtools/client/shared/test/test-actor-registry.js
>    !/devtools/client/shared/test/test-actor.js
>  
> +[browser_animation_detail_displayed.js]

Please keep the list of tests in alphabetical order. `browser_animation_detail_displayed.js` should go after `browser_animation_controller_exposes_document_currentTime.js`.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_path.js:4
(Diff revision 7)
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +

I'm a bit concerned by the length of this test. It might run really fast, but at first sight, it seems to me there's a chance it takes some time to run on debug builds. On CI, especially on linux debug, tests have a tendency to run much longer than locally, and go over the 45 seconds limit.
Did you already push these commits to try by any chance? If you did, it would be nice to look at the logs for linux debug, and see if this test (or others) are getting close to taking 45 seconds.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_path.js:7
(Diff revision 7)
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test animated property's path element.

Can you explain a bit more what a path is here. 
For someone looking at the animationinspector code for the first time, it might be useful to go over the tests, to learn what features exists. And so, having a good comment at the top of each test that really describes in details what the test is doing is really useful.
This could be a 2 lines description of where animated properties are shown, and how, and what is this path thing we test here.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_path.js:242
(Diff revision 7)
> +    yield clickOnAnimation(panel, i);
> +    info(`Click to select the animation[${ i }]`);

These 2 lines should be inverted, the info that says you're clicking on an animation should appear before you actually click.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_path.js:252
(Diff revision 7)
> +    for (let property in properties) {
> +      const testcase = properties[property];
> +      info(`Test path of ${ property }`);
> +      const className = testcase.expectedClass;
> +      const pathEl = detailEl.querySelector(`path.${ className }`);
> +      ok(pathEl, `Path element which has '${ className }' class should exist`);

Please rephase to `Path element with class ${ className } should exist`

::: devtools/client/animationinspector/test/browser_animation_animated_properties_path.js:298
(Diff revision 7)
> +  const stopEls = svgEl.querySelectorAll("stop");
> +  for (let i in stopEls) {
> +    const stopEl = stopEls[i];
> +    if (stopEl.getAttribute("offset") == offset) {
> +      return stopEl;
> +    }
> +  }
> +  return null;

This can be simplified to:

```
return [...svgEl.querySelectorAll("stop")].find(stopEl => {
  return stopEl.getAttribute("offset") == offset;
});
```

::: devtools/client/animationinspector/test/browser_animation_detail_displayed.js:15
(Diff revision 7)
> +let inspector;
> +let timelineComponent;
> +let animationDetailEl;

Don't these variables need to be nullified when the test ends?
To avoid having to define these globally, and in order to make this test look more like other tests, you could just use one `add_task` only.
So, keep the test as it is, but remove the these global variables, just define them within the first add_task, and remove the other 2 opening add_task below, so everything is part of just one add_task.

::: devtools/client/animationinspector/test/browser_animation_detail_displayed.js:51
(Diff revision 7)
> +  ok(animationDetailEl.querySelector(".property"),
> +     "The property in animation-detail element should be displayed");
> +});
> +
> +add_task(function* () {
> +  // 5. Resize the displaying area if user dragged on splitter.

I'm not sure we should be testing this here at all.
The splitter is a shared component that is already tested somewhere else. You're not really testing an animationinspector feature here, but a shared component feature.
I would simply remove this whole last part.
Attachment #8823606 - Flags: review?(pbrosset) → review+
Comment on attachment 8837016 [details]
Bug 1210796 - Part 14: Add close button.

https://reviewboard.mozilla.org/r/112280/#review119974

Thanks for addressing my comments. One more thing, the CSS can be further simplified if you use the devtools-button class I think. Unless there's a reason you did not want to use it.

::: devtools/client/animationinspector/components/animation-timeline.js:205
(Diff revision 4)
>  
> +    this.animationDetailCloseButton = createNode({
> +      parent: headerTitleEl,
> +      nodeType: "button",
> +      attributes: {
> +        "class": "animation-inspector-close-button",

Is there a reason you didn't use the `devtools-button` class like I mentioned in comment 144?
Attachment #8837016 - Flags: review?(pbrosset)
Comment on attachment 8844797 [details]
Bug 1210796 - Part 16: Move unchanged properties to bottom in the list.

https://reviewboard.mozilla.org/r/118066/#review119976
Attachment #8844797 - Flags: review?(pbrosset) → review+
Comment on attachment 8837016 [details]
Bug 1210796 - Part 14: Add close button.

https://reviewboard.mozilla.org/r/112280/#review121512

Nice. Thanks.
Attachment #8837016 - Flags: review?(pbrosset) → review+
Comment on attachment 8823601 [details]
Bug 1210796 - Part 6: Fixed animation detail panel.

https://reviewboard.mozilla.org/r/102144/#review121540

Alright, I believe this is the last R+, good job Daisuke!
Before landing though, could you give me a little bit more time? I'd love to do one more round of manual testing if possible. I'll also flag the bug to have some manual QA and some documentation on MDN.

::: devtools/client/themes/animationinspector.css:231
(Diff revisions 7 - 8)
>  #timeline-rate {
>    position: relative;
>    width: 4.5em;
>  }
>  
> +.animation-root > .unctrolled {

typo in the classname here: `.unctrolled` should probably be `.uncontrolled`.
Attachment #8823601 - Flags: review?(pbrosset) → review+
I built this patch series on try to be able to play with it a little bit locally. Works great on my simple tests.

I had more problems on this more complex test case though: http://www.primalscreen.com/
but it's not all due to your patches. This page is very heavy in terms of animations, and the animation panel is having a hard time keeping up. A lot of the problems would be solved by bug 1210363 I think.

Right now, if you just wait, you'll see a lot of animations appearing, and the panel is essentially impossible to use.

If you click on an animation, you can sometimes see a weird bug too: the 0%/50%/100% header is repeated several times, and the property distance is not graphed (see the attached screenshot).

It would be good to try and fix those 2 things. Do not attempt to fix the craziness of the panel refreshing constantly though, that's something we should do in another bug. But at least making sure the properties panel displays correctly in this case would be great, if possible.

I feel bad that we haven't had time to work on animation grouping. It would really help a lot in cases like this.
Flagging for MDN documentation and manual testing. This feature might not be so straightforward to test or document, so I think Daisuke and/or Brian should give a description here about the feature, in details.
Flags: qe-verify+
Keywords: dev-doc-needed
Attached file primalscreen-test.html — 
Thanks, Patrick.
I could reproduce both in attached test code.

1. Duplicated 0%/50%/100% header
I think, the header might create during rendering previous one.
Will investigate in especially patch 15.

2. Zero graph
I tested transform(-50%, 100%) -> transformX(-50%), then nsIDOMWindowUtils::computeAnimationDistance returns 0.
Also in case of transform(-50%, 100%) -> transform(-50%, 0%), returns 0.
But transform(-50%, 100px) -> transformX(-50%), transform(-50%, 100px) -> transform(-50%, 10%) returns 100. The method might not process for percentage.
Will try to fix computeAnimationDistance too.
(In reply to Daisuke Akatsuka (:daisuke) from comment #212)
> Created attachment 8847862 [details]
> primalscreen-test.html
> 
> 2. Zero graph
> I tested transform(-50%, 100%) -> transformX(-50%), then
> nsIDOMWindowUtils::computeAnimationDistance returns 0.
> Also in case of transform(-50%, 100%) -> transform(-50%, 0%), returns 0.
> But transform(-50%, 100px) -> transformX(-50%), transform(-50%, 100px) ->
> transform(-50%, 10%) returns 100. The method might not process for
> percentage.

But, processed correctly if something like height: 100% -> height: 0%. So, the problem might be only transform.
(In reply to Daisuke Akatsuka (:daisuke) from comment #213)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #212)
> > Created attachment 8847862 [details]
> > primalscreen-test.html
> > 
> > 2. Zero graph
> > I tested transform(-50%, 100%) -> transformX(-50%), then
> > nsIDOMWindowUtils::computeAnimationDistance returns 0.
> > Also in case of transform(-50%, 100%) -> transform(-50%, 0%), returns 0.
> > But transform(-50%, 100px) -> transformX(-50%), transform(-50%, 100px) ->
> > transform(-50%, 10%) returns 100. The method might not process for
> > percentage.

Oh sorry, not transform(-50%, 100%) -> transformX(-50%). 
transform: translate(-50%, 100%) -> transform: translateX(-50%)
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eab631dba666bbb9f5f33b5711a54d82d612c28

Hi Patrick,
I modified following points. Just to be on the safe side, could you confirm the behavior and code?

Part 2: https://reviewboard.mozilla.org/r/102136/diff/8-9/ in devtools/server/actors/animation.js 
* Assigned the keyframe offset as the distance in case of the value could not calculate by nsiDOMWindowUtils::ComputeAnimationDistance.
* Corresponded to missing keyframe.
( I don't know why https://reviewboard.mozilla.org/r/102136/diff/8-9/ has ColorWidget.js and SwatchColorPickerTooltip.js changes. https://reviewboard.mozilla.org/r/102136/diff/1-9/ looks fine,, )

Part 9: https://reviewboard.mozilla.org/r/102150/diff/9-11/
* Corresponded to different start time animation.

Part 13: https://reviewboard.mozilla.org/r/102154/diff/10-11/
* Add test for different start time animation.

Part 15: https://reviewboard.mozilla.org/r/116200/diff/5-7/
* Fix repeated header ( Reuse the detail view if the animation is not changed. )
* Add test for that there is no duplication.

Thanks!
Flags: needinfo?(pbrosset)
(In reply to Daisuke Akatsuka (:daisuke) from comment #243)
> ( I don't know why https://reviewboard.mozilla.org/r/102136/diff/8-9/ has
> ColorWidget.js and SwatchColorPickerTooltip.js changes.
> https://reviewboard.mozilla.org/r/102136/diff/1-9/ looks fine,, )

If you rebase your patches and then submit again, I'm pretty sure the interdiff shows the changes to the files you touched that happened as part of the rebase.
(In reply to Brian Birtles (:birtles) from comment #244)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #243)
> > ( I don't know why https://reviewboard.mozilla.org/r/102136/diff/8-9/ has
> > ColorWidget.js and SwatchColorPickerTooltip.js changes.
> > https://reviewboard.mozilla.org/r/102136/diff/1-9/ looks fine,, )
> 
> If you rebase your patches and then submit again, I'm pretty sure the
> interdiff shows the changes to the files you touched that happened as part
> of the rebase.

Oh, I see, Thanks!
Sorry, I found a bug for missing keyframe.
I modified server/actor/animation.js in patch 2.

Thanks.
The last modifying sometime fail on try-server.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc168680a418d21647b02d79a157519d5ecc5ed7
Will investigate.
I discussed with Brian about missing keyframes and animation which has no change value.

1) Missing keyframes
Will add a method to get computed style without animation rule in nsIDOMWindowUtils. ( patch 1 )

2) Unchanged value animation
Currently, we avoided to display if the animation value does not change in patch 16.
However we may be better to make to change a bit.
For example, if user set opacity[0.5, 0.5] as keyframes, it may user intent to change the opacity during animation duration.
So, we change to following behavior.
* sort
Unchanged value properties move to bottom in animated properties list.
* opacity
May better to change to dilute the color?

Thanks.
Comment on attachment 8823596 [details]
Bug 1210796 - Part 1: Add GetAnimationTypeForLonghand into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/102134/#review124790

This really should be two patches. Can you split it into two and then flag hiro and I for review on the second one?

(If you're worried about having to update the numbers of the other patches, just call it '1a' I guess.)

Also, we should add tests for CSS transitions (and maybe CSS animations too).

Clearing the review request since I'll review the updated patch once it has been split into two.

::: dom/base/nsDOMWindowUtils.cpp:2804
(Diff revisions 6 - 7)
> +  if (!aElement) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

I think we only take |content| so we can call AsElement() right? And then we don't check the return value of it.

I think we should just replace this part with the same arrangement we have in GetOMTAStyle:

  nsCOMPtr<Element> element = do_QueryInterface(aElement);
  if (!element) {
    return NS_ERROR_INVALID_ARG;
  }

::: dom/base/nsDOMWindowUtils.cpp:2818
(Diff revisions 6 - 7)
> +    nsCSSProps::LookupProperty(aProperty, CSSEnabledState::eForAllContent);
> +  if (propertyID == eCSSProperty_UNKNOWN) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +  if (nsCSSProps::IsShorthand(propertyID)) {
> +    // The given property should be longhand.

Nit: 'should be a longhand'

::: dom/base/nsDOMWindowUtils.cpp:2825
(Diff revisions 6 - 7)
> +  }
> +
> +  // Get style context of given element.
> +  nsIPresShell* shell = GetPresShell();
> +  if (!shell) {
> +    return NS_ERROR_UNEXPECTED;

It looks like we normally use NS_ERROR_FAILURE for the case of a null presshell.

::: dom/base/nsDOMWindowUtils.cpp:2836
(Diff revisions 6 - 7)
> +    : nullptr;
> +  RefPtr<nsStyleContext> styleContext =
> +    nsComputedDOMStyle::GetStyleContextForElementNoFlush(element,
> +                                                         pseudo, shell);
> +
> +  // Remove animation rule.

I don't think we need this comment.

::: dom/base/nsDOMWindowUtils.cpp:2842
(Diff revisions 6 - 7)
> +  RefPtr<nsStyleContext> styleContextWithoutAnimation =
> +    styleContext->PresContext()->StyleSet()->AsGecko()->
> +      ResolveStyleByRemovingAnimation(element, styleContext,
> +                                      eRestyle_AllHintsWithAnimations);
> +
> +  // Get computed value.

I don't think we need this comment.

::: dom/base/nsDOMWindowUtils.cpp:2847
(Diff revisions 6 - 7)
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  if (!StyleAnimationValue::UncomputeValue(propertyID,
> +                                           Move(computedValue), aResult)) {
> +    return NS_ERROR_UNEXPECTED;

Likewise, we should use NS_ERROR_FAILURE for both of these.

Although I wonder if we actually expect either of these to fail. In particular, if we get a result from ExtractComputedValue and pass it to UncomputeValue it shouldn't fail. If you search for other call sites of UncomputeValue you'll see they mostly assign the result to a DebugOnly<bool> then MOZ_ASSERT it is true.

The check for the result of StyleAnimationValue::ExtractComputedValue is probably fine though, although we should use NS_ERROR_FAILURE.

::: dom/base/test/test_domwindowutils.html:164
(Diff revisions 6 - 7)
> +  is(utils.getComputedStyleWithoutAnimation(div, "opacity"), "1",
> +     "opacity should be 1 before animate");
> +  const animation = div.animate({ opacity: [0, 1] }, 1000);
> +  animation.currentTime = 500;
> +  is(utils.getComputedStyleWithoutAnimation(div, "opacity"), "1",
> +     "opacity should be 1 even if while animating");

s/even if while animating/even while animating/

::: dom/base/test/test_domwindowutils.html:167
(Diff revisions 6 - 7)
> +  animation.currentTime = 500;
> +  is(utils.getComputedStyleWithoutAnimation(div, "opacity"), "1",
> +     "opacity should be 1 even if while animating");
> +  animation.cancel();
> +  is(utils.getComputedStyleWithoutAnimation(div, "opacity"), "1",
> +     "opacity should be 1 even after cancel the animation");

s/even after cancel/even after cancelling/

Then again, do we need this test? Why would it fail?

::: dom/base/test/test_domwindowutils.html:172
(Diff revisions 6 - 7)
> +    "NS_ERROR_ILLEGAL_VALUE",
> +    "background property should throw");
> +
> +  SimpleTest.doesThrow(
> +    () => utils.getComputedStyleWithoutAnimation(div, "invalid"),
>      "NS_ERROR_ILLEGAL_VALUE",
>      "Invalid property should throw");
> +
> +  SimpleTest.doesThrow(
> +    () => utils.getComputedStyleWithoutAnimation(null, "opacity"),
> +    "NS_ERROR_ILLEGAL_VALUE",

(These codes will need to be updated based on the feedback above.)

::: dom/interfaces/base/nsIDOMWindowUtils.idl:1567
(Diff revision 7)
> +   * Returns animation type of the given parameter.
> +   * The parameter aProperty should be CSS property name not IDL attribute.
> +   * Also, aProperty should be longhand property.
> +   * e.g. background-color.

/**
 * Returns the animation type of the specified property (e.g. 'coord').
 *
 * @param aProperty A longhand CSS property (e.g. 'background-color').
 */

::: dom/interfaces/base/nsIDOMWindowUtils.idl:1575
(Diff revision 7)
> +   * Returns computed style without animation rule of given element.
> +   * aProperty parameter should be CSS property name not IDL attribute.
> +   * Also, should be longhand property name.
> +   * e.g. background-color.

/**
 * Returns the computed style for the specified property on the given element
 * after removing styles from declarative animations.
 *
 * @param aProperty A longhand CSS property (e.g. 'background-color'
 */
Attachment #8823596 - Flags: review?(bbirtles)
Oh, I'm sorry. graph-helper was not added in patch 2 (because of added new patch before that?)
Anyway, will republish and throw to try-server.
(In reply to Daisuke Akatsuka (:daisuke) from comment #243)
> Part 2: https://reviewboard.mozilla.org/r/102136/diff/8-9/ in
> devtools/server/actors/animation.js 
> * Assigned the keyframe offset as the distance in case of the value could
> not calculate by nsiDOMWindowUtils::ComputeAnimationDistance.
Sounds reasonable to me.

> Part 13: https://reviewboard.mozilla.org/r/102154/diff/10-11/
> * Add test for different start time animation.
The new test looks good to me. Thanks.

> Part 15: https://reviewboard.mozilla.org/r/116200/diff/5-7/
> * Fix repeated header ( Reuse the detail view if the animation is not
> changed. )
> * Add test for that there is no duplication.
I did another review of the new test and left a few comments.
Flags: needinfo?(pbrosset)
Comment on attachment 8842651 [details]
Bug 1210796 - Part 15: Displays the animation detail if the previously displayed animation is included in timeline list after refresh all UI.

https://reviewboard.mozilla.org/r/116200/#review125368

::: devtools/client/animationinspector/components/animation-timeline.js:281
(Diff revision 11)
>      }
>      this[name] = [];
>    },
>  
>    unrender: function () {
> +    this.unrenderWithoutDetailsComponent();

The details component life-cycle is now different from the timeline one. This, to me, suggests that we should not make the timeline be responsible for the details component.
Both components should live at the same level instead, and there should be a parent component that links them together.

Let's do this later though, in a different bug. 

In the meantime, maybe rename this function `unrenderButLeaveDetailsComponent` ? I think it's easier to understand this way.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_for_delayed_starttime_animations.js:19
(Diff revision 11)
> -  yield setStyle(null, panel, "animation", "anim 100s", "#target2");
> -  yield setStyle(null, panel, "animation", "anim 100s", "#target3");
> -  yield setStyle(null, panel, "animation", "anim 100s", "#target4");
> -  yield setStyle(null, panel, "animation", "anim 100s", "#target5");
> +  yield setStyleAndWaitForAnimationSelecting(panel, "animation", "anim 100s", "#target2");
> +  yield setStyleAndWaitForAnimationSelecting(panel, "animation", "anim 100s", "#target3");
> +  yield setStyleAndWaitForAnimationSelecting(panel, "animation", "anim 100s", "#target4");
> +  yield setStyleAndWaitForAnimationSelecting(panel, "animation", "anim 100s", "#target5");
>  
> -  yield clickOnAnimation(panel, 1);
> +  // Check duplication.

This comment makes sense for us because we know now what's going on, but for someone not working on this bug reading the comment in some time, it won't make any sense.
Please either remove it or make it longer and more self-explanatory.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_for_delayed_starttime_animations.js:24
(Diff revision 11)
> -  yield clickOnAnimation(panel, 1);
> +  // Check duplication.
>    const timelineComponent = panel.animationsTimelineComponent;
>    const detailsComponent = timelineComponent.details;
> +  const headers =
> +    detailsComponent.containerEl.querySelectorAll(".animated-properties-header");
> +  is(headers.length, 1, "The property's header should be only one");

Please rephrase to "There should be only one header in the details panel"
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review125668

I think dholbert or heycam should review this since they reviewed the patch in bug 1277433 where we decided not to just resolve computed styles in StyleAnimationValue::ExtractComputedValue (which is why this patch is complicated). Before requesting their review, however, please add a description to the commit message explaining the purpose of this patch since it will make it easier for them to review and easier for others to understand later.

Clearing review for now since I think either heycam or dholbert should review this (although I notice they both have quite a few reviews at the moment).

::: dom/base/nsDOMWindowUtils.cpp:2811
(Diff revision 1)
> +  if (propertyID == eCSSProperty_UNKNOWN) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +  if (nsCSSProps::IsShorthand(propertyID)) {
> +    // The given property should be a longhand.
> +    return NS_ERROR_INVALID_ARG;
> +  }

Just combine these into one:

  if (propertyID == eCSSProperty_UNKNOWN ||
      nsCSSProps::IsShorthand(propertyID)) {
    return NS_ERROR_INVALID_ARG;
  }

I don't think you need the comment since the header file describes that |aProperty| must be a longhand and it's pretty obvious what this is doing.

::: dom/base/nsDOMWindowUtils.cpp:2819
(Diff revision 1)
> +  if (nsCSSProps::IsShorthand(propertyID)) {
> +    // The given property should be a longhand.
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  // Get style context of given element.

I'm not sure we need this comment. If we keep it we should add a blank line after where we assign |styleContext| and before we assign |styleContextWithoutAnimation| so that this stands as a separate block.

::: dom/base/nsDOMWindowUtils.cpp:2843
(Diff revision 1)
> +  if (computedValue.GetUnit() == StyleAnimationValue::eUnit_DiscreteCSSValue &&
> +      (computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Unset ||
> +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Initial ||
> +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Inherit)) {
> +    nsComputedDOMStyle::GetComputedValueFromStyleContext(
> +      element, styleContextWithoutAnimation, propertyID, aResult);
> +    return NS_OK;
> +  }

This block needs a comment explaining why we need special handling here.

::: dom/base/nsDOMWindowUtils.cpp:2844
(Diff revision 1)
> +      (computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Unset ||
> +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Initial ||
> +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Inherit)) {

Do we have a test for each of these three cases?

::: dom/base/test/test_domwindowutils.html:8
(Diff revision 1)
> +  <style>
> +  @keyframes cssanimation {
> +    from {
> +      opacity: 1;
> +    }
> +    to {
> +      opacity: 0;
> +    }
> +  }
> +  </style>

We can drop this apparently.

::: dom/base/test/test_domwindowutils.html:169
(Diff revision 1)
> +function test_getComputedStyleWithoutAnimation() {
> +  [
> +    {
> +      property: "opacity",
> +      keyframes: [1, 0],
> +      defaultlValue: "1",

defaultValue

::: dom/base/test/test_domwindowutils.html:176
(Diff revision 1)
> +      isDiscrete: false,
> +    },
> +    {
> +      property: "clear",
> +      keyframes: ["unset", "inline-end"],
> +      defaultlValue: "none",

defaultValue

::: dom/base/test/test_domwindowutils.html:196
(Diff revision 1)
> +    if (isDiscrete) {
> +      // We haven't support discretely animation for CSS Transition yet.
> +      // https://bugzilla.mozilla.org/show_bug.cgi?id=1320854
> +      return;
> +    }

Rather than doing an early return here, I think it would be better just to nest the check like so:

  // We don't support discrete animations for CSS transitions yet (bug 1320854)
  if (!isDiscrete) {
    ... test transitions ...
  }

Or alternatively, just split the transitions/animations/script animations into three separate tests and do an early return in the transitions test.

::: dom/base/test/test_domwindowutils.html:197
(Diff revision 1)
> +      return target.getAnimations()[0];
> +    }, "CSS Animation");
> +    document.styleSheets[0].deleteRule(0);
> +
> +    if (isDiscrete) {
> +      // We haven't support discretely animation for CSS Transition yet.

Nit: s/haven't/don't/
     s/discretely/discrete/
     s/CSS Transition/CSS Transitions/

::: dom/base/test/test_domwindowutils.html:202
(Diff revision 1)
> +      // We haven't support discretely animation for CSS Transition yet.
> +      // https://bugzilla.mozilla.org/show_bug.cgi?id=1320854
> +      return;
> +    }
> +
> +    checkComputedStyle(property, defaultlValue, whileTransitionStyle, target => {

defaultValue

::: dom/base/test/test_domwindowutils.html:230
(Diff revision 1)
> +
> +  next();
> +}
> +
> +function checkComputedStyle(property, expectedBeforeAnimation,
> +                            expectedWhileAnimation, animate, animationType) {

Nit: s/expectedWhileAnimation/expectedDuringAnimation/ would be more clear, I think.

::: dom/interfaces/base/nsIDOMWindowUtils.idl:1577
(Diff revision 1)
>  
>    /**
> +   * Returns the computed style for the specified property on the given element
> +   * after removing styles from declarative animations.
> +   *
> +   * @param aProperty A longhand CSS property (e.g. 'background-color'

Nit: Missing ')' at end.

::: layout/style/nsComputedDOMStyle.h:134
(Diff revision 1)
> +  /**
> +   * Get computed value of given property from given style context.
> +   * @param aElement - target element.
> +   * @param aContext - style context which is used to compute.
> +   * @param aPropID - css property id.
> +   * @param aResult - fill the result into this.
> +   */
> +  static void GetComputedValueFromStyleContext(mozilla::dom::Element* aElement,
> +                                               nsStyleContext* aContext,
> +                                               const nsCSSPropertyID aPropID,
> +                                               nsAString& aResult);
> +

Is this the right place in the file to add this method? We shouldn't just add new methods to the end but try to group it with other related methods.

In this case, I suspect it makes sense to add it after the GetStyleContextForElement... methods. This also happens to match the order in the .cpp file.

::: layout/style/nsComputedDOMStyle.h:135
(Diff revision 1)
> +   * Get computed value of given property from given style context.
> +   * @param aElement - target element.
> +   * @param aContext - style context which is used to compute.
> +   * @param aPropID - css property id.
> +   * @param aResult - fill the result into this.

Gets the computed value of a property from the given style context. This may be used, for example, to get the un-animated computed value of a property by supplying a style context with animation styles removed.

@param aElement  The target element.
@param aContext  The style context to used.
@param aPropID   The property whose computed value we should return.
@param aResult   A string that will be set to the computed value.

::: layout/style/nsComputedDOMStyle.cpp:720
(Diff revision 1)
> +nsComputedDOMStyle::GetComputedValueFromStyleContext(
> +  mozilla::dom::Element* aElement, nsStyleContext* aContext,
> +  const nsCSSPropertyID aPropID, nsAString& aResult)
> +{
> +  MOZ_ASSERT(!nsCSSProps::IsShorthand(aPropID),
> +             "nsCSSPropertyID should be a longhand property");

Nit: nsCSSPropertyID is the type so we should say either "aPropID should be..." or just "Property should be a longhand property".
Attachment #8850335 - Flags: review?(bbirtles)
(In reply to Patrick Brosset <:pbro> from comment #317)
> Comment on attachment 8842651 [details]
> Bug 1210796 - Part 15: Displays the animation detail if the previously
> displayed animation is included in timeline list after refresh all UI.
> 
> https://reviewboard.mozilla.org/r/116200/#review125368
> 
> ::: devtools/client/animationinspector/components/animation-timeline.js:281
> (Diff revision 11)
> >      }
> >      this[name] = [];
> >    },
> >  
> >    unrender: function () {
> > +    this.unrenderWithoutDetailsComponent();
> 
> The details component life-cycle is now different from the timeline one.
> This, to me, suggests that we should not make the timeline be responsible
> for the details component.
> Both components should live at the same level instead, and there should be a
> parent component that links them together.
> 
> Let's do this later though, in a different bug. 
> 

I filed the Bug 1350155. (The detail component should be same life-cycle of the timeline component)
Also, will fix your other comments.

Thanks!
Hi Daniel!
Could you review a patch of part 1a, especially the following files?

* layout/style/nsComputedDOMStyle.h
https://reviewboard.mozilla.org/r/122964/diff/2#3

* dom/base/nsDOMWindowUtils.cpp
https://reviewboard.mozilla.org/r/122964/diff/2#4

Thanks.
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review125968

::: dom/base/nsDOMWindowUtils.cpp:2820
(Diff revision 2)
> +  nsIAtom* pseudo =
> +    element->GetPseudoElementType() < CSSPseudoElementType::Count
> +    ? nsCSSPseudoElements::GetPseudoAtom(element->GetPseudoElementType())

Right now this is calling element->GetPseudoElementType() redundantly (twice in a row) in some cases.

Let's capture that in a local variable so that we only have to call it once.

::: dom/base/nsDOMWindowUtils.cpp:2824
(Diff revision 2)
> +  RefPtr<nsStyleContext> styleContext =
> +    nsComputedDOMStyle::GetStyleContextForElementNoFlush(element,
> +                                                         pseudo, shell);

I'm not really familiar with these GetStyleContextNoFlush / ResolveStyleByRemovingAnimation APIs -- which probably means heycam would be a better final reviewer than I here, if you can get some of his time. :-/

(Looks like heycam r+'d the ResolveStyleByRemovingAnimation impl, so he pobably understands the implications/invariants around it & related code better than I do.)

::: dom/base/nsDOMWindowUtils.cpp:2828
(Diff revision 2)
> +  RefPtr<nsStyleContext> styleContextWithoutAnimation =
> +    styleContext->PresContext()->StyleSet()->AsGecko()->
> +      ResolveStyleByRemovingAnimation(element, styleContext,
> +                                      eRestyle_AllHintsWithAnimations);

Three things:
 (1) Maybe shorten the variable name to "styleContextNoAnim"?  That will remove the need for some awkward line-wrapping later on (in the call to GetComputedValueFromStyleContext further down)

 (2) Is the "AsGecko()" call justified here?  If you call that function, you're *asserting* that this code is unreachable with a Servo/Stylo style-system backend -- but is that actually the case? I'm betting we can call DOMWindowUtils functions in Stylo builds, so I don't think this is valid...

 (3) I'd prefer we used a helper local variable to avoid the not-exactly-coding-style-style-conforming linewrapping here.  (We should only linewrap after -> when we have no other option, really.)  E.g. maybe capture the result of AsGecko() in a local variable (if that call is legitimate per (2) above), and then make the ResolveStyleByRemovingAnimation call on that local var?

::: dom/base/nsDOMWindowUtils.cpp:2840
(Diff revision 2)
> +  // If user sets 'unset' 'initial' or 'inherit' keywords to discretable
> +  // property (e.g. 'align-content'),
> +  // ExtractComputedValue returns the keywords as is.
> +  // Also, if user does not set any specific keyword, returns 'unset'.
> +  // In those cases, we get computed value from style context
> +  // which has no animation rules.

Two things:
(1) "discretable" isn't a word, as far as I know. :)  I think you mean "discretely-animatable", perhaps?

(2) It took me a little while to understand what this paragraph was saying. (Right now it seems a little like a series of factual statements, without a clear reason for what's motivating them / tying them together.)  Let's rephrase to make the motivation clearer, maybe like this:

  // Note: ExtractComputedValue can return 'unset', 'initial', or 'inherit' in
  // its "computedValue" outparam, even though these technically aren't valid
  // computed values. (It has this behavior for discretely-animatable
  // properties, e.g. 'align-content', when these keywords are explicitly
  // specified or when there is no specified value.)  But we need to return a
  // valid computed value -- these keywords won't do.  So we fall back to a
  // nsComputedDOMStyle convenience-method in this case.

::: dom/base/nsDOMWindowUtils.cpp:2846
(Diff revision 2)
> +  if (computedValue.GetUnit() == StyleAnimationValue::eUnit_DiscreteCSSValue &&
> +      (computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Unset ||
> +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Initial ||
> +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Inherit)) {
> +    nsComputedDOMStyle::GetComputedValueFromStyleContext(
> +      element, styleContextWithoutAnimation, propertyID, aResult);

If you shorten the variable to styleContextNoAnim, then you won't need to resort to the awkward deindentation-of-args here (which would be nice both for coding-style-conformance & because right now it's a bit hard to follow when combined the varying levels of indentation in the "if" statement above it).

Then this would look like this (under 80 chars):
    nsComputedDOMStyle::GetComputedValueFromStyleContext(element,
                                                         styleContextNoAnim,
                                                         propertyID, aResult);

::: layout/style/nsComputedDOMStyle.h:108
(Diff revision 2)
> +  /**
> +   * Gets the computed value of a property from the given style context.

s/the computed value/a string representation of the computed value/

::: layout/style/nsComputedDOMStyle.h:114
(Diff revision 2)
> +   * Gets the computed value of a property from the given style context.
> +   * This may be used, for example, to get the un-animated computed value of
> +   * a property by supplying a style context with animation styles removed.
> +   *
> +   * @param aElement - The target element.
> +   * @param aContext - The style context to used.

s/to/to be/

::: layout/style/nsComputedDOMStyle.h:115
(Diff revision 2)
> +   * This may be used, for example, to get the un-animated computed value of
> +   * a property by supplying a style context with animation styles removed.
> +   *
> +   * @param aElement - The target element.
> +   * @param aContext - The style context to used.
> +   * @param aPropID  - The property whose computed value we should return.

s/property/longhand property/

(The function's implementation asserts that this must be a longhand property.)

::: layout/style/nsComputedDOMStyle.cpp:714
(Diff revision 2)
> +/* static */ void
> +nsComputedDOMStyle::GetComputedValueFromStyleContext(
> +  mozilla::dom::Element* aElement, nsStyleContext* aContext,
> +  const nsCSSPropertyID aPropID, nsAString& aResult)
> +{
> +  MOZ_ASSERT(!nsCSSProps::IsShorthand(aPropID),
> +             "aPropID should be a longhand property");
> +

Two requests on this chunk:

(1) Drop the "mozilla::dom::" prefix here.

This file has "using namespace mozilla::dom" at the top, so it's unneeded. (And most other Element usages in this file don't have any namespace prefixing.)

(2) I'd prefer that you rewrap the params to put each on its own line. This makes them easier to read & to compare against callsites & against the decl in the .h file.

(The de-indentation is still necessary, though, because the 3rd arg is still too long to fit without being de-indented, even with 1 param per line.)
Attachment #8850335 - Flags: review?(dholbert) → review-
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review126008

Just noticed the extended commit message -- here are some nits on that. (Side note: as of the past few weeks, MozReview is *supposed* to make the commit message show up as a "virtual file" in the MozReview UI, so that reviewers can leave issues on specific lines. But that didn't happen here for some reason; might be worth figuring out what's going on & letting MozReview folks know so they can fix it.  Maybe related to a local quirk of your toolchain?  We ran into a different sort of trouble with some git<-->hg tools over in bug 1346321.)

Anyway, I'll just quote the commit message:
> In this patch, we implement nsiDOMWindowUtils::GetComputedStyleWithoutAnimation which returns computed value of given CSS property without animation rule.

s/nsiDOMWindowUtils/nsIDOMWindowUtils/ (capital I)

Ideally you should also wrap the lines of the extended commit message to 80 characters.  (everything after the first "Bug N: Do stuff. r=whoever" line)

> This method is used from AnimationInspector of devtools to make up for keyframes of animation.

The phrase "to make up for keyframes of animation" doesn't make sense to me, and I'm not sure what it's trying to say. Could you rephrase?

> Currently, although AnimationInspector gets the detail of keyframes using KeyframeEffectReadOnly::GetProperties, the value is undefined in case of missing keyframes.

I'm missing some context about this "missing keyframes" scenario. Could you add a bit more explanation about that?

> In this case, we need computed value on style context without animation since to use underlying value for.

The phrase "since to use underlying value for." doesn't make sense.  Please reword.

> to discretable property (e.g. 'align-content’)

As noted for the code, I don't think "discretable" is a defined term.  You probably want to say "discretely-animatable", I think?
> In order to implement this, we expand nsComputedDOMStyle.
> [...]
> We use nsComputedDOMStyle mechanism to change to absolute style as same as other property and so on ( e.g. ‘opacity’).

s/nsComputedDOMStyle mechanism/this new nsComputedDOMStyle mechanism/
  (On its own, "nsComputedDOMStyle mechanism" is ambiguous.)
s/change to absolute style/resolve these keywords into a valid keyword computed style/
(In reply to Daniel Holbert [:dholbert] from comment #338)
> s/change to absolute style/resolve these keywords into a valid keyword computed style/

Sorry, I left out a word ("for") -- meant to suggest "resolve these keywords into a valid keyword for computed style"
It sounds like there is quite a bit of review feedback to work in from Daniel's review so I will wait to review part 1a until that feedback is incorporated to avoid making overlapping comments.
(In reply to Daniel Holbert [:dholbert] from comment #337)
> Comment on attachment 8850335 [details]
> Bug 1210796 - Part 1a: Add GetComputedStyleWithoutAnimation into
> nsIDOMWindowUtils to use in animationinspector of devtools.
> 
> https://reviewboard.mozilla.org/r/122964/#review125968
> 
> ::: dom/base/nsDOMWindowUtils.cpp:2824
> (Diff revision 2)
> > +  RefPtr<nsStyleContext> styleContext =
> > +    nsComputedDOMStyle::GetStyleContextForElementNoFlush(element,
> > +                                                         pseudo, shell);
> 
> I'm not really familiar with these GetStyleContextNoFlush /
> ResolveStyleByRemovingAnimation APIs -- which probably means heycam would be
> a better final reviewer than I here, if you can get some of his time. :-/
> 
> (Looks like heycam r+'d the ResolveStyleByRemovingAnimation impl, so he
> pobably understands the implications/invariants around it & related code
> better than I do.)

Thank you very much, Daniel!

Hi Cameron!
Could you review a patch of part 1a?

> ::: dom/base/nsDOMWindowUtils.cpp:2828
> (Diff revision 2)
> > +  RefPtr<nsStyleContext> styleContextWithoutAnimation =
> > +    styleContext->PresContext()->StyleSet()->AsGecko()->
> > +      ResolveStyleByRemovingAnimation(element, styleContext,
> > +                                      eRestyle_AllHintsWithAnimations);
> 

>  (2) Is the "AsGecko()" call justified here?  If you call that function,
> you're *asserting* that this code is unreachable with a Servo/Stylo
> style-system backend -- but is that actually the case? I'm betting we can
> call DOMWindowUtils functions in Stylo builds, so I don't think this is
> valid...

I talked with Hiro.
He gave the advice as we should fix for Stylo in another bug after implement bug 1311257.
So, I added MOZ_ASSERT for Servo for now.

Thanks!
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review126160

::: dom/base/nsDOMWindowUtils.cpp:2828
(Diff revision 3)
> +                    : nullptr;
> +  RefPtr<nsStyleContext> styleContext =
> +    nsComputedDOMStyle::GetStyleContextForElementNoFlush(element,
> +                                                         pseudo, shell);
> +
> +  // We will support Servo after implement bug 1311257.

Nit: s/after implement/in/

::: dom/base/nsDOMWindowUtils.cpp:2829
(Diff revision 3)
> +  MOZ_ASSERT(styleContext->PresContext()->StyleSet()->IsGecko(),
> +             "Do not support Servo yet");

Rather than asserting, could we just return NS_ERROR_NOT_IMPLEMENTED in this case so that the browser doesn't crash when trying to debug animations in Stylo?

::: dom/base/test/test_domwindowutils.html:183
(Diff revision 3)
> +                         + ` from { ${property}: ${ keyframes[0] };}`
> +                         + ` to { ${property}: ${ keyframes[1] };} }`;

Nit: + lines should be indented by two spaces more.
Nit: Space after the ';' and before the '}' for consistency.

i.e. ` from { ${property}: ${ keyframes[0] }; }`

::: dom/base/test/test_domwindowutils.html:191
(Diff revision 3)
> +      checkComputedStyle(property, initialStyle,
> +                         expectedInitialStyle, expectedInitialStyle,
> +                         target => {
> +                           target.style.animation = "cssanimation 1s";
> +                           return target.getAnimations()[0];
> +                         }, "CSS Animation");

CSS Animations

::: dom/base/test/test_domwindowutils.html:212
(Diff revision 3)
> +    "NS_ERROR_ILLEGAL_VALUE",
> +    "Shorthand property should throw");
> +
> +  SimpleTest.doesThrow(
> +    () => utils.getComputedStyleWithoutAnimation(div, "invalid"),
> +    "NS_ERROR_ILLEGAL_VALUE",
> +    "Invalid property should throw");
> +
> +  SimpleTest.doesThrow(
> +    () => utils.getComputedStyleWithoutAnimation(null, "opacity"),
> +    "NS_ERROR_ILLEGAL_VALUE",

Should these be NS_ERROR_ILLEGAL_VALUE? I think they should be NS_ERROR_INVALID_ARG ?

::: dom/base/test/test_domwindowutils.html:241
(Diff revision 3)
> +
> +  is(utils.getComputedStyleWithoutAnimation(div, property),
> +     expectedBeforeAnimation,
> +     `'${ property }' property with '${ initialStyle }' style `
> +     + `should be '${ expectedBeforeAnimation }' `
> +     + `before animate by ${ animationType }`);

s/animate/animating/

::: layout/style/nsComputedDOMStyle.h:116
(Diff revision 3)
> +   * @param aPropID  - The longhand property
> +   *                   whose computed value we should return.

Nit: Wrapping here seems off. Probably should be:

   * @param aPropID  - The longhand property whose computed value we should
   *                   return.
Attachment #8850335 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #359)
> Comment on attachment 8850335 [details]
> Bug 1210796 - Part 1a: Add GetComputedStyleWithoutAnimation into
> nsIDOMWindowUtils to use in animationinspector of devtools.
> 
> https://reviewboard.mozilla.org/r/122964/#review126160

Thank you very much, Brian!
I fixed.
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review126200

::: dom/base/nsDOMWindowUtils.cpp:2833
(Diff revision 4)
> +  nsStyleSet* styleSet = styleContext->PresContext()->StyleSet()->AsGecko();
> +  RefPtr<nsStyleContext> styleContextNoAnim =
> +    styleSet->ResolveStyleByRemovingAnimation(element, styleContext,
> +                                              eRestyle_AllHintsWithAnimations);

Instead of calling GetStyleContextForElementNoFlush() and then ResolveStyleByRemovingAnimation() to remove the effects of the animation rules, can we instead add another argument to GetStyleContextForElementNoFlush, |AnimationFlag aAnimationFlag = eWithAnimation|, and just pass that argument down to DoGetStyleContextForElementNoFlush?  Or does that behave differently from what you're doing here?

::: dom/base/nsDOMWindowUtils.cpp:2844
(Diff revision 4)
> +  // Note: ExtractComputedValue can return 'unset', 'initial', or 'inherit' in
> +  // its "computedValue" outparam, even though these technically aren't valid
> +  // computed values. (It has this behavior for discretely-animatable
> +  // properties, e.g. 'align-content', when these keywords are explicitly
> +  // specified or when there is no specified value.)  But we need to return a
> +  // valid computed value -- these keywords won't do.  So we fall back to a
> +  // nsComputedDOMStyle convenience-method in this case.
> +  if (computedValue.GetUnit() == StyleAnimationValue::eUnit_DiscreteCSSValue &&
> +      (computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Unset ||
> +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Initial ||
> +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Inherit)) {
> +    nsComputedDOMStyle::GetComputedValueFromStyleContext(element,
> +                                                         styleContextNoAnim,
> +                                                         propertyID, aResult);
> +    return NS_OK;
> +  }

Just so I'm clear: if ExtractComputedValue returns inherit, then we do actually want to return the inherited value, right?

I wonder if instead of adding this helper GetcomputedValueFromStyleContext function, we can instead just make the JS caller handle these "initial" and "inherit" values.  So GetComputedStyleWithoutAnimation would return "initial" or "inherit" strings that devtools can check for explicitly.  

For "inherit", JS can just call getComputedStyle() on the parent element.

For "initial", is there some data that devtools has available to get the initial value of a given property?

If the value from ExtractComputedValue is eCSSUnit_Unset, then we can turn that into "initial" or "inherit" according to whether propertyID is an inherited property.

What do you think?

::: layout/style/nsComputedDOMStyle.cpp:714
(Diff revision 4)
> +/* static */ void
> +nsComputedDOMStyle::GetComputedValueFromStyleContext(

(If my comment above sounds OK, then we don't need this function.)

::: layout/style/nsComputedDOMStyle.cpp:724
(Diff revision 4)
> +  nsComputedDOMStyle computedStyle(aElement, EmptyString(), aContext->Arena(),
> +                                   nsComputedDOMStyle::eDefaultOnly);

I don't think we should create this refcounted object on the stack.

Also, it feels a bit weird to use aContext->Arena() as the way to get the pres shell.  Instead, can you add an nsIPresShell* argument to GetComputedValueFromStyleContext, and up in the caller, do something like:

  nsIDocument* doc = aElement->GetUncomposedDoc();
  if (!doc) {
    return NS_ERROR_FAILURE;
  }

  nsIPresShell* shell = doc->GetShell();
  if (!shell) {
    return NS_ERROR_FAILURE;
  }

and then pass |shell| in.

::: layout/style/nsComputedDOMStyle.cpp:728
(Diff revision 4)
> +
> +  nsComputedDOMStyle computedStyle(aElement, EmptyString(), aContext->Arena(),
> +                                   nsComputedDOMStyle::eDefaultOnly);
> +  computedStyle.mStyleContext = aContext;
> +  const nsComputedStyleMap::Entry* propEntry =
> +    computedStyle.GetComputedStyleMap()->FindEntryForProperty(aPropID);

Nit: since GetComputedStyleMap is a static method, better to write this as |propEntry = GetComputedStyleMap()->...|.
(In reply to Cameron McCormack (:heycam) from comment #377)
> Comment on attachment 8850335 [details]
> Bug 1210796 - Part 1a: Add GetComputedStyleWithoutAnimation into
> nsIDOMWindowUtils to use in animationinspector of devtools.
> 
> https://reviewboard.mozilla.org/r/122964/#review126200
> 
> ::: dom/base/nsDOMWindowUtils.cpp:2833
> (Diff revision 4)
> > +  nsStyleSet* styleSet = styleContext->PresContext()->StyleSet()->AsGecko();
> > +  RefPtr<nsStyleContext> styleContextNoAnim =
> > +    styleSet->ResolveStyleByRemovingAnimation(element, styleContext,
> > +                                              eRestyle_AllHintsWithAnimations);
> 
> Instead of calling GetStyleContextForElementNoFlush() and then
> ResolveStyleByRemovingAnimation() to remove the effects of the animation
> rules, can we instead add another argument to
> GetStyleContextForElementNoFlush, |AnimationFlag aAnimationFlag =
> eWithAnimation|, and just pass that argument down to
> DoGetStyleContextForElementNoFlush?  Or does that behave differently from
> what you're doing here?

Maybe we should just rework nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation to not flush. No one uses it (anymore?).

> ::: dom/base/nsDOMWindowUtils.cpp:2844
> (Diff revision 4)
> > +  // Note: ExtractComputedValue can return 'unset', 'initial', or 'inherit' in
> > +  // its "computedValue" outparam, even though these technically aren't valid
> > +  // computed values. (It has this behavior for discretely-animatable
> > +  // properties, e.g. 'align-content', when these keywords are explicitly
> > +  // specified or when there is no specified value.)  But we need to return a
> > +  // valid computed value -- these keywords won't do.  So we fall back to a
> > +  // nsComputedDOMStyle convenience-method in this case.
> > +  if (computedValue.GetUnit() == StyleAnimationValue::eUnit_DiscreteCSSValue &&
> > +      (computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Unset ||
> > +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Initial ||
> > +       computedValue.GetCSSValueValue()->GetUnit() == eCSSUnit_Inherit)) {
> > +    nsComputedDOMStyle::GetComputedValueFromStyleContext(element,
> > +                                                         styleContextNoAnim,
> > +                                                         propertyID, aResult);
> > +    return NS_OK;
> > +  }
> 
> Just so I'm clear: if ExtractComputedValue returns inherit, then we do
> actually want to return the inherited value, right?
> 
> I wonder if instead of adding this helper GetcomputedValueFromStyleContext
> function, we can instead just make the JS caller handle these "initial" and
> "inherit" values.  So GetComputedStyleWithoutAnimation would return
> "initial" or "inherit" strings that devtools can check for explicitly.

I thought about handling this in JS, but it seems like it belongs in the C++ side? As I understand it, the fact that we get initial/inherit/unset for discretely animated properties is because of certain shortcuts/optimizations we took when implementing discrete animation? If so, it seems like we should handle that on the Gecko side and make this API always return computed values regardless of the animation type?
(In reply to Brian Birtles (:birtles) from comment #378)
> Maybe we should just rework
> nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation to not flush.
> No one uses it (anymore?).

Ah, that'd be fine too.

> I thought about handling this in JS, but it seems like it belongs in the C++
> side? As I understand it, the fact that we get initial/inherit/unset for
> discretely animated properties is because of certain shortcuts/optimizations
> we took when implementing discrete animation? If so, it seems like we should
> handle that on the Gecko side and make this API always return computed
> values regardless of the animation type?

I don't recall the exact reasons behind returning those values, but I guess it does make sense conceptually to handle them on the C++ side.  But I'm wary of adding this nsComputedDOMStyle helper function that pokes at its insides to be able to extract computed values without the problems that would come from using its regular interface (passing it an element, and calling its GetPropertyValue method).

What if we extend the nsComputedDOMStyle constructor with an AnimationFlag argument, and handle it similarly to aStyleType?  This would let us create and use "normal" nsComputedDOMStyle object.
(In reply to Cameron McCormack (:heycam) from comment #379)
> (In reply to Brian Birtles (:birtles) from comment #378)

> > I thought about handling this in JS, but it seems like it belongs in the C++
> > side? As I understand it, the fact that we get initial/inherit/unset for
> > discretely animated properties is because of certain shortcuts/optimizations
> > we took when implementing discrete animation? If so, it seems like we should
> > handle that on the Gecko side and make this API always return computed
> > values regardless of the animation type?
> 
> I don't recall the exact reasons behind returning those values, but I guess
> it does make sense conceptually to handle them on the C++ side.  But I'm
> wary of adding this nsComputedDOMStyle helper function that pokes at its
> insides to be able to extract computed values without the problems that
> would come from using its regular interface (passing it an element, and
> calling its GetPropertyValue method).
> 
> What if we extend the nsComputedDOMStyle constructor with an AnimationFlag
> argument, and handle it similarly to aStyleType?  This would let us create
> and use "normal" nsComputedDOMStyle object.

Thank you very much, Cameron, Brian!

We have some reasons why can not calculate in JS.
1. In order to get the style without animation using getComputedStyle, I tried to set animation.currentTime to zero. ( or animation.cancel() )
   However, the devtools handles that event using MutationObsever.
2. If we re-set the keyword such 'initial' to element's style, animation run if the animation is CSS Transitions.
3. To resolve above problems, although we may be able to use cloneNode and appendChild, that process is a bit heavy.

I'll try to extend nsComputedDOMStyle constructor or GetStyleContextForElementNoFlush method.

Thanks!
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review126542

(It looks like you've addressed all my review feedback here -- thanks! -- though it seems this will be changing a bit.  For subsequent revisions, no need to tag me for review, & you should prefer heycam over me I think.  --> Canceling review request.)
Attachment #8850335 - Flags: review?(dholbert)
I asked to hiro on irc regarding to DoGetStyleContextForElementNoFlush with eWithoutAnimation flag.
This meant, get style context without updating the animation.
And, he recommended GetStyleContextForElement instead of GetStyleContextForElementNoFlush.
So, will use GetStyleContextForElement and ResolveStyleByRemovingAnimation to get a StyleContext without animation rule.

Also, he gave an advice that we can use the cached base style from KeyframeEffectReadOnly::BaseStyle in this case.
It will be faster than create and use StyleContext.
Will try that as well.

The algorithm outline will be like following:
1. Get cached base style from KeyframeEffectReadonly.
2. If animation type of cached value is 'discrete' and the value is 'initial', 'unset' or 'inherit',
   we use nsComputedDOMStyle mechanism with a StyleContext without animation rule.
3. Otherwise, call StyleAnimationValue::UncomputeValue.

Thanks.
(In reply to Daisuke Akatsuka (:daisuke) from comment #382)
> I asked to hiro on irc regarding to DoGetStyleContextForElementNoFlush with
> eWithoutAnimation flag.
> This meant, get style context without updating the animation.
> And, he recommended GetStyleContextForElement instead of
> GetStyleContextForElementNoFlush.
> So, will use GetStyleContextForElement and ResolveStyleByRemovingAnimation
> to get a StyleContext without animation rule.
> 
> Also, he gave an advice that we can use the cached base style from
> KeyframeEffectReadOnly::BaseStyle in this case.
> It will be faster than create and use StyleContext.
> Will try that as well.

I spoke with Daisuke about this part, and I think it's a little bit risky. We only fill in the base style when we need it and I'm concerned that there could arise some discrepancy between when DevTools wants to look up the base style, and when Gecko thinks it is needed.

For example, DevTools might be using cached data, or Gecko might introduce some optimization when we can skip fetching base values in some situation that DevTools still wants them (e.g. Gecko could decide that since there is an overriding 'replace' animation we don't need base styles, but DevTools will still want base styles when showing the overridden animation). If that were to happen a bug would arise in DevTools where the animation value would be empty.

From what I understand we don't call this method a lot (primarily just when we anticipate a new animation, I believe, but perhaps also when one changes) so if performance is the only reason to prefer KeyframeEffectReadOnly::BaseStyle then I think we can just take the simpler and more robust approach of using GetStyleContextForElement and ResolveStyleByRemovingAnimation for now (until we discover this is a performance issue).
Thanks Brian!

I updated as following.
* Leave the code which gets computed value from style context.
  But use GetStyleContextForElement instead of GetStyleContextForElementNoFlush.
* Extend nsComputedDOMStyle constructor.
  Also, I added new enum StyleContextRule since AnimationFlag has different meaning.
(In reply to Daisuke Akatsuka (:daisuke) from comment #382)
> I asked to hiro on irc regarding to DoGetStyleContextForElementNoFlush with
> eWithoutAnimation flag.
> This meant, get style context without updating the animation.
> And, he recommended GetStyleContextForElement instead of
> GetStyleContextForElementNoFlush.
> So, will use GetStyleContextForElement and ResolveStyleByRemovingAnimation
> to get a StyleContext without animation rule.

I was actually looking at GetStyleContextForElementWithoutAnimation as part of bug 1315874. It's an odd method! If there's a frame we just pull the style context off it, despite the fact that it may include animation styles. If there's no existing style context we resolve one without animation styles.

I'm probably misunderstanding something, but I wonder if it makes sense to create GetNonAnimationStyleContextNoFlush that just wraps DoGetStyleContextForElementNoFlush and passes eWithoutAnimation. Then, in DoGetStyleContextForElementNoFlush, if we find an existing style context on the frame and we have eWithoutAnimation, call ResolveStyleByRemovingAnimation. Does that make sense? I think that's what I want to use in bug 1315874 so I'll try it there.
(In reply to Brian Birtles (:birtles) from comment #420)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #382)
> > I asked to hiro on irc regarding to DoGetStyleContextForElementNoFlush with
> > eWithoutAnimation flag.
> > This meant, get style context without updating the animation.
> > And, he recommended GetStyleContextForElement instead of
> > GetStyleContextForElementNoFlush.
> > So, will use GetStyleContextForElement and ResolveStyleByRemovingAnimation
> > to get a StyleContext without animation rule.
> 
> I'm probably misunderstanding something, but I wonder if it makes sense to
> create GetNonAnimationStyleContextNoFlush that just wraps
> DoGetStyleContextForElementNoFlush and passes eWithoutAnimation. Then, in
> DoGetStyleContextForElementNoFlush, if we find an existing style context on
> the frame and we have eWithoutAnimation, call
> ResolveStyleByRemovingAnimation. Does that make sense? I think that's what I
> want to use in bug 1315874 so I'll try it there.

Oh, I want to use that implementation instead of GetStyleContextForElement and ResolveStyleByRemovingAnimation in the patch part 2.
Thanks!
Blocks: 1309468
(In reply to Daisuke Akatsuka (:daisuke) from comment #421)
> (In reply to Brian Birtles (:birtles) from comment #420)
> > (In reply to Daisuke Akatsuka (:daisuke) from comment #382)
> > > I asked to hiro on irc regarding to DoGetStyleContextForElementNoFlush with
> > > eWithoutAnimation flag.
> > > This meant, get style context without updating the animation.
> > > And, he recommended GetStyleContextForElement instead of
> > > GetStyleContextForElementNoFlush.
> > > So, will use GetStyleContextForElement and ResolveStyleByRemovingAnimation
> > > to get a StyleContext without animation rule.
> > 
> > I'm probably misunderstanding something, but I wonder if it makes sense to
> > create GetNonAnimationStyleContextNoFlush that just wraps
> > DoGetStyleContextForElementNoFlush and passes eWithoutAnimation. Then, in
> > DoGetStyleContextForElementNoFlush, if we find an existing style context on
> > the frame and we have eWithoutAnimation, call
> > ResolveStyleByRemovingAnimation. Does that make sense? I think that's what I
> > want to use in bug 1315874 so I'll try it there.
> 
> Oh, I want to use that implementation instead of GetStyleContextForElement
> and ResolveStyleByRemovingAnimation in the patch part 2.
> Thanks!

This has landed on mozilla-central now (GetUnanimatedStyleContextNoFlush):

  http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/layout/style/nsComputedDOMStyle.h#110
Yap, Thanks! I'm making with it now!
Hi Cameron,
Could you re-review patch 1a?
Thanks.
Flags: needinfo?(cam)
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review130192

One major question, and a few comments (so I'd like to see one more version of the patch).

::: dom/base/nsDOMWindowUtils.cpp:2822
(Diff revision 7)
> +  CSSPseudoElementType pseudoType = element->GetPseudoElementType();
> +  nsIAtom* pseudo = pseudoType < CSSPseudoElementType::Count
> +                    ? nsCSSPseudoElements::GetPseudoAtom(pseudoType)
> +                    : nullptr;

I wonder if this is the right thing to be doing.  Is the consumer of this API expecting to call getUnanimatedComputedStyle on the DOM Element that implements a pseudo-element?  For example, the native anonymous <div> that an <input> element creates for its ::placeholder?

Or should we instead be passing in the pseudo element string "::placeholder" and the originating element (the <input>)?  That is how nsDOMWindowUtils::GetVisitedDependentComputedStyle works, for example.

::: dom/base/nsDOMWindowUtils.cpp:2832
(Diff revision 7)
> +  RefPtr<nsStyleContext> styleContext =
> +    nsComputedDOMStyle::GetUnanimatedStyleContextNoFlush(element,
> +                                                         pseudo, shell);
> +
> +  // We will support Servo in bug 1311257.
> +  if (styleContext->PresContext()->StyleSet()->IsServo()) {

May as well get this through |shell->StyleSet()| instead.

::: dom/base/nsDOMWindowUtils.cpp:2853
(Diff revision 7)
> +    RefPtr<nsComputedDOMStyle> computedStyle =
> +      NS_NewComputedDOMStyle(
> +       element, EmptyString(), shell,
> +       nsComputedDOMStyle::StyleType::eAll,
> +       nsComputedDOMStyle::AnimationFlag::eWithoutAnimation);

Does this NS_NewComputedDOMStyle call return exactly the same styles as the GetUnanimatedStyleContextNoFlush call earlier?  I guess it's the same, except that this call here might flush, but the earlier call doesn't.  Is it important that we don't flush?

(It seems like it's a bit of a waste to create a whole new nsComputedDOMStyle object, resolving styles again, when we have the nsStyleContext above that has the right data in it, we just don't have an nsComputedDOMStyle object to call its GetPropertyValue API.  I suppose this case is rare.  But in the long term maybe we should refactor nsComputedDOMStyle so that it can return computed property value strings from an arbitrary nsStyleContext.)

::: dom/base/test/test_domwindowutils.html:228
(Diff revision 7)
> +    "Null element should throw");
> +
> +  next();
> +}
> +
> +function checkComputedStyle(property, initialStyle, expectedBeforeAnimation,

Maybe call this "checkUnanimatedComputedStyle", so it's clear from the callers that it's not checking regular computed style.

::: layout/style/nsComputedDOMStyle.h:768
(Diff revision 7)
>     * UpdateCurrentStyleSources.  Initially false.
>     */
>    bool mResolvedStyleContext;
>  
> +  /**
> +   * Whether we include animation rule to computed style.

"Whether we include animation rules in the computed style."

::: layout/style/nsComputedDOMStyle.cpp:72
(Diff revision 7)
> +                       nsComputedDOMStyle::AnimationFlag aFlag
> +)

Nit: ")" on the previous line.

::: layout/style/nsComputedDOMStyle.cpp:888
(Diff revision 7)
> +    MOZ_ASSERT(mStyleContext->PresContext()->StyleSet()->IsGecko(),
> +               "eWithoutAnimationRules support Gecko only");
> +    nsStyleSet* styleSet = mStyleContext->PresContext()->StyleSet()->AsGecko();

Get the style set with |mPresShell->StyleSet()|.

::: layout/style/nsComputedDOMStyle.cpp:891
(Diff revision 7)
> +    mStyleContext = styleSet->ResolveStyleByRemovingAnimation(
> +                      mContent->AsElement(),
> +                      mStyleContext,
> +                      eRestyle_AllHintsWithAnimations);

You should use SetResolvedStyleContext instead of just assigning to mStyleContext, otherwise we will need to recompute styles every time a query is made to the nsComputedDOMStyle object.  (See how ClearCurrentStyleSources, which is called at the end of functions like GetPropertyCSSValue, will clear out mStyleContext unless it's recorded as being a "resolved" style context.)
Attachment #8850335 - Flags: review?(cam) → review-
Flags: needinfo?(cam)
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review130192

> I wonder if this is the right thing to be doing.  Is the consumer of this API expecting to call getUnanimatedComputedStyle on the DOM Element that implements a pseudo-element?  For example, the native anonymous <div> that an <input> element creates for its ::placeholder?
> 
> Or should we instead be passing in the pseudo element string "::placeholder" and the originating element (the <input>)?  That is how nsDOMWindowUtils::GetVisitedDependentComputedStyle works, for example.

Thanks, Cameron. Yeah, I was wrong.. 
Although, I sent animation.effect.target as the target from DevTools, the element was CSSPseudoElement[1] in case of  pseudo element. So, I could not calculate since it is not nsIDOMElement.
I modified to use same way of GetVisitedDependentComputedStyle.
Also, added a test for pseudo element.

[1] https://searchfox.org/mozilla-central/source/dom/webidl/CSSPseudoElement.webidl

> Does this NS_NewComputedDOMStyle call return exactly the same styles as the GetUnanimatedStyleContextNoFlush call earlier?  I guess it's the same, except that this call here might flush, but the earlier call doesn't.  Is it important that we don't flush?
> 
> (It seems like it's a bit of a waste to create a whole new nsComputedDOMStyle object, resolving styles again, when we have the nsStyleContext above that has the right data in it, we just don't have an nsComputedDOMStyle object to call its GetPropertyValue API.  I suppose this case is rare.  But in the long term maybe we should refactor nsComputedDOMStyle so that it can return computed property value strings from an arbitrary nsStyleContext.)

I think, we don't need to flush at this time at least in case of DevTools uses. 
I wanted to avoid to flush is for the performance. 

> It seems like it's a bit of a waste to create a whole new nsComputedDOMStyle object, resolving styles again

Yeah, so, our first approarch was to get computed style using nsStyleContext which we already built.
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review130554

OK, this is looking good.  Just one more change, please!

::: dom/base/nsDOMWindowUtils.cpp:2824
(Diff revision 8)
> +  nsCOMPtr<nsIAtom> pseudo = nullptr;
> +  if (!aPseudoElement.IsEmpty() &&
> +      aPseudoElement.First() == char16_t(':')) {
> +    nsAString::const_iterator start, end;
> +    aPseudoElement.BeginReading(start);
> +    aPseudoElement.EndReading(end);
> +    NS_ASSERTION(start != end, "aPseudoElement is not empty!");
> +    ++start;
> +    if (start == end || *start != char16_t(':')) {
> +      --start;
> +    }
> +    pseudo = NS_Atomize(Substring(start, end));
> +    MOZ_ASSERT(pseudo);
> +  }

This code is pretty similar to the code in the nsComputedDOMStyle constructor.  I think we should just take the existing functionality in the nsComputedDOMStyle constructor, move it to a static method on nsCSSPseudoElements, and then call it from there and from here.  (And I think we should include the check for two colons or one colon that the nsComputedDOMStyle constructor does, just so that getUnanimatedComputedStyle is consistent with getComputedStyle.)

::: layout/style/nsComputedDOMStyle.cpp:887
(Diff revision 8)
> +    MOZ_ASSERT(mStyleContext->PresContext()->StyleSet()->IsGecko(),
> +               "eWithoutAnimationRules support Gecko only");

Use mPresShell->StyleSet() here too.
Attachment #8850335 - Flags: review?(cam) → review-
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review130554

> This code is pretty similar to the code in the nsComputedDOMStyle constructor.  I think we should just take the existing functionality in the nsComputedDOMStyle constructor, move it to a static method on nsCSSPseudoElements, and then call it from there and from here.  (And I think we should include the check for two colons or one colon that the nsComputedDOMStyle constructor does, just so that getUnanimatedComputedStyle is consistent with getComputedStyle.)

Thank you very much!
I fixed this one.
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review130642

Looks great, thanks!

::: layout/style/nsCSSPseudoElements.h:95
(Diff revision 9)
>    static Type GetPseudoType(nsIAtom* aAtom, EnabledState aEnabledState);
>  
>    // Get the atom for a given Type.  aType must be < CSSPseudoElementType::Count
>    static nsIAtom* GetPseudoAtom(Type aType);
>  
> +  // Get the atom for a give nsAString. (e.g. "::before")

given
Attachment #8850335 - Flags: review?(cam) → review+
Comment on attachment 8850335 [details]
Bug 1210796 - Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools.

https://reviewboard.mozilla.org/r/122964/#review130642

Thank you very much!
Hi Patrick,
Finally, finished all! (looks like)
Could you confirm?
After your confirming, I'd like to land them.

Thanks.
Flags: needinfo?(pbrosset)
(In reply to Daisuke Akatsuka (:daisuke) from comment #498)
> Hi Patrick,
> Finally, finished all! (looks like)
> Could you confirm?
> After your confirming, I'd like to land them.
> 
> Thanks.
Congrats Daisuke for working on this bug. Almost 500 comments!!! 
I've tested this locally a few times already, so I can confirm now without any second thoughts. BUT: we're 1 week away from the next merge. I feel bad asking you to hold off and rebase this again, but I would feel better if that landed at the beginning of a cycle. What do you think? This way we can get some QA done and use the full cycle to address follow-up bugs rather than have to uplift things quickly?
Flags: needinfo?(pbrosset)
Wao! Just 500! :-)

Thank you for your advice, Patrick.
Okay! I will make them to land at 18th Apr.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba19624101a2
Part 1: Add GetAnimationTypeForLonghand into nsIDOMWindowUtils to use in animationinspector of devtools. r=hiro
https://hg.mozilla.org/integration/autoland/rev/99574da8257c
Part 1a: Add GetUnanimatedComputedStyle into nsIDOMWindowUtils to use in animationinspector of devtools. r=birtles,heycam
https://hg.mozilla.org/integration/autoland/rev/844922324bc0
Part 2: Visualize each properties. r=pbro
https://hg.mozilla.org/integration/autoland/rev/926ea1a72380
Part 3: Isolated timeline. r=pbro
https://hg.mozilla.org/integration/autoland/rev/f0d775da0c7d
Part 4: Add animated property header. r=pbro
https://hg.mozilla.org/integration/autoland/rev/92bf2a07db90
Part 5: Add aniamted property's progress line tick. r=pbro
https://hg.mozilla.org/integration/autoland/rev/25f5eb7a8264
Part 6: Fixed animation detail panel. r=pbro
https://hg.mozilla.org/integration/autoland/rev/c981014be506
Part 7: Change selection logic. r=pbro
https://hg.mozilla.org/integration/autoland/rev/5dd5d0243a90
Part 8: Add selection color to selected animation element. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e6c7932be66b
Part 9: Add progress indicator. r=pbro
https://hg.mozilla.org/integration/autoland/rev/3a9fa00c5984
Part 10: Display animation's detail if the animation is only one in the list. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e8d79cd6b35b
Part 11: Disable clicking on keyframes because we can’t calculate correct current time since the animation detail handles the animation progress not time. r=pbro
https://hg.mozilla.org/integration/autoland/rev/235e77ab5191
Part 12: Unite common codes into utils.js and use. r=pbro
https://hg.mozilla.org/integration/autoland/rev/657c7b23a565
Part 13: Add tests. r=pbro
https://hg.mozilla.org/integration/autoland/rev/c54665e222d2
Part 14: Add close button. r=pbro
https://hg.mozilla.org/integration/autoland/rev/284a66e290fc
Part 15: Displays the animation detail if the previously displayed animation is included in timeline list after refresh all UI. r=pbro
https://hg.mozilla.org/integration/autoland/rev/5132225c2534
Part 16: Move unchanged properties to bottom in the list. r=pbro
Depends on: 1357993
Depends on: 1357526
Depends on: 1368371
I tested using Firefox 55.0b12 on Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.10 with the following test cases:
    1. use primalscreen-test.html from comment #212 transform: translate(-50%, 100%) -> transform: translateX(-50%)
    2. test2.html (comment #520), transform: translate(-50%, 100px) -> transform: translateX(-50%, 0%)
    3. http://www.primalscreen.com/ testing with this page the duplicated 0%/50%/100% header, this issue was fixed.
Are there any other test cases I could use? If possible, please give me more info about the expected results when testing this feature.
Flags: needinfo?(dakatsuka)
I'm so sorry, I don't have other test cases than DevTools tests.
https://reviewboard.mozilla.org/r/102154/diff/25#index_header
Flags: needinfo?(dakatsuka)
Hi Daisuke,
Doing this 3 test cases (comment #521) is enough to close the bug, marking flags as verified on Fx 55.0b12.
Thanks.
Flags: needinfo?(dakatsuka)
Hi Timera,
I'm sorry for larte.
I attached(attachment 8892259 [details]) sample html which contains cases that affect to the tool.

Properties:
 backgroundColor: should display as the color
 opacity: as pink graph.
 transform: as orange graph.
 textAlign: as blue graph. also the shape is like stairs since this animate discretely.
 width: as blue graph.

Direction:
 normal, reverse, alternate-reverse
 Please confirm progress indicator on the tool.
 If the direction is reverse, the indicator moves from right to left.

Easing:
 linear, ease-in, steps(2), frames(2)
 Please confirm the graph shape. e.g. steps(2) makes like stairs shape.

Thanks!
Flags: needinfo?(dakatsuka)
I can confirm that the test case expected results are the same as describe in comment #525, I verified using Firefox 55.0b12 on Win 8.1 x64, Mac OS X 10.10 and Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
Depends on: 1388113
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: