The animation inspector panel assumes these values don't change during the lifetime of an animation. This is an incorrect assumption. Whenever duration, delay or iteration count change, the UI of the player widget should be refreshed.

Note that the way we update things like currentTime in the devtools UI is by polling the AnimationPlayer object on the actor-side, so it's easy for the UI to know when the duration or delay change, but it would still be useful to have the webanimations API emit events when these values change. This way we could reduce our protocol traffic. Cameron: Do you think it would be possible to add events for when the animation-delay, or duration, or iterationCount, or keyframes change in an animation?

In this patch, I extracted the part that deals with the animation title, duration, delay, iteration count into its own UI component class, therefore making it easier to update when needed. As I said in a previous comment, since we already poll the AnimationPlayerActor for new state data, we are already being notified when any of these values change. So in this patch, I just use this to refresh the component when needed. I've updated the impacted tests and added a new one for this feature.

Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5699e52c493

Comment on attachment 8555243 [details] [diff] [review] bug1120899-extract-player-metadata-to-component v1.patch Review of attachment 8555243 [details] [diff] [review]: ----------------------------------------------------------------- Really minor stuff, but clearing the review so I can take another look at it before landing ::: browser/devtools/animationinspector/animation-panel.js @@ +186,5 @@ > "class": "player-widget " + state.playState > } > }); > > + this.metaDataComponent.render(this.el); Nit on naming: 'render' here implies to me that it's doing what the 'updateState' function does (and should be called multiple time any time the UI changed). I would probably call render -> createMarkup then call updateState -> render. @@ +418,5 @@ > + this.nameLabel = createNode({ > + parent: this.el, > + nodeType: "span" > + }); > + this.nameLabel.textContent = L10N.getStr("player.animationNameLabel"); Looks like this textContent shouldn't need to be set here. It's set in updateState and handling both animations/transitions there. ::: browser/devtools/animationinspector/test/browser_animation_playerWidgets_dont_show_time_after_duration.js @@ +34,5 @@ > > is(widget.currentTimeEl.value, front.state.duration, > "The timeline slider has the right value"); > is(widget.timeDisplayEl.textContent, > + getFormattedTime(front.state.duration) + "s", I guess it depends on what we want to test here, but I still think that getFormattedTime should be moved somewhere shared (like ViewHelpers). And then this test plus the animation-panel view can just use function. Or at least move this copy/pasted code into head.js so it's only duplicated in one place. ::: browser/devtools/animationinspector/test/browser_animation_playerWidgets_meta_data.js @@ +54,3 @@ > }); > + > +function isVisible(node) { Maybe move this isVisible function into head.js

Thanks Brian, here's an updated patch with changes according to your review comments. Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4581933ed5f (In reply to Brian Grinstead [:bgrins] from comment #4) > ::: browser/devtools/animationinspector/animation-panel.js > @@ +186,5 @@ > > "class": "player-widget " + state.playState > > } > > }); > > > > + this.metaDataComponent.render(this.el); > > Nit on naming: 'render' here implies to me that it's doing what the > 'updateState' function does (and should be called multiple time any time the > UI changed). I would probably call render -> createMarkup then call > updateState -> render. Done. > @@ +418,5 @@ > > + this.nameLabel = createNode({ > > + parent: this.el, > > + nodeType: "span" > > + }); > > + this.nameLabel.textContent = L10N.getStr("player.animationNameLabel"); > > Looks like this textContent shouldn't need to be set here. It's set in > updateState and handling both animations/transitions there. Good catch, changed. > ::: > browser/devtools/animationinspector/test/ > browser_animation_playerWidgets_dont_show_time_after_duration.js > @@ +34,5 @@ > > > > is(widget.currentTimeEl.value, front.state.duration, > > "The timeline slider has the right value"); > > is(widget.timeDisplayEl.textContent, > > + getFormattedTime(front.state.duration) + "s", > > I guess it depends on what we want to test here, but I still think that > getFormattedTime should be moved somewhere shared (like ViewHelpers). And > then this test plus the animation-panel view can just use function. > > Or at least move this copy/pasted code into head.js so it's only duplicated > in one place. So I ended up just using L10N.numberWithDecimals (from ViewHelpers) instead of my own time formatting utils, therefore avoiding any duplication. The reason why I didn't use it from the beggining is that when numbers don't have decimals, they aren't padded with 0s. So 5 will be returned as "5" and not "5.00". I wanted to always have padding, no matter what, to make sure numbers would not jump in the label element when time passes. But that isn't a big deal, it's better not have code duplication right now, especially that bug 1121879 is ongoing and will make what I wanted to do possible using numberWithDecimals. > ::: > browser/devtools/animationinspector/test/ > browser_animation_playerWidgets_meta_data.js > @@ +54,3 @@ > > }); > > + > > +function isVisible(node) { > > Maybe move this isVisible function into head.js Done.

Comment on attachment 8558428 [details] [diff] [review] bug1120899-extract-player-metadata-to-component v2.patch Review of attachment 8558428 [details] [diff] [review]: ----------------------------------------------------------------- Looks good ::: browser/devtools/animationinspector/animation-panel.js @@ +485,5 @@ > + }, > + > + destroy: function() { > + this.el.remove(); > + this.containerEl = this.el = null; this.containerEl is never set in the first place. Also, should we be nulling out this.state? Not sure if it's really necessary in this case.

Thanks Brian. Addressed the review comments in this new patch. I also found out that I forgot to change 2 function calls in animation-panel.js after I changed the function's signature in this patch. The effect wasn't immediately visible, and there was no specific test to avoid this from regressing, so I added a test for this. Try was green, so I'm pushing this to fx-team: https://hg.mozilla.org/integration/fx-team/rev/76666ba94d14

Comment on attachment 8570130 [details] [diff] [review] mozharness script for luciddream, WIP wrong bug, sorry

Patrick, can you please provide me more reliable STR in order to verify this issue?

(In reply to Mihai Boldan, QA [:mboldan] from comment #11) > Patrick, can you please provide me more reliable STR in order to verify this > issue? - Open http://bgrins.github.io/devtools-demos/inspector/animation-timing.html - Open the inspector and switch to the animation-inspector panel - Select #box1 in the inspector - You should see a single animation displayed in the timeline. It should be cut off at the end because it's an infinite animation. - Now, switch back to the "Rules" panel, and in the #box1 {...} rule, add: animation-iteration-count: 5; - Switch back to the animation panel - You should see that the animation displayed in the timeline has changed. It isn't cut off at the end anymore, but instead displays 5 segments (plus the initial delay which was there already). Also the tooltip should show 'repeats: 5'. You could also try with animation-delay, or animation-duration.

Using STR from Comment 12, I managed to verify this feature on Nightly 44.0a1 (2015-10-28) across platforms. While testing, I observed two potential issues: 1. STR: - Start FF with a clean profile; - Follow the STR from Comment 12; - Click on the Rewind button; - Click on the play button and wait until the animation is played the second time; - Go to Rules panel and change the no.5 from the 'animation-iteration-count: 5;' with 2. - Switch back to the animation panel AR: - The animation displayed in the timeline is not changed until the Play/Pause button is selected. - Note that if the STR from Comment 12 are followed on a clean profile, the animation displayed in the timeline is not changed until the Play/Pause button is selected. 2. STR: - Follow the STR from Comment 12; - Go to Rules panel and change the no.5 from the 'animation-iteration-count: 5;' with 8. - click on the time elapsed bar in order to select the scrubbed and move it back and forward. AR: The scrubber is not displayed. Patrick, should I file new bugs for this issues?

Yes can you please file a bug about this issue? At first sight, it looks like changing the duration, delay or iteration-count only works some of the times, and sort of randomly. Testing this a little more showed that: - if the animation is paused (either because you pressed the rewind button, or pause button while it was playing): then changing any of these things in the rule-view does refresh the animation-panel correctly - if the animation has ended (you just let it play until the end): then changing these things does *not* refresh the animation-panel. Pressing play/pause/rewind does force a refresh so you see the changes then. The fact that the scrubber is not displayed is related and should be fixed in the same bug.

I've logged Bug 1221088 for the issue found while testing this bug, so I'm marking this issue Verified Fix.