1122437 - Polish the player widget in the animation inspector panel

Status: VERIFIED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Now that the first version of the animation panel has landed in bug 1105825, I've noticed a few UI problems we should fix and a few changes we could make to overall polish the UI and its behavior.

I've got 4 small things:

- When an animation ends, the player should remain on the screen but be disabled, this is better than making it disappear automatically. It avoids the UI to "jump". Also, in the future, the web animations API will allow us to re-run an ended animation, so we'll need to keep the widget around for this anyway.

- When you pause an animation, then switch to another node and then back to the same one again, the state of the timeline widget is incorrect.

- When an animation ends, the time label continues to be updated until the next synchronization from the actor, that means that for instance, even if the animation has a duration of 1 second, the current time label may show 1.02s for instance.

- Animations that don't repeat (iterationCount=1) shouldn't need to display "repeats:1" in the UI.

Each of these 4 things are very minor fixes, a few lines at most, and they're all about polishing the UI, so I didn't file separate bugs for each of them.

I will create separate patches though.

Comment 1

[Patrick Brosset <:pbro>]

I'm sorry in advance Mike, but I'm going to drop 4 patches for you to review :)
These are small patches though.

Let me know if you need any more background info on the animation panel/actor before reviewing.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug1122437-1-disable-finished-animation-players v1.patch

Disable animation player widgets when the corresponding animation ends.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug1122437-2-fix-initial-widget-time-display v1.patch

Make sure the currentTime display is correct when a paused animation node is selected.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug1122437-3-prevent-local-rAF-loop-to-show-time-past-duration v1.patch

Make sure that when an animation ends and its corresponding widget is currently shown, the timeline in the widget and currentTime label stop too, at the defined duration*iteration.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1122437-4-hide-iterationcount-display-if-one v1.patch

Hide "repeats:1" when iterationCount is 1.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug1122437-3-prevent-local-rAF-loop-to-show-time-past-duration v2.patch

Fixed a problem in part 3 whereby the end time wasn't displayed correctly when the animation ended.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: bug1122437-4-hide-iterationcount-display-if-one v2.patch

Changed the test for part 4 a little bit so it wouldn't intermittently fail on try.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug1122437-3-prevent-local-rAF-loop-to-show-time-past-duration v3.patch

Getting rid of a potential intermittent that part 3 would introduce.
The problem is that if the test platform is slow, the test keyframes animation may already be finished by the time the test really starts. So I've changed the test to instead apply a css class at the right time to start the animation.

Comment 9

[Patrick Brosset <:pbro>]

Attachment: bug1122437-1-disable-finished-animation-players v2.patch

Ah, the test introduced in part 1 can also fail intermittently because the css animation used in the test may end before the animation panel is ready.
Here's a change that should take care of this.

And a pending try push for all patches here (note that the patches from bug 1122435 were also applied before, as I intend to land them first):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59afd503cdc1

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #1)
> I'm sorry in advance Mike, but I'm going to drop 4 patches for you to review
> :)
> These are small patches though.
> 
> Let me know if you need any more background info on the animation
> panel/actor before reviewing.

No problem... I have been working in coffee shops all week due to building work in my house. I'll ping you if I need to know anything.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8552641 [details] [diff] [review]
bug1122437-3-prevent-local-rAF-loop-to-show-time-past-duration v3.patch

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

::: browser/devtools/animationinspector/animation-panel.js
@@ +404,5 @@
>        time = Math.max(0, time - state.delay);
>      }
>  
> +    // For finite animations, make sure the displayed time does not go beyond
> +    // the animation total duration (may happen due to the local rAF loop).

nit: Obvious now I know what rAF is but comments should be obvious at a glance. Can you change this to requestAnimationFrame?

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug1122437-3-prevent-local-rAF-loop-to-show-time-past-duration v4.patch

Updated the comment as per Mike's review.

Comment 13

[Patrick Brosset <:pbro>]

Landed on fx-team:

remote:   https://hg.mozilla.org/integration/fx-team/rev/6d82f9285191
remote:   https://hg.mozilla.org/integration/fx-team/rev/d8c9dc4cf18f
remote:   https://hg.mozilla.org/integration/fx-team/rev/1f0224c652c6
remote:   https://hg.mozilla.org/integration/fx-team/rev/2ad51338fdc5

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6d82f9285191
https://hg.mozilla.org/mozilla-central/rev/d8c9dc4cf18f
https://hg.mozilla.org/mozilla-central/rev/1f0224c652c6
https://hg.mozilla.org/mozilla-central/rev/2ad51338fdc5

Comment 15

[Vasilica Tamas, QA [:vtamas]]

I am aware that this bug has been fixed for a long time and I’m not sure which parts still apply . However, based on Description, I will describe in a few words which is the actual result under Firefox 44.0a1 (2015-10-25).

[1] When an animation ends, the animation remains displayed on the screen.

[2] When you pause an animation, then switch to another node and then back to the same one again, the animation graphs are changed and the red scrubber is not displayed.
Before: http://i.imgur.com/lcAteXd.jpg
After: http://i.imgur.com/O1byWK3.jpg

[3] When an animation ends, the time label stops as well. But noticed that 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


[4] > - Animations that don't repeat (iterationCount=1) shouldn't need to display
    > "repeats:1" in the UI.

It is not displayed a repeat counter in Animation panel.


Patrick, any thoughts about this issues?

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #15)
> [1] When an animation ends, the animation remains displayed on the screen.
What do you mean exactly? If there is an animation displayed in the tool and the animation ends, then yes, it does remain visible in the tool, but that's on purpose. We want to make it possible for people to be able to inspect animations even when they are finished (scrub backwards, replay, ...).

> [2] When you pause an animation, then switch to another node and then back
> to the same one again, the animation graphs are changed and the red scrubber
> is not displayed.
> Before: http://i.imgur.com/lcAteXd.jpg
> After: http://i.imgur.com/O1byWK3.jpg
Filed bug 1219614.

> [3] When an animation ends, the time label stops as well. But noticed that
> 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
That's right indeed. 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.
Filed bug 1219611.

> [4] > - Animations that don't repeat (iterationCount=1) shouldn't need to
> display
>     > "repeats:1" in the UI.
So, this information is displayed in a tooltip when you hover over an animation in the panel. And you're right, no need to display it when there's no repeat. I filed bug 1219608 for this.

Comment 17

[Vasilica Tamas, QA [:vtamas]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #16)
> (In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #15)
> > [1] When an animation ends, the animation remains displayed on the screen.
> What do you mean exactly? If there is an animation displayed in the tool and
> the animation ends, then yes, it does remain visible in the tool, but that's
> on purpose. We want to make it possible for people to be able to inspect
> animations even when they are finished (scrub backwards, replay, ...).

I meant that the expected result of this part is accomplished.


> > [4] > - Animations that don't repeat (iterationCount=1) shouldn't need to
> > display
> >     > "repeats:1" in the UI.
> So, this information is displayed in a tooltip when you hover over an
> animation in the panel. And you're right, no need to display it when there's
> no repeat. I filed bug 1219608 for this.

I confirm that the repeat counter is displayed in a tooltip, while hovering over an animation.


Based on Comment 15 and Comment 16, I am marking this bug as Verified since the other issues are tracked separately.