Closed Bug 1261561 Opened 4 years ago Closed 4 years ago

endDelay incorrectly positioned for animations with multiple iterations

Categories

(DevTools :: Inspector: Animations, defect, P1)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: pbro, Assigned: r_kato, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js][btpp-fix-now])

Attachments

(3 files)

STR:
- create an animation that has more than 1 iteration
- apply an endDelay to it
- visualize the animation in the animation-inspector

The endDelay is placed at the end of the first iteration, instead of at the end of the animation.

See screenshot.
Blocks: 1253494
I think this should be a very simple fix.
Here's the code where we calculate the start x position for the endDelay:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/utils.js#326
it doesn't seem to take the number of iterations. If instead of doing |x + w| we did  |x + iterationW| I think that would solve it.

Marking as a P1 because we need to get this fixed before bug 1253494 ships out, but since it's a simple fix, I'm not assigning it to me just now and marking myself as mentor instead.

Please feel free to pick this up, by first going through our contribution guide: https://developer.mozilla.org/en-US/docs/Tools/Contributing

If no one takes it before some time next week, I'll do.
Mentor: pbrosset
Priority: -- → P1
Whiteboard: [good first bug][lang=js][btpp-fix-now]
I hear about this from :hiro, so I'd like to work on!

Could you assign this bug to me...?
Assignee: nobody → ryo_kato
Status: NEW → ASSIGNED
I tried writing tests for getAnimationDimensions() in test/unit/test_timeScale.js, but that didn't work well... Could you tell me where to write tests?
(In reply to Ryo Kato [:r_kato] from comment #4)
> I tried writing tests for getAnimationDimensions() in
> test/unit/test_timeScale.js, but that didn't work well... Could you tell me
> where to write tests?
Yes, this is the right approach for the test. However test_timeScale.js is already quite a big test. I would advise creating another xpcshell test in the same folder. Maybe test_timeScale_dimensions.js and use it to test the output of the getAnimationDimensions method specifically.
What exactly did you mean by "that didn't work well"? Could you give more details so I can help you better?

Also, are you planning to push a separate patch for the test, or amend the first one? 
If a separate patch, then I guess I can review the first one right away.
(In reply to Patrick Brosset [:pbro] from comment #5)
> I would advise creating another xpcshell test in the same folder.
> Maybe test_timeScale_dimensions.js and use it to test the output
> of the getAnimationDimensions method specifically.

> Also, are you planning to push a separate patch for the test, or
> amend the first one? 

Thank you for advice! I will write a separate test as "Part 2:" patch,
so I won't amend "Part 1:" patch.
Comment on attachment 8737823 [details]
MozReview Request: Bug 1261561 - Part 1: Fix TimeScale.getAnimationDimensions() to handle multiple iterations r?pbro

https://reviewboard.mozilla.org/r/44135/#review40937

Thanks. This looks good.
Attachment #8737823 - Flags: review?(pbrosset) → review+
Comment on attachment 8738121 [details]
MozReview Request: Bug 1261561 - Part 2: Add a xpcshell test for TimeScale.getAnimationDimensions() r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44339/diff/1-2/
Attachment #8738121 - Flags: review?(pbrosset) → review+
Comment on attachment 8738121 [details]
MozReview Request: Bug 1261561 - Part 2: Add a xpcshell test for TimeScale.getAnimationDimensions() r=pbro

https://reviewboard.mozilla.org/r/44339/#review41491

::: devtools/client/animationinspector/test/unit/test_timeScale_dimensions.js:37
(Diff revision 2)
> +  }],
> +  expectedEndDelayX: 90
> +}];
> +
> +function run_test() {
> +  do_print("Test calcurating endDelayX");

s/calcurating/calculating
Comment on attachment 8738121 [details]
MozReview Request: Bug 1261561 - Part 2: Add a xpcshell test for TimeScale.getAnimationDimensions() r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44339/diff/2-3/
Attachment #8738121 - Attachment description: MozReview Request: Bug 1261561 - Part 2: Add a xpcshell test for TimeScale.getAnimationDimensions() r?pbro → MozReview Request: Bug 1261561 - Part 2: Add a xpcshell test for TimeScale.getAnimationDimensions() r=pbro
https://reviewboard.mozilla.org/r/44339/#review41491

> s/calcurating/calculating

Thank you for reviewing! I've just fixed that typo.
I did push patches here to try server on behalf of r_kato.  There were two tries since I did forget to run xpcshell tests first.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1edef265486
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f59524e2fd66
The result seem good.  Adding checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6c1e3c4a7f49
https://hg.mozilla.org/mozilla-central/rev/21a9c5fb6e0c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.