Closed Bug 1454392 Opened 6 years ago Closed 6 years ago

Manual seek on the animation breaks the animation grid

Categories

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

61 Branch
defect

Tracking

(firefox61 disabled, firefox62 verified)

VERIFIED FIXED
Firefox 62
Tracking Status
firefox61 --- disabled
firefox62 --- verified

People

(Reporter: gpalko, Assigned: daisuke)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files)

[Environment:]
Windows 8.1, Windows 10, Mac OSX 10.13.2
Nightly 61.0a1 2018-04-04

[Steps:]
1. Open Firefox Nightly and load https://rawgit.com/dadaa/3b73f847427025b51ba1ab7333013d0c/raw/77f3f0bb884875a179c3407f73bf8a8dd54751c9/doc_multi_timing.html
2. Press F12.
3. Select Inspector.
4. Select Animation tab.
5. Make sure, that <body> is selected in Inspector
6. On Animations panel: move the seek bar to ~1600 sec and play the animation
7. Select a node and select <body> again
[Actual Result:]
6. Elements from animation grid are also moved to right by the seeker
7. Some nodes disappeared from Animations panel
 
[Expected Result:]
6. Playback should continue from the time set by the seekbar
7. All the nodes should be displayed on Animation panel
Attached image animationGrid.png
Added screenshot with the broken animation grid.
Assignee: nobody → dakatsuka
Patrick, I'm sorry to bother you when you are very busy,,
Could you review those patches because I modified server side as well?

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=116f5de79c6dadef8ed74e911be70384fd1ffa6a
Comment on attachment 8974304 [details]
Bug 1454392 - Part 1: Introduce createdTime variable which holds time that animation created.

https://reviewboard.mozilla.org/r/242614/#review248494

This first server-only change looks good to me. But we'll need some backward compat handling in the client-side consumer code too.
Attachment #8974304 - Flags: review?(pbrosset) → review+
Comment on attachment 8974305 [details]
Bug 1454392 - Part 2: Apply createdTime to summary graph so as keeping proper graph.

https://reviewboard.mozilla.org/r/242616/#review248496

::: devtools/client/inspector/animation/components/graph/DelaySign.js:32
(Diff revision 1)
>        playbackRate,
> -      previousStartTime = 0,
>      } = animation.state;
>  
> -    const delay = animation.state.delay / playbackRate;
> -    const startTime =
> +    const toRate = v => v / playbackRate;
> +    const startTime = createdTime + toRate(Math.min(delay, 0)) - timeScale.minStartTime;

There we go, right here `createdTime` may not exist if you're remote debugging a device that doesn't have a server with the same level of code.
The `animation.state` object will just have undefined for this property.
So you'll need to handle this case in some way. Find a way to degrade gracefully.

The best would obviously be to have the bug fixed with any server, whether `createdTime` exists or not.
But if that's not possible, I suggest adding a check for whether the property exists, and if it doesn't, you just keep the old logic (with the bug).

::: devtools/client/inspector/animation/components/graph/EndDelaySign.js:35
(Diff revision 1)
>        playbackRate,
> -      previousStartTime = 0,
>      } = animation.state;
>  
> -    const endDelay = animation.state.endDelay / playbackRate;
> -    const startTime = previousStartTime - timeScale.minStartTime;
> +    const toRate = v => v / playbackRate;
> +    const startTime = createdTime - timeScale.minStartTime;

Same comment here.

::: devtools/client/inspector/animation/components/graph/SummaryGraphPath.js:200
(Diff revision 1)
>        timeScale,
>      } = this.props;
>  
> -    const totalDuration = this.getTotalDuration(animation, timeScale);
> -    const { playbackRate, previousStartTime = 0 } = animation.state;
> +    const { createdTime, playbackRate } = animation.state;
> +
> +    // Make graph coordinates to be able to handle as playbackRate 1.

Not sure I understand this comment line. Could you rephrase it please?

