When animations end in the timeline, make sure the time-label shows the right time

RESOLVED FIXED in Firefox 47

Status

RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: pbro, Assigned: nchevobbe, Mentored)

Tracking

unspecified
Firefox 47
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
When an animation ends, the time label stops as well. But the final time is different for each playback.

http://i.imgur.com/yBCaYAs.jpg
http://i.imgur.com/CCvBsc2.jpg
http://i.imgur.com/Xe2mOk2.jpg

The time label updates using a requestAnimationFrame loop on the client-side. At each iteration of the loop, we detect whether the animations are done or not. If they are, we stop the scrubber and time label, but this could be a few millis after the actual end. We should make sure we position the scrubber and time label exactly at the right end time.

This loop is in \devtools\client\animationinspector\components.js in startAnimatingScrubber()
(Reporter)

Updated

3 years ago
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
(Assignee)

Comment 1

3 years ago
I'd be happy to work on this one. Can someone assign it to me please ? Thank you
(Reporter)

Updated

3 years ago
Assignee: nobody → chevobbe.nicolas
(Assignee)

Comment 2

3 years ago
Created attachment 8713353 [details] [diff] [review]
Bug_1219611.patch

Hi !
I think it's good like this.
I removed a test in displayTimelineCurrentTime, which prevented the time-label to show the animation end time ( because the animation was not paused nor infinite ). I think it was here to prevent the time-label to show a number higher than the animation length, but I'm not sure of this.

The tests all PASS, so I guess it's fine.

Let me know if something is wrong, thank you.
Attachment #8713353 - Flags: review?(pbrosset)
(Reporter)

Comment 3

3 years ago
Comment on attachment 8713353 [details] [diff] [review]
Bug_1219611.patch

Review of attachment 8713353 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: devtools/client/animationinspector/components/animation-timeline.js
@@ +356,5 @@
>      return this.animations.some(({state}) => !state.iterationCount);
>    },
>  
>    startAnimatingScrubber: function(time) {
>      let x = TimeScale.startTimeToDistance(time);

Maybe move this variable declaration just before the if where it's used, for better packing.

startAnimationScrubber: function(time) {
  let isOutOfBounds....
  let isAllPaused...
  let hasInfinite...

  let x = TimeSc....
  if (x > 100...
}
Attachment #8713353 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 4

3 years ago
Created attachment 8713589 [details] [diff] [review]
Bug_1219611.patch

Edited to match Comment 3
Attachment #8713353 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Pushed to TRY : https://treeherder.mozilla.org/#/jobs?repo=try&revision=3288d4e4e1c4 

I don't know if I did it well though. 
On my bookmark for the bug ( where I have my commit for this patch ), I did : 

> hg qnew patch.try
> hg qref --message "try: -b do -p linux64,macosx64,win64,eslint-gecko -u xpcshell,mochitest-dt,mochitest-o,mochitest-e10s-dt -t none"
> hg push --bookmark timelabel_1219611 -f try

I don't know if I should restrict the push to my bookmark only.
(Assignee)

Comment 6

3 years ago
The job is over. 
There is one test that failed ( only on  Linux x64 opt ) : 

> TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | swatch changed colors - Got rgb(255, 0, 153), expected rgb(255, 255, 85) 

But I don't think that it's because of my patch though.
(Reporter)

Comment 7

3 years ago
(In reply to Nicolas Chevobbe from comment #5)
> Pushed to TRY :
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3288d4e4e1c4 
> 
> I don't know if I did it well though. 
> On my bookmark for the bug ( where I have my commit for this patch ), I did
> : 
> 
> > hg qnew patch.try
> > hg qref --message "try: -b do -p linux64,macosx64,win64,eslint-gecko -u xpcshell,mochitest-dt,mochitest-o,mochitest-e10s-dt -t none"
> > hg push --bookmark timelabel_1219611 -f try
> 
> I don't know if I should restrict the push to my bookmark only.
Looks like whatever you did worked! I don't think the --bookmark part was required anyway.
When you push to try, you *force* push (-f) with your current commit being the one with the try message. This will push all the commits that try hasn't seen before. So if you updated recently, you might be pushing commits that just landed on fx-team and that try hasn't seen before.
That's fine (there's one in your try push for instance: 8be8932a2e0f).

(In reply to Nicolas Chevobbe from comment #6)
> The job is over. 
> There is one test that failed ( only on  Linux x64 opt ) : 
> 
> > TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | swatch changed colors - Got rgb(255, 0, 153), expected rgb(255, 255, 85) 
> 
> But I don't think that it's because of my patch though.
Yup, seems unrelated, and there's already an intermittent failure bug opened to track this: bug 1244270.
(Assignee)

Comment 9

3 years ago
Great !
May I ask you if you are willing to vouch for me to be a Mozillian (  https://mozillians.org/en-US/u/chevobbe.nicolas/  ) or should I contribute more to consider it ?

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af13bfd0725a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1214664

Updated

5 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.