1465077 - Negative elapsed time and scrubber position are displayed, if an animation is replayed

Status: VERIFIED FIXED

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

Description

[Gyula Palko[:gpalko]]

Attachment: resumed_seeker.PNG

[Environments:]
Windows 10, Windows 7, Ubuntu 16.04, OsX10.12
62.0a1 20180528220216

[Steps:]
1. Open Firefox and open a new tab with https://rawgit.com/dadaa/3b73f847427025b51ba1ab7333013d0c/raw/77f3f0bb884875a179c3407f73bf8a8dd54751c9/doc_simple_animation.html
2. Press F12.
3. Select Inspector and Animations panel.
4. Select "ball delayed" node in Inspector
5. Switch to 10x playback rate
6. Wait for the animation to be finished
7. On Animations panel click the Play/Resume button

[Actual Result:]
The animation is replayed, negative elapsed time is displayed, the scrubber starts according to the negative time(see attached screenshot)

[Expected Result:]
6. The animation should run again from 0.0s

Comment 1

[Gyula Palko[:gpalko]]

The issue is not reproducing in the old animation inspector.

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1465077 - Part 1: Introduce doSetCurrentTimes so as to not forget the createdTime.

Review commit: https://reviewboard.mozilla.org/r/247856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247856/

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1465077 - Part 2: Add a test which checks whether the scrubber never be located at negative position.

Review commit: https://reviewboard.mozilla.org/r/247858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247858/

Comment 4

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a6f8fff9c0a6ee4b63ba416ad7094f256ff5405

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8981803 [details]
Bug 1465077 - Part 1: Introduce doSetCurrentTimes so as to not forget the createdTime.

https://reviewboard.mozilla.org/r/247856/#review254402

::: devtools/client/inspector/animation/animation.js:196
(Diff revision 1)
> +   *
> +   * @param {Number} currentTime
> +   * @param {Array} animations
> +   * @param {Object} timeScale
> +   */
> +  async callSetCurrentTimes(currentTime, animations, timeScale) {

`animations` and `timeScale` can be retrieved from `this.state`, right? So I think there is no need to pass them as arguments here.

Also, I'm not sure about the method name you've chosen here. What about `doSetCurrentTime` instead?
It's like, it's the low-level call while `setCurrentTime` is higher level API.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8981804 [details]
Bug 1465077 - Part 2: Add a test which checks whether the scrubber never be located at negative position.

https://reviewboard.mozilla.org/r/247858/#review254424

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8981803 [details]
Bug 1465077 - Part 1: Introduce doSetCurrentTimes so as to not forget the createdTime.

https://reviewboard.mozilla.org/r/247856/#review254402

> `animations` and `timeScale` can be retrieved from `this.state`, right? So I think there is no need to pass them as arguments here.
> 
> Also, I'm not sure about the method name you've chosen here. What about `doSetCurrentTime` instead?
> It's like, it's the low-level call while `setCurrentTime` is higher level API.

Thanks!

I thought that this function might be called with specific animations. Due to this reason, I made the animations to be the parameter. However, as you said, because there is no need actually in this case, retrieves from `this.state`.

Comment 8

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8981803 [details]
Bug 1465077 - Part 1: Introduce doSetCurrentTimes so as to not forget the createdTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247856/diff/1-2/

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8981804 [details]
Bug 1465077 - Part 2: Add a test which checks whether the scrubber never be located at negative position.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247858/diff/1-2/

Comment 10

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2484c16d17fc448fa59f15ef16b7598849b9d0ac

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8981803 [details]
Bug 1465077 - Part 1: Introduce doSetCurrentTimes so as to not forget the createdTime.

https://reviewboard.mozilla.org/r/247856/#review255056

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3de98e58e52
Part 1: Introduce doSetCurrentTimes so as to not forget the createdTime. r=pbro
https://hg.mozilla.org/integration/autoland/rev/8533951dca06
Part 2: Add a test which checks whether the scrubber never be located at negative position. r=pbro

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f3de98e58e52
https://hg.mozilla.org/mozilla-central/rev/8533951dca06

Comment 14

[Gyula Palko[:gpalko]]

Verified as fixed on Nightly 62.0a1(20180611100116) on Win10 x64, Ubuntu 12.04 and MacOS 10.12
Animations are reloaded from 0.0s.