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.

Bug triage (filter on CLIMBING SHOES).

I added some comments to bug 1210795 which overlap with this significantly.

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)

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.

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.)

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.

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.

We changed properties color. * opacity: pink * transform: orange * other: gray

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()?

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.

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.

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

(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.

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?

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 }); } ```

(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?

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

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.

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}`; ```

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.

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.

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.

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.

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?

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.

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 }; ```

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

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.

Comment on attachment 8823603 [details] Bug 1210796 - Part 8: Add selection color to selected animation element. https://reviewboard.mozilla.org/r/102148/#review114864

Thank you for your kindly review, Patrick. I'll fix according to your advice. Thanks!

(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.

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.

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

(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

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

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.

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

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

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.

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?