Closed Bug 1468475 Opened 6 years ago Closed 6 years ago

Negative elapsed time and scrubber position are displayed, if an animation is rewound

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 wontfix, firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: gpalko, Assigned: mantaroh)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(5 files, 15 obsolete files)

afterRewind.PNG
53.74 KB, image/png
Details
screen.mp4
1.06 MB, video/mp4
Details
Bug 1468475 - Part 1. Introduce zeroPositionTime in order to shift the graduation if animations have negative-delay.
59 bytes, text/x-review-board-request
daisuke
: review+
Details
Bug 1468475 - Part 2. Display the zero graduation.
59 bytes, text/x-review-board-request
daisuke
: review+
Details
Bug 1468475 - Part 3. Add negative current time tests.
59 bytes, text/x-review-board-request
daisuke
: review+
Details
Attached image afterRewind.PNG — 
[Environments:]
Windows 10, Windows 7, Ubuntu 16.04, OsX10.12
62.0a1 20180612220131

[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 <body> and rewind the animations
5. Select a node(e.g. ball long)
6. Inspect the playback time and the animation scrubber

[Actual Result:]
Negative elapsed time is displayed(-10.000s), the scrubber starts according to the negative time(see attached screenshot)

[Expected Result:]
6. The scrubber should be displayed at 0.0s

NOTE: This bug is a regression; After running mozregression, this changelog is pointed out: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=35aa0dde259f5f51c0aaf86935a54b8087c2e8c6&tochange=89d79c2258be1c420153e1adc54338b0165fc88e
Product: Firefox → DevTools
Priority: -- → P3
This cause is the negative-delay animation. This STR's site animations start on currentTime = 10000(10s), because, this site has a negative delay animation and this negative delay is '-10000' (-10s).  As result of it, the zero current time is same to -10s for other animations.

I disucssed with Daisuke, we might need to start from negative value instead of zero when animations has a negative-delay. (i.e.,  we draw the graph from negative start position.)
I try to this bug.

This WIP Code. This patch introduced "zeroBaseTime" into timeScale. timeScale will store the time which should display as zero into this variable. However,  time of animation ispector will need to start zero not negative value in order to make the  calculation of time to be simple. So this "zeroBaseTime" is just for display.

Furthermore, this patch will set the current time to "zeroBaseTime" when rewind the animation. If we set the curren time to zero, some animation's current time might to be negative value.

I don't implement the following yet:
 * Should be display "0s" always. - If inspector has negative-delay animation, header of timeline might not have the "0s" label. 

Ttry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=473c099e3935bdfb676963cbd4cabdc4e7a715c7
Assignee: nobody → mantaroh
Attached video screen.mp4 (obsolete) — 
This attachment is the behavior of wip patch.

The graph draws negative time if animations have a negative-delay. And rewinding position will be zero, not negative time.

However, this patch doesn't ensure displaying the zero position. If animations have a negative-delay like '-9s', the graduation start from '-9s' and press the graduation according to the calculated interval time. In this case, the graph might not have the '0s' graduation since the current code will not consider shifting the graph.
Attachment #8989961 - Attachment is obsolete: true
Attachment #8989961 - Flags: review?(dakatsuka)
Attachment #8989962 - Attachment is obsolete: true
Attachment #8989962 - Flags: review?(dakatsuka)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> Created attachment 8990188 [details]
> Bug 1468475 - Part 1. Introduce zeroPositionTime in order to shift the
> graduation if animations have negative-delay.
> 
> This patch will introduce the zeroPositionTime, this value means the time
> that
> current time of animation will be zero. In front-side, use this value for
> shifting the graduation in order to display the zero.
> 
> Review commit: https://reviewboard.mozilla.org/r/255212/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/255212/

I changed the zeroBaseTime to zeroPositionTime, this means that distance of time which between created time and negative-delay time.

And I updated the rewind button test by using negative-delay animation.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4e705bf5383ad55a0652d872108d531c3053081
Attachment #8990188 - Attachment is obsolete: true
Attachment #8990188 - Flags: review?(dakatsuka)
Attachment #8990189 - Attachment is obsolete: true
Attachment #8990189 - Flags: review?(dakatsuka)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #9)
> Created attachment 8990191 [details]
> Bug 1468475 - Part 1. Introduce zeroPositionTime in order to shift the
> graduation if animations have negative-delay.
> 
> This patch will introduce the zeroPositionTime, this value means the time
> that
> current time of animation will be zero. In front-side, use this value for
> shifting the graduation in order to display the zero.
> 
> Review commit: https://reviewboard.mozilla.org/r/255216/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/255216/

This change is inverting the sign of basePositionTime.

Try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dada632a0cb9221d1b6bb5c3114e4c50703379c5
Attachment #8990191 - Attachment is obsolete: true
Attachment #8990191 - Flags: review?(dakatsuka)
Attachment #8990192 - Attachment is obsolete: true
Attachment #8990192 - Flags: review?(dakatsuka)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #12)
> Created attachment 8990203 [details]
> Bug 1468475 - Part 1. Introduce zeroPositionTime in order to shift the
> graduation if animations have negative-delay.
> 
> This patch will introduce the zeroPositionTime, this value means the time
> that
> current time of animation will be zero. In front-side, use this value for
> shifting the graduation in order to display the zero.
> 
> Review commit: https://reviewboard.mozilla.org/r/255228/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/255228/

Sorry, I forgot adding previous fix. 
(https://hg.mozilla.org/try/rev/772e79638e5d575cdd1857390ac23029c9906d4a#l2.26)

I added these code and some test.

Try again again...https://treeherder.mozilla.org/#/jobs?repo=try&revision=70adf658e219696f450f7204efbe3eb8e92c6c45
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #14)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #12)
> > Created attachment 8990203 [details]
> > Bug 1468475 - Part 1. Introduce zeroPositionTime in order to shift the
> > graduation if animations have negative-delay.
> > 
> > This patch will introduce the zeroPositionTime, this value means the time
> > that
> > current time of animation will be zero. In front-side, use this value for
> > shifting the graduation in order to display the zero.
> > 
> > Review commit: https://reviewboard.mozilla.org/r/255228/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/255228/
> 
> Sorry, I forgot adding previous fix. 
> (https://hg.mozilla.org/try/rev/772e79638e5d575cdd1857390ac23029c9906d4a#l2.
> 26)
> 
> I added these code and some test.

Daisuke suggested the following points:
 * We need to prevent to dynamically graph change when setting the current time by using the scrubber.
 * We don't need to stamp the graduation if time is negative, excepting the first graduation and zero.

In order to prevent dynamically graph change, the inspector might store the current time when creating the animation player in server-side as the snapshot. In client-side, calculate the zero position time by using this created current time.
Attachment #8990203 - Attachment is obsolete: true
Attachment #8990203 - Flags: review?(dakatsuka)
Attachment #8990204 - Attachment is obsolete: true
Attachment #8990204 - Flags: review?(dakatsuka)
Attachment #8990628 - Attachment is obsolete: true
Attachment #8990628 - Flags: review?(dakatsuka)
Attached video screen.mp4 — 
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15)
> In order to prevent dynamically graph change, the inspector might store the
> current time when creating the animation player in server-side as the
> snapshot. In client-side, calculate the zero position time by using this
> created current time.

This attachment capture is the behavior after modifying the above point.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e24ce45b15b3b42f09a76de9a5a729fc47139fc
Attachment #8989894 - Attachment is obsolete: true
Comment on attachment 8990628 [details]
Bug 1468475 - Part 2. Display the zero graduation.

https://reviewboard.mozilla.org/r/255698/#review262388


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/components/AnimationListContainer.js:88
(Diff revision 1)
> +      ticks.push({ position, label });
> +    }
> +
>      for (let i = 0; i <= tickCount; i++) {
> -      const position = i * intervalWidth * 100 / width;
> -      const label = isAllDurationInfinity && i === tickCount
> +      const position = ((i * intervalWidth) + shiftWidth) * 100 / width;
> +      let distance = timeScale.distanceToRelativeTime(position);

Error: 'distance' is never reassigned. Use 'const' instead. [eslint: prefer-const]
Attachment #8990627 - Attachment is obsolete: true
Attachment #8990627 - Flags: review?(dakatsuka)
Attachment #8990629 - Attachment is obsolete: true
Attachment #8990629 - Flags: review?(dakatsuka)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #23)
> Created attachment 8990659 [details]
> Bug 1468475 - Part 3. Add negative current time tests.
> 
> This patch will add tests which is related with negative current time.
>  * Rewind the animations which has negative delay.
>  * Replay the animations which has negative delay.
>  * Inspect the negative current time animation.
> 
> Review commit: https://reviewboard.mozilla.org/r/255736/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/255736/

I addressed the daisuke's suggestion:
 * Display the negative guraduation as well if the graduation width is the size which is enough to display the label.
 * Inverse the sign of calculated zeroPositionTime in initialization of timeScale.
 * Add the code ensureing the compatibility.(i.e., the case that createdCurrentTime is undefined.)
 * test need to be separated into third patch.

I think we can skip display the label if the label area is narrow due to shifting the graduations. (we can detect this case by checking shifted width is shorter than interval width.) 

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78a21dcd2a74544ba57796fec70bfe5c1621b546
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #24)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #23)
> > Created attachment 8990659 [details]
> > Bug 1468475 - Part 3. Add negative current time tests.
> > 
> > This patch will add tests which is related with negative current time.
> >  * Rewind the animations which has negative delay.
> >  * Replay the animations which has negative delay.
> >  * Inspect the negative current time animation.
> > 
> > Review commit: https://reviewboard.mozilla.org/r/255736/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/255736/
> 
> I addressed the daisuke's suggestion:
>  * Display the negative guraduation as well if the graduation width is the
> size which is enough to display the label.
>  * Inverse the sign of calculated zeroPositionTime in initialization of
> timeScale.
>  * Add the code ensureing the compatibility.(i.e., the case that
> createdCurrentTime is undefined.)
>  * test need to be separated into third patch.
> 
> I think we can skip display the label if the label area is narrow due to
> shifting the graduations. (we can detect this case by checking shifted width
> is shorter than interval width.) 
> 
> Try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=78a21dcd2a74544ba57796fec70bfe5c1621b546

Daisuke's comment about this:

 * Change the 'createdCurrentTime' to 'currentTimeAtCreated' or else.
 * Use String.padStart for converting the time.
 * After changing the current time with setting current time, the first graduations will not work well in some situation.
We will need to consider that to shift zero position time is two cases:
 * Animations have negative current time which is smaller than negative delay.
 * Animations have the negative delay.

We should calculate the zero position time individually way based on each case. As a result of this calculation, if duration will expand to the negative direction, we should update the zero position time since previous zero position time is based on the previous duration.

Try:https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6a8d51df0d70b35e5535079da98e81ef7d80f31&group_state=expanded
Attachment #8990657 - Attachment is obsolete: true
Attachment #8990657 - Flags: review?(dakatsuka)
Attachment #8990658 - Attachment is obsolete: true
Attachment #8990658 - Flags: review?(dakatsuka)
Attachment #8990659 - Attachment is obsolete: true
Attachment #8990659 - Flags: review?(dakatsuka)
Comment on attachment 8990903 [details]
Bug 1468475 - Part 1. Introduce zeroPositionTime in order to shift the graduation if animations have negative-delay.

https://reviewboard.mozilla.org/r/255910/#review262768

Thanks Man-san,
your patch works well!

::: devtools/client/inspector/animation/utils/timescale.js:46
(Diff revision 1)
>        } = animation.state;
>  
>        const toRate = v => v / playbackRate;
> -      const startTime = createdTime + toRate(Math.min(delay, 0));
> +      const negativeDelay = toRate(Math.min(delay, 0));
> +      let startPositionTime = createdTime + negativeDelay;
> +      const originalCurrentTime =

Could you add a comment for backward compatibility here? Also, the name of `originalCurrentTime` makes me to confuse a bit. And, we need to check whether the currentTimeAtCreated is undefined or not, since if `currentTimeAtCreated` = 0, the eval will be wrong.

For example, how about this?

```
// If currentTimeAtCreated is not defined (which happens when connected to server older
// than FF62), use startPositionTime instead. See bug 1468475.
const actualCurrentTimeAtCreated = toRate(typeof currentTimeAtCreated === "undefined" ? currentTimeAtCreated : startPositionTime);
```

::: devtools/client/inspector/animation/utils/timescale.js:85
(Diff revision 1)
>  
> -      minStartTime = Math.min(minStartTime, startTime);
>        maxEndTime = Math.max(maxEndTime, endTime);
>        animationsCurrentTime =
>          Math.max(animationsCurrentTime, createdTime + toRate(currentTime));
> +      zeroPositionTime = Math.max(zeroPositionTime, animationZeroPositionTime);

We can move this line to `else` block?

```
if (startPositionTime < minStartTime) {
...
} else {
  zeroPositionTime = Math.max(zeroPositionTime, animationZeroPositionTime);
}
```

::: devtools/client/inspector/animation/utils/timescale.js:86
(Diff revision 1)
> -      minStartTime = Math.min(minStartTime, startTime);
>        maxEndTime = Math.max(maxEndTime, endTime);
>        animationsCurrentTime =
>          Math.max(animationsCurrentTime, createdTime + toRate(currentTime));
> +      zeroPositionTime = Math.max(zeroPositionTime, animationZeroPositionTime);
> +      if (startPositionTime < minStartTime) {

NIT: Please add a blank line before `if`.

::: devtools/client/inspector/animation/utils/timescale.js:174
(Diff revision 1)
>     */
>    formatTime(time) {
> +    // Ignore negative zero
> +    if (Math.abs(time) < (1 / 1000)) {
> +      time = 0.0;
> +    }

NIT: Add a blank line after here.
Attachment #8990903 - Flags: review?(dakatsuka) → review+
Comment on attachment 8990904 [details]
Bug 1468475 - Part 2. Display the zero graduation.

https://reviewboard.mozilla.org/r/255912/#review262774

Thanks!

::: devtools/client/inspector/animation/components/AnimationListContainer.js:81
(Diff revision 1)
> +    const needToShift = zeroBasePosition !== 0 && shiftWidth !== 0;
>  
>      const ticks = [];
> +    // Need to display first graduation since position will be shifted.
> +    if (needToShift) {
> +      const position = 0;

We don't need this variable?
```
ticks.push({ position: 0, label });
```

::: devtools/client/inspector/animation/components/AnimationListContainer.js:95
(Diff revision 1)
> -                      ? getStr("player.infiniteTimeLabel")
> +                  ? getStr("player.infiniteTimeLabel")
> -                      : timeScale.formatTime(timeScale.distanceToRelativeTime(position));
> +                  : timeScale.formatTime(distance);
> +      // As result of shifting the label, first shifted label might overlap
> +      // to the most left label. So display empyt label in this case.
> +      // And prevent to skip displaying zero position label.
> +      if (needToShift &&

nit: it might be better that compares `i == 0` at first? Also, `i === 0` is better.

```
if (i === 0 && 
    needToShift && 
    ...
```
Attachment #8990904 - Flags: review?(dakatsuka) → review+
Comment on attachment 8990905 [details]
Bug 1468475 - Part 3. Add negative current time tests.

https://reviewboard.mozilla.org/r/255914/#review262778

Thanks!

::: devtools/client/inspector/animation/test/browser_animation_rewind-button.js:13
(Diff revision 1)
>  // * make animations to rewind to zero
>  // * the state should be always paused after rewinding
>  
>  add_task(async function() {
> -  await addTab(URL_ROOT + "doc_custom_playback_rate.html");
> +  await addTab(URL_ROOT + "doc_multi_timings.html");
>    const { animationInspector, panel } = await openAnimationInspector();

It might be better to remove extra animation.
Perhaps:

```
  await removeAnimatedElementsExcept([".animated",
                                      ".negative-delay"]);
```

Is this enough?
Attachment #8990905 - Flags: review?(dakatsuka) → review+
Comment on attachment 8990903 [details]
Bug 1468475 - Part 1. Introduce zeroPositionTime in order to shift the graduation if animations have negative-delay.

https://reviewboard.mozilla.org/r/255910/#review262768

Thanks, Diasuke,

I addressed these patches.
Comment on attachment 8990903 [details]
Bug 1468475 - Part 1. Introduce zeroPositionTime in order to shift the graduation if animations have negative-delay.

https://reviewboard.mozilla.org/r/255910/#review263258


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/utils/timescale.js:46
(Diff revision 2)
>        } = animation.state;
>  
>        const toRate = v => v / playbackRate;
> -      const startTime = createdTime + toRate(Math.min(delay, 0));
> +      const negativeDelay = toRate(Math.min(delay, 0));
> +      let startPositionTime = createdTime + negativeDelay;
> +      // If currentTimeAtCreated is not defined (which happens when connected to server older

Error: Line 46 exceeds the maximum line length of 90. [eslint: max-len]
Comment on attachment 8990904 [details]
Bug 1468475 - Part 2. Display the zero graduation.

https://reviewboard.mozilla.org/r/255912/#review263260


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/components/AnimationListContainer.js:81
(Diff revision 2)
> +    const needToShift = zeroBasePosition !== 0 && shiftWidth !== 0;
>  
>      const ticks = [];
> +    // Need to display first graduation since position will be shifted.
> +    if (needToShift) {
> +      const position = 0;

Error: 'position' is assigned a value but never used. [eslint: no-unused-vars]
Status: NEW → ASSIGNED
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bf7932d022c3
Part 1. Introduce zeroPositionTime in order to shift the graduation if animations have negative-delay. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/b319a3547802
Part 2. Display the zero graduation. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/12b86e5e0725
Part 3. Add negative current time tests. r=daisuke
https://hg.mozilla.org/mozilla-central/rev/bf7932d022c3
https://hg.mozilla.org/mozilla-central/rev/b319a3547802
https://hg.mozilla.org/mozilla-central/rev/12b86e5e0725
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: