This basically involves changing the position of the vertical bars representing iterations.

The AnimationPlayerActor in devtools\server\actors\animation.js will need to also send the iterationStart information as part of its state. devtools\client\animationinspector\components\animation-time-block.js is the file responsible for generating the DOM for displaying the iterations. The vertical bars are actually achieved with a linear-gradient (see devtools\client\themes\animationinspector.css) /* Iterations of the animation are displayed with a repeating linear-gradient which size is dynamically changed from JS. The gradient only draws 1px borders between each iteration. These borders must have the same color as the border of this element */ background-image: linear-gradient(to right, var(--timeline-border-color) 0, var(--timeline-border-color) 1px, transparent 1px, transparent 2px); background-repeat: repeat-x; background-position: -1px 0; In Javascript, we do something like: background-size: (100 / (state.iterationCount || 1)) 100%; It seems to me like we can keep doing this but simply adjust the background-position to make the gradient start at the right offset, according to iterationStart.

Hello Patrick, Nice to meet you over bugzilla. Thank you very much for your advice! I implemented this bug, couldn't you review the code?

Comment on attachment 8728187 [details] [diff] [review] Part1: Show iterationStart to the tooltip Review of attachment 8728187 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly good, thanks for your help! I have a few comments about splitting the test, making it simpler, and maybe displaying both the ratio and the time in the tooltip. ::: devtools/client/animationinspector/components/animation-time-block.js @@ +129,5 @@ > > + // Adding the iteration start. > + if (state.iterationStart !== 0) { > + text += L10N.getStr("player.animationIterationStartLabel") + " "; > + text += state.iterationStart; I didn't know that previously, but iterationStart is a ratio of an iteration duration, not a time value in seconds (or ms). So it makes sense to display it this way, but, I think it would help people if we also displayed the actual time value. I think someone looking at the tooltip quickly, not knowing this new property of the waapi, might be confused, especially because there are other time unit values in this tooltip. So, assuming we have a duration of 2s and an iterationStart of 0.25, maybe we could display something like this instead: Iteration start: 0.25× (0.5s) The 0.5s part could come from getTime(state.iterationStart * state.duration) although getTime rounds numbers after 2 decimal places, so maybe we'd need to fix this. Doing this would require the player.animationIterationStartLabel string to be changed to something more complex like: player.animationIterationStartLabel=Iteration start: %1$S× (%2$Ss) And then, in JS: let iterationStartTime = state.iterationStart * state.duration / 1000; L10N.getFormatStr("player.animationIterationStartLabel", state.iterationStart, L10N.numberWithDecimals(iterationStartTime, 2); Keep in mind that the %1$S and %2$S string parameters need to be documented in animationinspector.properties too, so localizers know which is which, and can re-order if required in other languages. ::: devtools/client/animationinspector/test/browser_animation_timeline_shows_time_info.js @@ +29,5 @@ > ok(title.match(/Repeats: /), "The tooltip shows the iterations"); > } else { > ok(!title.match(/Repeats: /), "The tooltip doesn't show the iterations"); > } > + if (controller.animationPlayers[i].state.iterationStart !== 0) { There are no animations in doc_simple_animation.html that have an iterationStart value, right? So I think this branch of the IF is not needed. Keeping the ELSE seems fine. @@ +31,5 @@ > ok(!title.match(/Repeats: /), "The tooltip doesn't show the iterations"); > } > + if (controller.animationPlayers[i].state.iterationStart !== 0) { > + ok(title.match(/Iteration start: [\d.]+/), > + "The tooltip shows the iteration start"); nit: please indent with spaces only (2 space indentation). For info, here's our coding style guide: https://wiki.mozilla.org/DevTools/CodingStandards @@ +34,5 @@ > + ok(title.match(/Iteration start: [\d.]+/), > + "The tooltip shows the iteration start"); > + } else { > + ok(!title.match(/Iteration start:/), > + "The tooltip doesn't show the iteration start"); Same here. @@ +40,4 @@ > }); > }); > + > +add_task(function*() { Please move this new task to a new test file. We try and have one task per file usually, and keep files small and tests fast. Indeed, adding tabs, opening and closing the toolbox, initializing the tools, etc... takes time, and on slow machines it's not uncommon to go over the test timeout and therefore have intermittent test failures. Also, I think it's fine to make this test simpler/shorter. You could name it, for instance, browser_animation_timeline_iterationStart.js and make it only test iterationStart values in the tooltip. No need for it to again test the delay, duration, iterationCount, because the first test already does this. So you could only make it check that the iterationStart value appears in the tooltip when there is one defined. @@ +60,5 @@ > + ok(!title.match(/Repeats: /), "The tooltip doesn't show the iterations"); > + } > + if (controller.animationPlayers[i].state.iterationStart !== 0) { > + ok(title.match(/Iteration start: [\d.]+/), > + "The tooltip shows the iteration start"); nit about space vs. tab here as well @@ +63,5 @@ > + ok(title.match(/Iteration start: [\d.]+/), > + "The tooltip shows the iteration start"); > + } else { > + ok(!title.match(/Iteration start:/), > + "The tooltip doesn't show the iteration start"); nit about space vs. tab here as well ::: devtools/client/locales/en-US/animationinspector.properties @@ +53,5 @@ > player.infiniteIterationCountText=∞ > > +# LOCALIZATION NOTE (player.animationIterationStartLabel): > +# This string is displayed in each animation player widget. It is the label > +# displayed before the animation iterationStart. In fact, this label is used in a tooltip. So this comment needs to be updated to something like: This string is displayed in a tooltip that appears when hovering over animations in the timeline. It is the label displayed before the animation iterationStart value. Or something like this. I know that there are other strings in this file that have similar comments, but they should also be updated at some stage. ::: devtools/server/actors/animation.js @@ +192,5 @@ > return iterations === "Infinity" ? null : iterations; > }, > > /** > + * Get the animation iterationStart from this player. If I understand correctly, iterationStart isn't a time amount, it's a ratio of the length of one iteration. I think this comment should mention this. @@ +296,4 @@ > > if (hasCurrentAnimation(changedAnimations)) { > // Only consider the state has having changed if any of delay, duration > // or iterationcount has changed (for now at least). Maybe also update this comment to mention iterationStart too.

Comment on attachment 8728188 [details] [diff] [review] Part2: Show iterationStart to the GUI Review of attachment 8728188 [details] [diff] [review]: ----------------------------------------------------------------- This looks great too. Just need a few improvements here and there. ::: devtools/client/animationinspector/components/animation-time-block.js @@ +59,5 @@ > parent: this.containerEl, > attributes: { > "class": "iterations" + (state.iterationCount ? "" : " infinite"), > // Individual iterations are represented by setting the size of the > // repeating linear-gradient. Worth expanding this comment to also say that the background-position is offset horizontally to reflect the value of the iterationStart property. @@ +63,5 @@ > // repeating linear-gradient. > "style": `left:${x}%; > width:${iterationW}%; > + background-size:${100 / (state.iterationCount || 1)}% 100%; > + background-position:calc(${-iterationStartW}% - 1px) 0;` The -1px part feels hacky. I know it was here before in the CSS file, without a comment, but because you're touching this code, it's a good time to fix this. So, either you have a great idea on how to do this (I don't!!), or you add a comment saying that we offset by 1px the background-position to the left in order to hide the first iteration separator, because the animation already has a left border. ::: devtools/client/animationinspector/components/keyframes.js @@ +51,5 @@ > createNode({ > parent: this.keyframesEl, > attributes: { > "class": "frame", > + "style": `left:${frame.offset * 100 + iterationStartOffset}%;`, Not sure we need to go through the trouble of importing TimeScale in this module, and calling getAnimationDimensions, etc... Why don't we just do this here: let offset = frame.offset + (animation.state.iterationStart || 0); And then later: "style": `left:${offset * 100}%;`, ::: devtools/client/animationinspector/test/browser_animation_timeline_shows_iterations.js @@ +45,5 @@ > "The iteration element has the infinite class"); > }); > > +// Check animations by element.animate(). > +add_task(function*() { Same comment as in my previous review, I'd like this new part to be moved to another test file in the same directory. And made simple too, so it really only checks the background position offset.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #4) > I didn't know that previously, but iterationStart is a ratio of an iteration > duration, not a time value in seconds (or ms). > > So it makes sense to display it this way, but, I think it would help people > if we also displayed the actual time value. > I think someone looking at the tooltip quickly, not knowing this new > property of the waapi, might be confused, especially because there are other > time unit values in this tooltip. > > So, assuming we have a duration of 2s and an iterationStart of 0.25, maybe > we could display something like this instead: > > Iteration start: 0.25× (0.5s) > > The 0.5s part could come from > getTime(state.iterationStart * state.duration) > although getTime rounds numbers after 2 decimal places, so maybe we'd need > to fix this. Thanks for reviewing this Patrick. I wonder if the × is confusing here. It seems like you're saying multiply 0.5s by 0.25? Originally, I was unsure if adding 0.5s was a good idea or not since this value doesn't incorporate the playback rate, any easing etc. but I think it's probably fine, especially when put alongside the duration which is also not scaled by the playback rate.

Hi Patrick, Thank you for your review! I revised the codes that you pointed.

Comment on attachment 8728830 [details] [diff] [review] Part1: Show iterationStart to the tooltip Review of attachment 8728830 [details] [diff] [review]: ----------------------------------------------------------------- R+ with the following comments addressed. Thanks! ::: devtools/client/animationinspector/components/animation-time-block.js @@ +120,5 @@ > text += "\n"; > > // Adding the iteration count (the infinite symbol, or an integer). > if (state.iterationCount !== 1) { > + text += L10N.getStr("player.animationIterationCountLabel") + " "; There has been some indentation changes on this line and the next 3 lines. This seems like an unrelated change that should be reverted. ::: devtools/client/animationinspector/test/browser_animation_timeline_iterationStart.js @@ +3,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +requestLongerTimeout(2); This is a new test, let's drop the requestLongerTimeout(2) for now. We can add it later if it turns out that the test is slow (unless it already is too slow for you locally or on try?) ::: devtools/client/locales/en-US/animationinspector.properties @@ +66,5 @@ > > +# LOCALIZATION NOTE (player.animationIterationStartLabel): > +# This string is displayed in a tooltip that appears when hovering over > +# animations in the timeline. It is the label displayed before the animation > +# iterationStart value. Please document what the 2 variables are in this comment too. @@ +67,5 @@ > +# LOCALIZATION NOTE (player.animationIterationStartLabel): > +# This string is displayed in a tooltip that appears when hovering over > +# animations in the timeline. It is the label displayed before the animation > +# iterationStart value. > +player.animationIterationStartLabel=Iteration start: %1$S× (%2$Ss) I agree with Brian, the × sign might be misleading, let's get rid of it.

Thanks, Patrick! I revised.

Fix bitrot.

Comment on attachment 8729799 [details] [diff] [review] Part 1: Show iterationStart to the tooltip Review of attachment 8729799 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/animationinspector/test/doc_script_animation.html @@ +2,5 @@ > +<html lang="en"> > +<head> > + <meta charset="UTF-8"> > + <style> > + target { I missed this in my previous review, you should change 'target' to '#target', otherwise this rule won't apply.

Comment on attachment 8729800 [details] [diff] [review] Part 2: Show iterationStart to the GUI Review of attachment 8729800 [details] [diff] [review]: ----------------------------------------------------------------- nit: you should change r?birtles in your commit message to r=pbro. I found one weird thing, not sure how much of a problem this is: if you have iterationCount=1 and iterationStart=0.25, then the keyframes are positioned with an offset as expected, but there's no vertical bars in the time-block. There's one iteration displayed only and it starts at offset 0. ::: devtools/client/animationinspector/components/keyframes.js @@ +45,5 @@ > > + let iterationStartOffset = > + animation.state.iterationStart % 1 == 0 > + ? 0 > + : 1 - animation.state.iterationStart % 1; It looks to me like you don't need to do %1 in the else branch here again. ::: devtools/client/animationinspector/test/browser_animation_timeline_shows_iterations.js @@ +3,4 @@ > http://creativecommons.org/publicdomain/zero/1.0/ */ > > "use strict"; > There are now only whitespace changes in this file. You should revert them to keep this file unchanged.

Here is a screenshot to illustrate what I said in my last comment.

Thank you very much! Revised followings: 1. Add '#' in the style (comment 12) 2. Change commit message of part 1 (comment 13) 3. Revert browser_animation_timeline_shows_iterations.js (comment 13) 4. Fix iterationStart vertical line by iterationCount=1 and iterationStart=0.25

(In reply to Patrick Brosset [:pbro] from comment #13) > Comment on attachment 8729800 [details] [diff] [review] > Part 2: Show iterationStart to the GUI > > Review of attachment 8729800 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: devtools/client/animationinspector/components/keyframes.js > @@ +45,5 @@ > > > > + let iterationStartOffset = > > + animation.state.iterationStart % 1 == 0 > > + ? 0 > > + : 1 - animation.state.iterationStart % 1; > > It looks to me like you don't need to do %1 in the else branch here again. Value of iterationStart allows over 1 in the spec. ( e.g. 1.5 ) So, I think it needs to calculate the offset.

Comment on attachment 8731559 [details] [diff] [review] Part2: Show iterationStart to the GUI Review of attachment 8731559 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/animationinspector/components/animation-time-block.js @@ +54,5 @@ > // and its width is according to its duration (divided by the playbackrate). > let {x, iterationW, delayX, delayW, negativeDelayW} = > TimeScale.getAnimationDimensions(animation); > > + // Represent iterationStart by background-size and background-position. This is a pretty smart solution! Seems to work really well in my tests. 2 comments: - can you move this whole block to another function, so it's easier to read? Something called |getIterationsBackgroundData| or something like this, that basically just takes the state as a parameter and returns the backgroundIterations object. - even if this works really well, the way we display iterations using a css gradient is hackish, and sort of hard to understand for people reading this code, so at some stage, we should move away from this maybe use a svg path, or canvas, ... anyway, not in this bug. ::: devtools/client/animationinspector/test/browser_animation_timeline_iterationStart.js @@ +3,5 @@ > http://creativecommons.org/publicdomain/zero/1.0/ */ > > "use strict"; > > // Check iteration start. I have a few comments about this test, all related to making it easier to read. I think this is an important goal because we often spend time reading tests when they start to fail intermittently. - This top comment should describe in more details what this test exactly does. - There should be more info("...") throughout the test to help when reading logs. - There are a couple of eslint errors in the test. - The overall logic of the test should be more easy to read in add_task, by moving out a lot of code in separate functions so that you can very quickly read the steps that the test goes over and then dig in to other functions for more info. Because these are very high level comments and I would like to R+ this patch now, I'm going to attach a diff of the changes I mean for this test and let you choose to use it or file a follow-up bug to do it later).

(In reply to Patrick Brosset [:pbro] from comment #19) > Comment on attachment 8731559 [details] [diff] [review] > Part2: Show iterationStart to the GUI > > Review of attachment 8731559 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/animationinspector/components/animation-time-block.js > @@ +54,5 @@ > > // and its width is according to its duration (divided by the playbackrate). > > let {x, iterationW, delayX, delayW, negativeDelayW} = > > TimeScale.getAnimationDimensions(animation); > > > > + // Represent iterationStart by background-size and background-position. > > This is a pretty smart solution! > Seems to work really well in my tests. Thank you very much!! > 2 comments: > - can you move this whole block to another function, so it's easier to read? > Something called |getIterationsBackgroundData| or something like this, that > basically just takes the state as a parameter and returns the > backgroundIterations object. OK! I'll implement as getIterationsBackgroundData method in TimeScale. > - even if this works really well, the way we display iterations using a css > gradient is hackish, and sort of hard to understand for people reading this > code, so at some stage, we should move away from this maybe use a svg path, > or canvas, ... anyway, not in this bug. I see. I may be able to fix after this bug. Let me discuss with Brian. > devtools/client/animationinspector/test/ > browser_animation_timeline_iterationStart.js > @@ +3,5 @@ > > http://creativecommons.org/publicdomain/zero/1.0/ */ > > > > "use strict"; > > > > // Check iteration start. > > I have a few comments about this test, all related to making it easier to > read. > I think this is an important goal because we often spend time reading tests > when they start to fail intermittently. > > - This top comment should describe in more details what this test exactly > does. > - There should be more info("...") throughout the test to help when reading > logs. > - There are a couple of eslint errors in the test. > - The overall logic of the test should be more easy to read in add_task, by > moving out a lot of code in separate functions so that you can very quickly > read the steps that the test goes over and then dig in to other functions > for more info. > > Because these are very high level comments and I would like to R+ this patch > now, I'm going to attach a diff of the changes I mean for this test and let > you choose to use it or file a follow-up bug to do it later). Thank you very very much!!! I'll use your diff!

Revised followings: 1. Make getIterationsBackgroundData method 2. Use Patrick's test diff

The Try push has real-looking ESLint failures. https://treeherder.mozilla.org/logviewer.html#?job_id=18382034&repo=try

Hello Ryan, Thank you for your comment! I revised those files.

Comment on attachment 8733698 [details] [diff] [review] Part1: Show iterationStart to the tooltip I'm not a peer for this code.

At the link provided by Ryan in comment 24, the following ESLint errors can be found: browser_animation_timeline_shows_time_info.js:36:2 | Newline required at end of file but not found. (eol-last) doc_script_animation.html:37:56 | Strings must use doublequote. (quotes) doc_script_animation.html:44:56 | Strings must use doublequote. (quotes) doc_script_animation.html:51:56 | Strings must use doublequote. (quotes) utils.js:329:29 | Unexpected space after unary operator "-". (space-unary-ops) So these are new ESLint rules violations introduced by the patches. I took a quick look at the new patches, they seem to fix these errors, but it's kind of hard to see what has changed here on splinter (and the interdiff on one of the patches is failing too). So I'm going to assume that only ESLint-related changes were made and R+ these patches. Now, I strongly suggest that you run ESLint locally (or on try) before re-landing those, to avoid a backout. It's really easy to do this locally with |mach eslint| or even without a text editor like sublimetext, atom, emacs, ... or even on try. Find out more information here: https://wiki.mozilla.org/DevTools/CodingStandards

Comment on attachment 8733698 [details] [diff] [review] Part1: Show iterationStart to the tooltip Review of attachment 8733698 [details] [diff] [review]: ----------------------------------------------------------------- R+ assuming only ESLint changes were made here and the ESLint job passes on try.

Comment on attachment 8733699 [details] [diff] [review] Part2: Show iterationStart to the GUI Review of attachment 8733699 [details] [diff] [review]: ----------------------------------------------------------------- R+ assuming only ESLint changes were made here and the ESLint job passes on try.

(In reply to Patrick Brosset [:pbro] from comment #29) > At the link provided by Ryan in comment 24, the following ESLint errors can > be found: > > browser_animation_timeline_shows_time_info.js:36:2 | Newline required at end > of file but not found. (eol-last) > doc_script_animation.html:37:56 | Strings must use doublequote. (quotes) > doc_script_animation.html:44:56 | Strings must use doublequote. (quotes) > doc_script_animation.html:51:56 | Strings must use doublequote. (quotes) > utils.js:329:29 | Unexpected space after unary operator "-". > (space-unary-ops) > > So these are new ESLint rules violations introduced by the patches. > > I took a quick look at the new patches, they seem to fix these errors, but > it's kind of hard to see what has changed here on splinter (and the > interdiff on one of the patches is failing too). > So I'm going to assume that only ESLint-related changes were made and R+ > these patches. > > Now, I strongly suggest that you run ESLint locally (or on try) before > re-landing those, to avoid a backout. It's really easy to do this locally > with |mach eslint| or even without a text editor like sublimetext, atom, > emacs, ... or even on try. > Find out more information here: > https://wiki.mozilla.org/DevTools/CodingStandards Thank you for your advice! I'm sorry, I did not know ESLint test.. Now, I installed. Also the latest patches that you reviewed passed ESLint test at local.

> Now, I installed. > Also the latest patches that you reviewed passed ESLint test at local. Also try server. https://treeherder.mozilla.org/#/jobs?repo=try&revision=74e62239d2bc

https://hg.mozilla.org/integration/fx-team/rev/a296e3d1019d https://hg.mozilla.org/integration/fx-team/rev/8a4790198087

https://hg.mozilla.org/mozilla-central/rev/a296e3d1019d https://hg.mozilla.org/mozilla-central/rev/8a4790198087

Comment on attachment 8733698 [details] [diff] [review] Part1: Show iterationStart to the tooltip Review of attachment 8733698 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/locales/en-US/animationinspector.properties @@ +68,5 @@ > +# This string is displayed in a tooltip that appears when hovering over > +# animations in the timeline. It is the label displayed before the animation > +# iterationStart value. > +# %1$S will be replaced by the original itaration start value > +# %2$S will be replaced by the actual time of itaration start Passing by note for next time someone touches this file: typo (twice) in the comment (itAration).

A related issue: 338 border: 1px solid var(--timeline-border-color); 339 /* Border-right is already handled by the gradient */ 340 border-width: 1px 0 1px 1px; 341 342 /* The background color is set independently */ 343 background-color: var(--timeline-background-color); 344 } 345 346 .animation-timeline .animation .iterations.infinite { 347 border-right-width: 0; 348 } 349 border-right-width is already zero in the first block, so the .infinite block is not needed?

(In reply to Francesco Lodolo [:flod] from comment #36) > Comment on attachment 8733698 [details] [diff] [review] > Part1: Show iterationStart to the tooltip > > Review of attachment 8733698 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/locales/en-US/animationinspector.properties > @@ +68,5 @@ > > +# This string is displayed in a tooltip that appears when hovering over > > +# animations in the timeline. It is the label displayed before the animation > > +# iterationStart value. > > +# %1$S will be replaced by the original itaration start value > > +# %2$S will be replaced by the actual time of itaration start > > Passing by note for next time someone touches this file: typo (twice) in the > comment (itAration). Hello Francesco, Oh, I'm so sorry,, and thank you for your check!

(In reply to Alfred Kayser from comment #37) > A related issue: > 338 border: 1px solid var(--timeline-border-color); > 339 /* Border-right is already handled by the gradient */ > 340 border-width: 1px 0 1px 1px; > 341 > 342 /* The background color is set independently */ > 343 background-color: var(--timeline-background-color); > 344 } > 345 > 346 .animation-timeline .animation .iterations.infinite { > 347 border-right-width: 0; > 348 } > 349 > > border-right-width is already zero in the first block, so the .infinite > block is not needed? Hello Alfred, Thank you for your feedback! Yes, I think so. I tried to test without this property at my local environment, it seems to work well. I'll remove this property in another bug.