::: devtools/client/inspector/animation/components/graph/SummaryGraphPath.js:201
(Diff revision 1)
>      } = this.props;
>  
> -    const totalDuration = this.getTotalDuration(animation, timeScale);
> -    const { playbackRate, previousStartTime = 0 } = animation.state;
> +    const { createdTime, playbackRate } = animation.state;
> +
> +    // Make graph coordinates to be able to handle as playbackRate 1.
> +    const offset = createdTime * playbackRate;

Same comment about having some special case when createdTime does not exist.

::: devtools/client/inspector/animation/utils/timescale.js:52
(Diff revision 1)
>        playbackRate,
> -      previousStartTime = 0,
>      } = state;
>  
>      const toRate = v => v / playbackRate;
> -    const minZero = v => Math.max(v, 0);
> +    const startTime = createdTime + toRate(Math.min(delay, 0));

And here too.
Attachment #8974305 - Flags: review?(pbrosset)
Comment on attachment 8974306 [details]
Bug 1454392 - Part 3: Change the scrubber to createdTime base.

https://reviewboard.mozilla.org/r/242624/#review248502

::: devtools/server/actors/animation.js:888
(Diff revision 1)
> +   * Set the created time based current time of animations at the same time.
> +   *
> +   * @param {Array} actors A list of AnimationPlayerActor.
> +   * @param {Number} currentTime Created time based current time.
> +   * @param {Boolean} shouldPause Should the players be paused too.
> +   */
> +  setCreatedTimeBasedCurrentTimes: function(actors, currentTime, shouldPause) {

The word `time` is repeated a lot of times here ;)
It's a bit confusing.

Also, this is a new actor method, so the same backward compat problem is going to occur.
You will need your client-side code to test whether the method exists or not, and some special handling if it doesn't.

Why don't we keep just one method `setCurrentTimes` but add an extra param at the end so it becomes:
`setCurrentTimes(players, time, {shouldPause, relativeToCreatedTime})`

The problem is that if you're connected to an older server that doesn't have this, the request will fail because it expects a boolean (`shouldPause`) instead of an object.
Maybe you can add a `trait` to the animations actor so your front-end knows if this is supported at all.
Attachment #8974306 - Flags: review?(pbrosset)
Comment on attachment 8974308 [details]
Bug 1454392 - Part 5: Add test whether proper currentTime was set for each animations.

https://reviewboard.mozilla.org/r/242628/#review248570
Attachment #8974308 - Flags: review?(pbrosset) → review+
Comment on attachment 8974307 [details]
Bug 1454392 - Part 4: Add test whether the graph layout was not broken.

https://reviewboard.mozilla.org/r/242626/#review248594

::: devtools/client/inspector/animation/test/browser_animation_summary-graph_layout-by-seek.js:50
(Diff revision 1)
> +    const coordinates = { svgViewBoxX, svgViewBoxWidth,
> +                          pathX, delayX, delayWidth, endDelayX, endDelayWidth };
> +    initialCoordinatesResult.push(coordinates);
> +  }
> +
> +  info("Seek to behind of 1st iteration");

By behind, do you mean before or after the 1st iteration? Might be better to use one of these words to make this easier to understand.

::: devtools/client/inspector/animation/test/browser_animation_summary-graph_layout-by-seek.js:60
(Diff revision 1)
> +  for (let i = 0; i < itemEls.length; i++) {
> +    const expectedCoordinates = initialCoordinatesResult[i];
> +    const itemEl = itemEls[i];
> +    const svgEl = itemEl.querySelector("svg");
> +    is(svgEl.viewBox.baseVal.x, expectedCoordinates.svgViewBoxX,
> +      "X of viewBox of svg should be same");
> +    is(svgEl.viewBox.baseVal.width, expectedCoordinates.svgViewBoxWidth,
> +      "Width of viewBox of svg should be same");
> +
> +    const pathEl = svgEl.querySelector(".animation-computed-timing-path");
> +    is(pathEl.transform.baseVal[0].matrix.e, expectedCoordinates.pathX,
> +      "X of tansform of path element should be same");
> +
> +    const delayEl = itemEl.querySelector(".animation-delay-sign");
> +
> +    if (delayEl) {
> +      const computedStyle = delayEl.ownerGlobal.getComputedStyle(delayEl);
> +      is(computedStyle.left, expectedCoordinates.delayX,
> +        "X of delay sign should be same");
> +      is(computedStyle.width, expectedCoordinates.delayWidth,
> +        "Width of delay sign should be same");
> +    } else {
> +      ok(!expectedCoordinates.delayX, "X of delay sign should exist");
> +      ok(!expectedCoordinates.delayWidth, "Width of delay sign should exist");
> +    }
> +
> +    const endDelayEl = itemEl.querySelector(".animation-end-delay-sign");
> +
> +    if (endDelayEl) {
> +      const computedStyle = endDelayEl.ownerGlobal.getComputedStyle(endDelayEl);
> +      is(computedStyle.left, expectedCoordinates.endDelayX,
> +         "X of endDelay sign should be same");
> +      is(computedStyle.width, expectedCoordinates.endDelayWidth,
> +         "Width of endDelay sign should be same");
> +    } else {
> +      ok(!expectedCoordinates.endDelayX, "X of endDelay sign should exist");
> +      ok(!expectedCoordinates.endDelayWidth, "Width of endDelay sign should exist");
> +    }
> +  }

Please also move this to a separate function like `checkExpectedCoordinates`, so it's similar to the first function.
Attachment #8974307 - Flags: review?(pbrosset) → review+
Comment on attachment 8974305 [details]
Bug 1454392 - Part 2: Apply createdTime to summary graph so as keeping proper graph.

https://reviewboard.mozilla.org/r/242616/#review248496

> There we go, right here `createdTime` may not exist if you're remote debugging a device that doesn't have a server with the same level of code.
> The `animation.state` object will just have undefined for this property.
> So you'll need to handle this case in some way. Find a way to degrade gracefully.
> 
> The best would obviously be to have the bug fixed with any server, whether `createdTime` exists or not.
> But if that's not possible, I suggest adding a check for whether the property exists, and if it doesn't, you just keep the old logic (with the bug).

Thank you for the reviewing, Patrick!

For example, there is one actor which is referring to an animation. Then, even if the actor re-generated by something, if we can have a guarantee that the re-generated actor refers same animation, we can implement in client side only. However, I think, in client side, we don't have a way to know that without any modifing the actor.
So, let me implement the code for backwards compatibility. Other points are as well.

I have one question. How long should we support the backwards compatibility? Forever?

> Not sure I understand this comment line. Could you rephrase it please?

Ah, sorry, I'll rewrite this.
Comment on attachment 8974306 [details]
Bug 1454392 - Part 3: Change the scrubber to createdTime base.

https://reviewboard.mozilla.org/r/242624/#review248502

> The word `time` is repeated a lot of times here ;)
> It's a bit confusing.
> 
> Also, this is a new actor method, so the same backward compat problem is going to occur.
> You will need your client-side code to test whether the method exists or not, and some special handling if it doesn't.
> 
> Why don't we keep just one method `setCurrentTimes` but add an extra param at the end so it becomes:
> `setCurrentTimes(players, time, {shouldPause, relativeToCreatedTime})`
> 
> The problem is that if you're connected to an older server that doesn't have this, the request will fail because it expects a boolean (`shouldPause`) instead of an object.
> Maybe you can add a `trait` to the animations actor so your front-end knows if this is supported at all.

Thanks!
Can we add one Option[1] arguiment to setCurrentTimes?? Like:
setCurrentTimes(players, time, shouldPause, relativeToCreatedTime);
Even if use that, the request will be failed in previous serever?

[1] https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#551
I updated the patches because the try had oranges for old animation inspector.
new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f356e5e6040c6a4592305dc565dbdb70d725045b
(In reply to Daisuke Akatsuka (:daisuke) from comment #13)
> I have one question. How long should we support the backwards compatibility?
> Forever?
Not forever, we used to support quite old versions of the server because we wanted to support firefox OS remote debugging too, but that's not needed anymore.
Nowadays, we support all the way to the latest ESR release. So, 52 now, or 60 already? I don't remember if ESR 60 has been published already. Let's assume 60 is ESR now: that means we need our toolbox to work with any server version >= 60. So if you are connected to a server that is 60, it won't have new methods or properties you add in 62, so your code must be able to handle that.
Comment on attachment 8974305 [details]
Bug 1454392 - Part 2: Apply createdTime to summary graph so as keeping proper graph.

https://reviewboard.mozilla.org/r/242616/#review249514

::: devtools/client/inspector/animation/components/graph/SummaryGraphPath.js:202
(Diff revision 2)
>  
> -    const totalDuration = this.getTotalDuration(animation, timeScale);
> -    const { playbackRate, previousStartTime = 0 } = animation.state;
> +    const { createdTime, playbackRate } = animation.state;
> +
> +    // If no createdTime, use previous way for backward compatibility.
> +    const baseTime = createdTime || (animation.state.previousStartTime || 0);
> +    // Absobe the playbackRate with viewBox of SVG and offset of child path elements.

Typo here: "Absobe"?

::: devtools/client/inspector/animation/utils/timescale.js:105
(Diff revision 2)
>     */
>    getDuration() {
>      return this.maxEndTime - this.minStartTime;
>    }
> +
> +  _initializeWithoutCreatedTime(animations) {

Please add a jsdoc comment for this function to explain why we even need to have it. Something like: "Same as the constructor but doesn't use the animation's createdTime property which has only been added in FF62, for backward compatbility reasons".

Also, please move this function to be right after the class constructor function.
Attachment #8974305 - Flags: review?(pbrosset) → review+
Comment on attachment 8974306 [details]
Bug 1454392 - Part 3: Change the scrubber to createdTime base.

https://reviewboard.mozilla.org/r/242624/#review249532

::: devtools/client/inspector/animation/animation.js:383
(Diff revision 3)
> -      await this.animationsFront.setCurrentTimes(animations, currentTime, true);
> +      currentTime =
> +        // If no createdTimeBasedCurrentTime,
> +        // set currentTime as it is for backward compatibility.
> +        typeof timeScale.createdTimeBasedCurrentTime === "undefined"
> +          ? currentTime : currentTime + timeScale.minStartTime;

Only the `await` expression below needs to be in the `try` block. These new lines don't need to be inside it. Please move them before.

::: devtools/client/inspector/animation/current-time-timer.js:30
(Diff revision 3)
>     *                   1st: current time
>     *                   2nd: if shouldStopAfterEndTime is true and
>     *                        the current time is over the end time, true is given.
>     */
>    constructor(timeScale, shouldStopAfterEndTime, win, onUpdated) {
> -    this.baseCurrentTime = timeScale.documentCurrentTime - timeScale.minStartTime;
> +    // If no createdTimeBasedCurrentTime, use previous way for backward compatibility.

For someone reading the code "the previous way" might not mean anything. We should never assume that the code reader has knowledge of everything that changed in the past within this code.

So, you need some light rephrasing here: "If createdTimeBasedCurrentTime is not defined (which happens when connected to server older than FF62), use documentCurrentTime instead. See bug 1454392".

In fact, everywhere you added a comment that says something about backward compat, it might be good to give the bug number.

::: devtools/client/inspector/animation/utils/timescale.js:30
(Diff revision 3)
>      if (!animations.every(animation => animation.state.createdTime)) {
>        // Backward compatibility for createdTime.
>        return this._initializeWithoutCreatedTime(animations);
>      }
>  
> -    this.minStartTime = Infinity;
> +    let createdTimeBasedCurrentTime = -Number.MAX_VALUE;

What about using -Infinity here instead?

::: devtools/client/inspector/animation/utils/timescale.js:30
(Diff revision 3)
>      if (!animations.every(animation => animation.state.createdTime)) {
>        // Backward compatibility for createdTime.
>        return this._initializeWithoutCreatedTime(animations);
>      }
>  
> -    this.minStartTime = Infinity;
> +    let createdTimeBasedCurrentTime = -Number.MAX_VALUE;

I'm a bit confused by this variable name.
Really, what we want here is `currentTime` only, right?
The only reason we prefix it with `createdTimeBased` is because it's relative to the animation's current time whereas we didn't use to have this before this bug.
So we're giving a variable a name because the code used to be different before.
I don't particularly like this. I prefer if the code can be as self-explanatory as possible, without requiring the reader to know that, before, in the past, we used to have a different kind of `currentTime`, and that now we use the concept of `createdTime`, etc.

Do you think we could rename this `currentTime` only? And then change also the `this.createdTimeBasedCurrentTime` to `this.currentTime`, and then all of the other places that depend on this?

::: devtools/server/actors/animation.js:859
(Diff revision 3)
>      let readyPromises = [];
>      // Until the WebAnimations API provides a way to play/pause via the document
>      // timeline, we have to iterate through the whole DOM to find all players.
>      for (let player of
> -         this.getAllAnimations(this.tabActor.window.document, true)) {
> +      this.getAllAnimations(this.tabActor.window.document, true)) {
> -      player.play();
> +      // Play animation using startTime. Because the Animation.Play() method recalcurates

Typo: recalculates

::: devtools/server/actors/animation.js:896
(Diff revision 3)
> +    // For backward compatibility for old animation inspector.
> +    // We can drop following procedures after dropping old one.
> +    if (!options.relativeToCreatedTime) {
> -    return Promise.all(players.map(player => {
> +      return Promise.all(players.map(player => {
> -      let pause = shouldPause ? player.pause() : Promise.resolve();
> +        let pause = shouldPause ? player.pause() : Promise.resolve();
> -      return pause.then(() => player.setCurrentTime(time));
> +        return pause.then(() => player.setCurrentTime(time));
> -    }));
> +      }));
> +    }

So, we only maintain backward compat in one direction: newer client with older server.
We never supported it for older client with newer server.

So, on the server, you can always assume that this option will be defined. 
In practice, someone could very well try to connect to a FF62 server with a FF60 client, and in that case the option would be missing, but we just don't support that case.

So, you can remove that backward compatibility check here.
Attachment #8974306 - Flags: review?(pbrosset)
(In reply to Daisuke Akatsuka (:daisuke) from comment #14)
> Can we add one Option[1] arguiment to setCurrentTimes?? Like:
> setCurrentTimes(players, time, shouldPause, relativeToCreatedTime);
> Even if use that, the request will be failed in previous serever?
I tested this, and it turns out that protocol.js does *not* fail in this case: calling a method with an extra parameter on a server that does not support it. So I think you're fine.
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8974305 [details]
Bug 1454392 - Part 2: Apply createdTime to summary graph so as keeping proper graph.

https://reviewboard.mozilla.org/r/242616/#review249514

> Please add a jsdoc comment for this function to explain why we even need to have it. Something like: "Same as the constructor but doesn't use the animation's createdTime property which has only been added in FF62, for backward compatbility reasons".
> 
> Also, please move this function to be right after the class constructor function.

Okay, thanks!
Comment on attachment 8974306 [details]
Bug 1454392 - Part 3: Change the scrubber to createdTime base.

https://reviewboard.mozilla.org/r/242624/#review249532

> What about using -Infinity here instead?

In this modifying, currentTime might be negative value. For example, if animation has "delay" which is negative, the graph moves to right by its delay. This is same behavior as previous one. After that, in case of moving the scrubber to the negative area, currentTime will be nagative, then resume from that time. In previous, skips negative delay duration, resumes from 0. I think, if time tick label begin from negative not zero, this behavior is not bad. However, in current label, it makes be confused a bit.
(I'll attach a movie later.)

> I'm a bit confused by this variable name.
> Really, what we want here is `currentTime` only, right?
> The only reason we prefix it with `createdTimeBased` is because it's relative to the animation's current time whereas we didn't use to have this before this bug.
> So we're giving a variable a name because the code used to be different before.
> I don't particularly like this. I prefer if the code can be as self-explanatory as possible, without requiring the reader to know that, before, in the past, we used to have a different kind of `currentTime`, and that now we use the concept of `createdTime`, etc.
> 
> Do you think we could rename this `currentTime` only? And then change also the `this.createdTimeBasedCurrentTime` to `this.currentTime`, and then all of the other places that depend on this?

Okay, thanks!

> So, we only maintain backward compat in one direction: newer client with older server.
> We never supported it for older client with newer server.
> 
> So, on the server, you can always assume that this option will be defined. 
> In practice, someone could very well try to connect to a FF62 server with a FF60 client, and in that case the option would be missing, but we just don't support that case.
> 
> So, you can remove that backward compatibility check here.

However, we can choose the old animation inspector by pref in Nightly. So as to ensure that work, we need to support, I think. No?
Attached video negative-delay.m4v
I attach a movie which has negative delay. (2nd animation)
(The tick label which begins from negative value might be better anyway.)
Removing tracking flag since the feature is pref'ed off in FX61 and has been moved to Fx62.
Patrick, thank you for all time!
Could you give your opinion for my comment 29 especially backward compatibility?
Flags: needinfo?(pbrosset)
Comment on attachment 8974306 [details]
Bug 1454392 - Part 3: Change the scrubber to createdTime base.

https://reviewboard.mozilla.org/r/242624/#review251340

::: devtools/server/actors/animation.js:854
(Diff revision 4)
>    /**
>     * Play all animations in the current tabActor's frames.
>     * This method only returns when animations have left their pending states.
>     */
>    playAll: function() {
>      let readyPromises = [];

This array isn't used anymore, and should be removed now. And the function should not return a promise anymore.

::: devtools/server/actors/animation.js:896
(Diff revision 4)
> +    // For backward compatibility for old animation inspector.
> +    // We can drop following procedures after dropping old one.
> +    if (!options.relativeToCreatedTime) {

I see what you mean about supporting the old front-end now.
Ok for this, but let's file a bug to remember to remove this as soon as the old panel is deleted.
Attachment #8974306 - Flags: review?(pbrosset) → review+
(In reply to Daisuke Akatsuka (:daisuke) from comment #38)
> Patrick, thank you for all time!
> Could you give your opinion for my comment 29 especially backward
> compatibility?
See my last comment. I'm fine keeping this in as long as we remove it as soon as the old front-end is deleted.
If we are confident that we'll delete it in this cycle, we don't even need to add the code by the way (it would just be broken in nightly for a few days/week).
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #40)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #38)
> > Patrick, thank you for all time!
> > Could you give your opinion for my comment 29 especially backward
> > compatibility?
> See my last comment. I'm fine keeping this in as long as we remove it as
> soon as the old front-end is deleted.
> If we are confident that we'll delete it in this cycle, we don't even need
> to add the code by the way (it would just be broken in nightly for a few
> days/week).

I'll file the bug, thanks!
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/851a86672d35
Part 1: Introduce createdTime variable which holds time that animation created. r=pbro
https://hg.mozilla.org/integration/autoland/rev/51437c0bf54c
Part 2: Apply createdTime to summary graph so as keeping proper graph. r=pbro
https://hg.mozilla.org/integration/autoland/rev/927f024d934d
Part 3: Change the scrubber to createdTime base. r=pbro
https://hg.mozilla.org/integration/autoland/rev/d10b9c00ffcd
Part 4: Add test whether the graph layout was not broken. r=pbro
https://hg.mozilla.org/integration/autoland/rev/2f6123d1d505
Part 5: Add test whether proper currentTime was set for each animations. r=pbro
See Also: → 1463372
Verified as fixed in Nightly 62.0a1(20180528220216). The issue is not reproducing, if manual seek is performed, the animation grid is displayed as expected.
Status: RESOLVED → VERIFIED
Depends on: 1468364
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: