Closed Bug 1456857 Opened 6 years ago Closed 6 years ago

When a div is paused, the other divs from the body are also paused

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Platform:
Unspecified
All
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 disabled, firefox62 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- disabled
firefox62 --- verified

People

(Reporter: david.olah, Assigned: daisuke)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(3 files)

Bug 1456857 - Part 1: Pause/Play animations respectively.
59 bytes, text/x-review-board-request
pbro
: review+
birtles
: review+
Details
Bug 1456857 - Part 2: Refer to proper props.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1456857 - Part 3: Add a test for pausing/playing animations.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
[Environment:]
Windows 8.1, Windows 10, Mac OSX 10.13.2, Ubuntu 16.04
Nightly 61.0a1 2018-04-24

[Steps:]
1. Open Firefox Nightly and load https://rawgit.com/dadaa/3b73f847427025b51ba1ab7333013d0c/raw/77f3f0bb884875a179c3407f73bf8a8dd54751c9/doc_simple_animation.html
2. Press F12.
3. Select Inspector tab.
4. Select Animations tab.
5. Select a div from the body (Ex: ball delayed)
6. Press pause button from the Animations

[Expected Result:]
Only the selected div is paused and the rest of the divs from the body continue playing

[Actual Result:]
All of the divs in the body are paused

[Note]
The other way around has the same result (when all of the divs are paused, pressing play in one div plays all of the divs from the body)
Needs Triage?
Flags: needinfo?(gl)
Flags: needinfo?(gl) → needinfo?(dakatsuka)
The React-based Animation Inspector is still Nightly-only.
status-firefox60: --- → unaffected
status-firefox61: affected → disabled
status-firefox62: --- → affected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → unaffected
Assignee: nobody → dakatsuka
Flags: needinfo?(dakatsuka)
Priority: -- → P2
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de4b1326a6853f75747e2393de29602bfacf40fd

Perhaps, we may want to something sign so as to know that the animation is pausing or running on the inspector?
Comment on attachment 8975759 [details]
Bug 1456857 - Part 2: Refer to proper props.

https://reviewboard.mozilla.org/r/243972/#review250074
Attachment #8975759 - Flags: review?(gl) → review+
Attachment #8975760 - Flags: review?(gl) → review?(pbrosset)
Comment on attachment 8975758 [details]
Bug 1456857 - Part 1: Pause/Play animations respectively.

https://reviewboard.mozilla.org/r/243970/#review251328

::: devtools/client/inspector/animation/animation.js:429
(Diff revision 1)
> +      if (typeof this.hasPausePlay === "undefined") {
> +        this.hasPausePlay =
> +          await this.inspector.target.actorHasMethod("animations", "pause");
> +      }

This should not be inside the `try/catch`, otherwise we'll silently ignore errors with this code too, where we really don't want to.

::: devtools/server/actors/animation.js:831
(Diff revision 1)
>      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)) {
>        // Play animation using startTime. Because the Animation.Play() method recalculates
>        // startTime and currentTime on the procedure, but we need to keep the currentTime.
>        player.startTime =
>          player.timeline.currentTime - player.currentTime / player.playbackRate;
>      }
>      this.allAnimationsPaused = false;
>      return Promise.all(readyPromises);

Unrelated to your patch, but I found out while reviewing it that the `readyPromises` array in this function is always empty. It looks like we should just remove it entirely and not return a promise at all at the end.

Also, it makes me wonder if we still need it in `pauseAll`, because you don't wait fore this promise in the new `pause` function. Should you?

::: devtools/server/actors/animation.js:869
(Diff revision 1)
> +  pause: function(actors) {
> +    for (const { player } of actors) {
> +      player.startTime = null;
> +    }
> +  },
> +
> +  /**
> +   * Play given animations.
> +   *
> +   * @param {Array} actors A list of AnimationPlayerActor.
> +   */
> +  play: function(actors) {
> +    for (const { player } of actors) {
> +      player.startTime =
> +        player.timeline.currentTime - player.currentTime / player.playbackRate;
> +    }
> +  },

Can we rename those to `playSome` and `pauseSome` so it's more obvious what they do. Also it will be a nice parallel to `playAll` and `pauseAll`.
Attachment #8975758 - Flags: review?(pbrosset)
Comment on attachment 8975760 [details]
Bug 1456857 - Part 3: Add a test for pausing/playing animations.

https://reviewboard.mozilla.org/r/243974/#review251330

::: devtools/client/inspector/animation/test/browser_animation_pause-resume-button_respectively.js:36
(Diff revision 1)
> +function assertStatus(animations, pauseResumeButtonEl, shouldFirstAnimationPaused,
> +                      shouldSecondAnimationPaused, shouldButtonPaused) {

Let's try to make this assert function more generic.
Right now it's limited to checking 2 animations only. What if we want to expand the test in the future and make it check more animations. I suggest passing in an array instead, like so:

```
function assertStatus(animations, buttonEl, expectedButtonState, expectedAnimationStates)
```

where `expectedAnimationStates` is an array like:

`["paused", "running", ...]`

This way you can simplify the function like:

```
expectedAnimationStates.forEach((state, index) => {
  is(animations[index].state.playState, state,
     `Animation ${index} should be ${state}`);
});
```

Same for the part that checks the button:

```
ok(buttonEl.classList.contains(expectedButtonState),
   `State of ${expectedButtonState}`);
```
Attachment #8975760 - Flags: review?(pbrosset)
Comment on attachment 8975758 [details]
Bug 1456857 - Part 1: Pause/Play animations respectively.

https://reviewboard.mozilla.org/r/243970/#review251328

> Unrelated to your patch, but I found out while reviewing it that the `readyPromises` array in this function is always empty. It looks like we should just remove it entirely and not return a promise at all at the end.
> 
> Also, it makes me wonder if we still need it in `pauseAll`, because you don't wait fore this promise in the new `pause` function. Should you?

Indeed, it looks we don't need the promise.
Comment on attachment 8975760 [details]
Bug 1456857 - Part 3: Add a test for pausing/playing animations.

https://reviewboard.mozilla.org/r/243974/#review251330

> Let's try to make this assert function more generic.
> Right now it's limited to checking 2 animations only. What if we want to expand the test in the future and make it check more animations. I suggest passing in an array instead, like so:
> 
> ```
> function assertStatus(animations, buttonEl, expectedButtonState, expectedAnimationStates)
> ```
> 
> where `expectedAnimationStates` is an array like:
> 
> `["paused", "running", ...]`
> 
> This way you can simplify the function like:
> 
> ```
> expectedAnimationStates.forEach((state, index) => {
>   is(animations[index].state.playState, state,
>      `Animation ${index} should be ${state}`);
> });
> ```
> 
> Same for the part that checks the button:
> 
> ```
> ok(buttonEl.classList.contains(expectedButtonState),
>    `State of ${expectedButtonState}`);
> ```

Okay! Will introduce expectedAnimationStates. However, for buttonEl, the button class is only either "paused" or nothing[1]. 
So, let me leave it as it is.

[1] https://searchfox.org/mozilla-central/source/devtools/client/inspector/animation/components/PauseResumeButton.js#83
Comment on attachment 8975758 [details]
Bug 1456857 - Part 1: Pause/Play animations respectively.

https://reviewboard.mozilla.org/r/243970/#review251410
Attachment #8975758 - Flags: review?(pbrosset) → review+
Comment on attachment 8975760 [details]
Bug 1456857 - Part 3: Add a test for pausing/playing animations.

https://reviewboard.mozilla.org/r/243974/#review251412

::: devtools/client/inspector/animation/test/browser_animation_pause-resume-button_respectively.js:45
(Diff revision 2)
> +  if (shouldButtonPaused) {
> +    ok(buttonEl.classList.contains("paused"),
> +      "State of button should be paused");
> +  } else {
> +    ok(!buttonEl.classList.contains("paused"),
> +      "State of button should be running");
> +  }

You can still simplify this more:

```
is(buttonEl.classList.contains("paused"), shouldButtonPaused, "State of button is correct");
```
Attachment #8975760 - Flags: review?(pbrosset) → review+
Comment on attachment 8975760 [details]
Bug 1456857 - Part 3: Add a test for pausing/playing animations.

https://reviewboard.mozilla.org/r/243974/#review251412

> You can still simplify this more:
> 
> ```
> is(buttonEl.classList.contains("paused"), shouldButtonPaused, "State of button is correct");
> ```

Okay, thanks!
The try had failed in Windows 10 x64 pgo, Windows 7 opt, pgo.
Let me investigate.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70b6e76ef607097de8aaeafd84548c361710781f
Hi Patrick,
I would like you check again patch 1.
https://reviewboard.mozilla.org/r/243970/diff/3#index_header

I changed followings:
* Logic for pausing. (The reason was commented in the code)
* Introduce pausePlayer/playPlayer function in server to re-use.
* Remove "await" from tests in server due to the pauseAll/playAll/toggleAll became sync.

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

Thanks.
Flags: needinfo?(pbrosset)
Comment on attachment 8975758 [details]
Bug 1456857 - Part 1: Pause/Play animations respectively.

https://reviewboard.mozilla.org/r/243970/#review251688

::: devtools/server/actors/animation.js:926
(Diff revision 3)
> +  /**
> +   * Pause given player.
> +   *
> +   * @param {Object} player
> +   */
> +  pausePlayer(player) {

See below but I'd probably call this pauseSync or doSyncPause or something of that sort.

::: devtools/server/actors/animation.js:932
(Diff revision 3)
> +    // For current Firefox(61), even if sets null to startTime to animation,
> +    // the playState keeps as it is in case of the animation has a pending task.
> +    // Though this spec was not decided yet, so as to avoid Firefox optimization
> +    // we set startTime to be the currentTime once, then set null to make the pause state.
> +    // See https://github.com/w3c/csswg-drafts/issues/2691

Perhaps reword as:

"Gecko includes an optimization that means that if the animation is play-pending and we set the startTime to null, the change will be ignored and then animation will continue to be play-pending. This violates the spec but until the spec is clarified[1] on this point we work around this by ensuring the animation's startTime is set to something non-null before setting it to null."

[1] https://github.com/w3c/csswg-drafts/issues/2691

::: devtools/server/actors/animation.js:942
(Diff revision 3)
> +    this.playPlayer(player);
> +    player.startTime = null;
> +  },
> +
> +  /**
> +   * Play given player.

(See below but probably mention that this does a synchronous play.)

::: devtools/server/actors/animation.js:946
(Diff revision 3)
> +  playPlayer(player) {
> +    // Play animation using startTime. Because the Animation.Play() method recalculates
> +    // startTime and currentTime on the procedure, but we need to keep the currentTime.
> +    player.startTime =
> +      player.timeline.currentTime - player.currentTime / player.playbackRate;
> +  },

I'd probably call this something like playSync (as in play synchronous).

Would it be good to check for a null startTime or zero playback rate?

e.g.

const currentTime = player.currentTime === null ? 0 : player.currentTime;

(or possibly even just `const currentTime = player.currentTime || 0` would work since if currentTime is null or 0 it will use 0 instead.)

If the playbackRate is zero then you can't really play the animation so it's probably just better to do an early return in that case.
Comment on attachment 8975758 [details]
Bug 1456857 - Part 1: Pause/Play animations respectively.

https://reviewboard.mozilla.org/r/243970/#review251688

> Perhaps reword as:
> 
> "Gecko includes an optimization that means that if the animation is play-pending and we set the startTime to null, the change will be ignored and then animation will continue to be play-pending. This violates the spec but until the spec is clarified[1] on this point we work around this by ensuring the animation's startTime is set to something non-null before setting it to null."
> 
> [1] https://github.com/w3c/csswg-drafts/issues/2691

Oops,

"...the change will be ignored and then animation will continue to be play-pending..."

should be:

"...the change will be ignored and the animation will continue to be play-pending..."
Brian knows much more than I do on the specifics related to playing/pausing the animations. So please do address his comments.
I'm fine with the rest of the changes in this latest commit.
R+!
Flags: needinfo?(pbrosset)
Thank you very much, Brian, Patrick!
I fix along to Brian's comment.
Comment on attachment 8975758 [details]
Bug 1456857 - Part 1: Pause/Play animations respectively.

https://reviewboard.mozilla.org/r/243970/#review251702

::: devtools/server/actors/animation.js:948
(Diff revision 4)
> +    if (!player.playbackRate) {
> +      // We can not play with playbackRate zero.
> +      return;
> -  }
> +    }
> +
> +    // Play animation using startTime. Because the Animation.Play() method recalculates
> +    // startTime and currentTime on the procedure, but we need to keep the currentTime.
> +    const currentTime = player.currentTime || 0;
> +    player.startTime = player.timeline.currentTime - currentTime / player.playbackRate;

I suppose it might be possible in future for an animation to have no timeline or an inactive timeline so perhaps we could add another early return towards the start of the method that just does:

  if (!player.timeline || player.timeline.currentTime === null) {
    return;
  }

(But perhaps we ignore such animations earlier on so this isn't necessary, I'm not sure.)

::: devtools/server/actors/animation.js:953
(Diff revision 4)
> +    // Play animation using startTime. Because the Animation.Play() method recalculates
> +    // startTime and currentTime on the procedure, but we need to keep the currentTime.

I don't think the second sentence here makes sense? We don't call play() I think the point of this method is just to do a synchronous play?

So perhaps we could make the comment:

"Play animation in a synchronous fashion by setting the start time directly."
Attachment #8975758 - Flags: review?(bbirtles) → review+
Comment on attachment 8975758 [details]
Bug 1456857 - Part 1: Pause/Play animations respectively.

https://reviewboard.mozilla.org/r/243970/#review251706

::: devtools/server/tests/browser/browser_animation_playPauseSeveral.js:23
(Diff revision 4)
> -  await animations.pauseAll();
> +  animations.pauseAll();
>    await checkStates(walker, animations, ALL_ANIMATED_NODES, "paused");
>  
>    info("Play all animations in the test document");
> -  await animations.playAll();
> +  animations.playAll();
>    await checkStates(walker, animations, ALL_ANIMATED_NODES, "running");
>  
>    info("Pause all animations in the test document using toggleAll");
> -  await animations.toggleAll();
> +  animations.toggleAll();
>    await checkStates(walker, animations, ALL_ANIMATED_NODES, "paused");
>  
>    info("Play all animations in the test document using toggleAll");
> -  await animations.toggleAll();
> +  animations.toggleAll();

As discussed, we should check if these actually need to be async or not.
Comment on attachment 8975758 [details]
Bug 1456857 - Part 1: Pause/Play animations respectively.

https://reviewboard.mozilla.org/r/243970/#review251706

> As discussed, we should check if these actually need to be async or not.

Yeah, sorry sorry,, These functions were of AnimationFront. So we need. Thanks!
(However, I don't know why we could pass the tests in try..)
Comment on attachment 8975758 [details]
Bug 1456857 - Part 1: Pause/Play animations respectively.

https://reviewboard.mozilla.org/r/243970/#review251702

> I suppose it might be possible in future for an animation to have no timeline or an inactive timeline so perhaps we could add another early return towards the start of the method that just does:
> 
>   if (!player.timeline || player.timeline.currentTime === null) {
>     return;
>   }
> 
> (But perhaps we ignore such animations earlier on so this isn't necessary, I'm not sure.)

Please let me do later, since we should process other points as well at the time we need, I think.

> I don't think the second sentence here makes sense? We don't call play() I think the point of this method is just to do a synchronous play?
> 
> So perhaps we could make the comment:
> 
> "Play animation in a synchronous fashion by setting the start time directly."

Yes, thansk.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15ca181d1924
Part 1: Pause/Play animations respectively. r=birtles,pbro
https://hg.mozilla.org/integration/autoland/rev/a7e3919c4817
Part 2: Refer to proper props. r=gl
https://hg.mozilla.org/integration/autoland/rev/29e8e11dd4e0
Part 3: Add a test for pausing/playing animations. r=pbro
Verified as fixed on Nightly 62.0a1(2018-05-28)
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
Product: Firefox → DevTools
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: