Closed Bug 1468364 Opened 6 years ago Closed 6 years ago

A node's graph isn't displayed correctly on body view when modifying it's speed

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

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

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

People

(Reporter: danibodea, Assigned: mantaroh)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(3 files)

speed modification and graphs bug.png
40.79 KB, image/png
Details
Bug 1468364 - Part 1. Calculate the created time of animation every creating the AnimationPlayerActor.
59 bytes, text/x-review-board-request
daisuke
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1468364 - Part 2. Add animation inspector test which checks the created time when changing the playback rate and current time.
59 bytes, text/x-review-board-request
daisuke
: review+
Details 
STR:
1. Open this page:
https://rawgit.com/dadaa/3b73f847427025b51ba1ab7333013d0c/raw/77f3f0bb884875a179c3407f73bf8a8dd54751c9/doc_custom_playback_rate.html
2. Open the Animation Inspector.
3. Select the first div and modify it's speed to 10x.
4. Select the body.

Actual result:
The graph of the first node is improperly positioned on the scale when selecting the body. (it's positioned from the 

Expected result:
The first node's graph should be positioned correctly, when selecting the body.


NOTE: This bug is a regression; After running a mozregression, this changelog is pointed out:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fa332d9d704da9056d4c7493bcc4b718810c4529&tochange=2f6123d1d50509a115af4b58592d2f55044de72a

NOTE: Please see the attached pic for details about the actual and expected result.
Hello Bodea, thank you for the reporting.

I think, it’s proper behavior which both graphs start at same position since both animations were started at the same timing. It looks the previous inspector had the problem rather.
Could you confirm?
Flags: needinfo?(daniel.bodea)
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
(In reply to Daisuke Akatsuka (:daisuke) from comment #1)
> Hello Bodea, thank you for the reporting.
> 
> I think, it’s proper behavior which both graphs start at same position since
> both animations were started at the same timing. It looks the previous
> inspector had the problem rather.
> Could you confirm?
Initially we thought the same, that the pre fix 1454392, the way the the animation was positioned is the bug, but after actually comparing and thinking about, when you change the speed in a child you modify the animation bar, which means the general body animation scrubber position is confusing in this case and counter intuitive. 
In the STR example, we are speeding the animation 10x, which means that if this happens while animation is playing, the scrubber for the sped up animation is out of position and:
A. the 10x animation in parent (body) is shown as started at T0, which is correct but incorrect in the same time
B. the 10x animation in parent(body) is at different progress than in child(div)
C. the most important point is that the actual animation is at different point than the one depicted in (parent)body

So, bottom line from our point of view, even if the behavior before fix 1454392 was faulty, we find that the behavior in shifting the ratio modified child bar in rapport to the parent scrubber position is the way to go.
Flags: needinfo?(daniel.bodea)
Ah, I have realized the problem. Thanks!
Priority: -- → P3
Product: Firefox → DevTools
This seems kinda bad - presenting incorrect information on timing of animations. Any chance we can get a fix here? Also seems like P3 is a bit low. P2 at least with an assignee seems more appropriate.

[Tracking Requested - why for this release]:
bad regression in dev tools
status-firefox61: --- → unaffected
tracking-firefox62: --- → ?
Flags: needinfo?(dakatsuka)
Thanks for the notice, Jim.
Assignee: nobody → dakatsuka
Flags: needinfo?(dakatsuka)
Priority: P3 → P2
I try this.

I discussed with daisuke, we need to calculate the start position of graph from playbackRate and need to update current time when adding/removing the animation. In order to get playbackRate which is original we need to store playbackRate to the server when animation is created.
Assignee: dakatsuka → mantaroh
Furthermore, we need to modify the server side in order to adjust current time.
WIP:https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bbc9ece337e12e2ad7ee92ee420107298ec5d76
This patch changed the server side in order to notify the adjusting time which will shift the current time.

The actor of animation can't calculate this value since actor doesn't know the distance from previous current time and previous animation's current time.
(The front-end can calculate this, because, it knows the current animation's current time and scrubber current time.)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #11)
> Comment on attachment 8988364 [details]
> Bug 1468364 - Part 1. Update the created time of animation after changing
> the playbackRate.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/253634/diff/1-2/

Daisuke suggested to me that we may fix this bug by changing the server side. I investigated this again, as result of it, I decided that change the created time when changing the playback rate. If the created time has changed, the graph looks like that target animation is created in the middle of playing all animations.

This patch will change the created  time when playback rate has change.
Comment on attachment 8988364 [details]
Bug 1468364 - Part 1. Calculate the created time of animation every creating the AnimationPlayerActor.

https://reviewboard.mozilla.org/r/253634/#review260572


Code analysis found 4 defects in this patch:
 - 4 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/server/actors/animation.js:925
(Diff revision 2)
>     * Set the playback rate of several animations at the same time.
>     * @param {Array} players A list of AnimationPlayerActor.
>     * @param {Number} rate The new rate.
>     */
>    setPlaybackRates: function(players, rate) {
> +    for( const actor of players) {

Error: Expected space(s) after "for". [eslint: keyword-spacing]

::: devtools/server/actors/animation.js:925
(Diff revision 2)
>     * Set the playback rate of several animations at the same time.
>     * @param {Array} players A list of AnimationPlayerActor.
>     * @param {Number} rate The new rate.
>     */
>    setPlaybackRates: function(players, rate) {
> +    for( const actor of players) {

Error: There should be no spaces inside this paren. [eslint: space-in-parens]

::: devtools/server/actors/animation.js:926
(Diff revision 2)
>     * @param {Array} players A list of AnimationPlayerActor.
>     * @param {Number} rate The new rate.
>     */
>    setPlaybackRates: function(players, rate) {
> +    for( const actor of players) {
> +      let animation = actor.player;

Error: 'animation' is never reassigned. use 'const' instead. [eslint: prefer-const]

::: devtools/server/actors/animation.js:931
(Diff revision 2)
> +      let animation = actor.player;
> +      this.animationCreatedTimeMap.delete(animation);
> +      const createdTime = animation.timeline.currentTime;
> +      this.animationCreatedTimeMap.set(animation, createdTime);
> +
> +    }

Error: Block must not be padded by blank lines. [eslint: padded-blocks]
I changed the fixing approach. AnimationsActor may update the created time when creating the "AnimationPlayerActor" every time.
The behavior works well after applying the patch.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b7aef9d43395abb9203acdf25c14c63db3c518d
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #18)
> Comment on attachment 8988364 [details]
> Bug 1468364 - Part 1. Update the created time of animation when creating the
> AnimationPlayerActor.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/253634/diff/3-4/

Sorry, the previous patch was wrong due to my broken tree.  I created the new patch.
Try : https://treeherder.mozilla.org/#/jobs?repo=try&author=mantaroh@gmail.com
Comment on attachment 8988364 [details]
Bug 1468364 - Part 1. Calculate the created time of animation every creating the AnimationPlayerActor.

https://reviewboard.mozilla.org/r/253634/#review260872

Thanks Man-san,

Please let me cancel this review once since we need to modify a bit.

::: commit-message-15c95:4
(Diff revision 4)
> +Bug 1468364 - Part 1. Update the created time of animation when creating the AnimationPlayerActor. r?daisuke
> +
> +This patch will update the created time of animation when creating the
> +AnimationPlayerActor everytime in order to shift the graph.

I don't think, this patch is for graph shifting.
I think, this actual meaning is fixing that the createdTime has mismatched by changing the playbackRate or currentTime by the scrubber.
Could you change the message?

::: devtools/server/actors/animation.js:977
(Diff revision 4)
>      // Remove invalid animations.
>      for (const previousAnimation of this.animationCreatedTimeMap.keys()) {
>        if (!currentAnimations.includes(previousAnimation)) {
>          this.animationCreatedTimeMap.delete(previousAnimation);
>        }
>      }

Since the createdTime of existing animations are updated always, we can drop this, I think. Instead of this, we may clear the map at here.

```
this.animationCreatedTimeMap.clear();
```

::: devtools/server/actors/animation.js:995
(Diff revision 4)
> -    if (!this.animationCreatedTimeMap.has(animation)) {
> +    if (this.animationCreatedTimeMap.has(animation)) {
> +      this.animationCreatedTimeMap.delete(animation);
> +    }

We can drop this as well.
Because, set() of the map updates the value regardless of the existence of key.
Attachment #8988364 - Flags: review?(dakatsuka)
Comment on attachment 8988364 [details]
Bug 1468364 - Part 1. Calculate the created time of animation every creating the AnimationPlayerActor.

https://reviewboard.mozilla.org/r/253634/#review260872

Thanks a lot, daisuke!

I'll modify this.

> I don't think, this patch is for graph shifting.
> I think, this actual meaning is fixing that the createdTime has mismatched by changing the playbackRate or currentTime by the scrubber.
> Could you change the message?

Thanks.
I'll rewrite this comment.

> Since the createdTime of existing animations are updated always, we can drop this, I think. Instead of this, we may clear the map at here.
> 
> ```
> this.animationCreatedTimeMap.clear();
> ```

I'll drop the animationCreatedTimeMap. We may set the calculated value to created time in the getAnimationPlayersForNode and onAnimationMutation.
Comment on attachment 8988364 [details]
Bug 1468364 - Part 1. Calculate the created time of animation every creating the AnimationPlayerActor.

https://reviewboard.mozilla.org/r/253634/#review260882

Thanks!
Attachment #8988364 - Flags: review?(dakatsuka) → review+
Comment on attachment 8988365 [details]
Bug 1468364 - Part 2. Add animation inspector test which checks the created time when changing the playback rate and current time.

https://reviewboard.mozilla.org/r/253636/#review260886

Thanks!

::: devtools/client/inspector/animation/test/head.js:830
(Diff revision 5)
> +/**
> + * Check the adjusted current time and created time from specified two animations.
> + *
> + * @param {Array} Target animation's array.
> + */
> +function checkAdjustingTheTime(animations) {

Could you change to two params since this checks with two animations only?
```
function checkAdjustingTheTime(animation1, animation2)
```

::: devtools/client/inspector/animation/test/head.js:831
(Diff revision 5)
> + * Check the adjusted current time and created time from specified two animations.
> + *
> + * @param {Array} Target animation's array.
> + */
> +function checkAdjustingTheTime(animations) {
> +  is(animations.length, 2, "Should have the two animations");

Then, drop this.
Attachment #8988365 - Flags: review?(dakatsuka) → review+
Comment on attachment 8988365 [details]
Bug 1468364 - Part 2. Add animation inspector test which checks the created time when changing the playback rate and current time.

https://reviewboard.mozilla.org/r/253636/#review260886

Thanks a lot, daisuke.

I addressed this patch.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ed54e921f535
Part 1. Calculate the created time of animation every creating the AnimationPlayerActor. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/2d8fac92ff52
Part 2. Add animation inspector test which checks the created time when changing the playback rate and current time. r=daisuke
https://hg.mozilla.org/mozilla-central/rev/ed54e921f535
https://hg.mozilla.org/mozilla-central/rev/2d8fac92ff52
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment on attachment 8988364 [details]
Bug 1468364 - Part 1. Calculate the created time of animation every creating the AnimationPlayerActor.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1454392
[User impact if declined]: The animation inspector doesn't work well if the currents time of inspected animations are different.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Low
[Why is the change risky/not risky?]: This change will effect in this kind of situations. And all automated tests and the added tests of this case has passed.
[String changes made/needed]: N/A
Attachment #8988364 - Flags: approval-mozilla-beta?
I have validated this bug fix on the latest Nightly v63.0a1 (2018-07-02) (64-bit) on Windows 10, Ubuntu 16.04 and Mac OS X 10.12.6.
status-firefox63: fixed → verified
Flags: in-testsuite+
Comment on attachment 8988364 [details]
Bug 1468364 - Part 1. Calculate the created time of animation every creating the AnimationPlayerActor.

Fix for regression for a feature we're aiming to ship in 62. Please uplift for beta 7.
Attachment #8988364 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: