1211886 - Infinite animations that are past their first iterations can't be paused/played

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

STR:
- go to http://bgrins.github.io/devtools-demos/inspector/animation-timing.html
- open the animation-inspector
- wait until all animations have completed their first iteration (i.e. the scrubber has gone beyond the timelines in the panel)
- try to use the pause button in the toolbar

Expected: the pause button still looks like a pause button and clicking it actually pauses the animations displayed in the timeline.

Actual: once the scrubber reaches the end of the timeline, the button turns to its play mode again, and clicking it has no effect on the animations.

Comment 1

[Patrick Brosset <:pbro>]

I have a fix for this, build on top of a rather long commit series that ends with bug 1211801, so making this depend on bug 1211801.

Comment 2

[Patrick Brosset <:pbro>]

No need to wait for bug 1211801 which I'll land later after all. This is small annoying bug that should be fixed asap. Will attach a patch later.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug_1211886_-_Make_infinite_animations_that_iterat.diff

The code change itself is pretty simple: if there are infinite animations displayed in the timeline, we let the scrubber move even past the timeline bounds. This way there's no doubt possible when deciding if the button should be in paused or playing state.
I added more test cases for this.

@Mike: this is a relatively urgent review request, this bug has been noticed when working on our FF44 campaign demo material, so it'd be great to fix it before 44.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8679968 [details] [diff] [review]
Bug_1211886_-_Make_infinite_animations_that_iterat.diff

Wrong patch, sorry.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1211886.patch

Comment 6

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d6f2e1ecb1
This try build is catching some weird, seemingly unrelated, leaks in other devtools tests.
Since they seem really unrelated and I don't see how this patch could cause them, I'm going to request check-in.

Comment 7

[Patrick Brosset <:pbro>]

Got confirmation today that the leaks in the previous try builds are unrelated. Landing this now:
https://hg.mozilla.org/integration/fx-team/rev/a88a1f9ba215

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a88a1f9ba215

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8679969 [details] [diff] [review]
bug1211886.patch

Approval Request Comment

[Feature/regressing bug #]: This is a bug from the original animation-panel timeline implementation in bug 1153271 and bug 1155661.

[User impact if declined]: If declined, the UI of the animation-panel (which is one of the tools demo'd heavily in FF44 marketing campaign) will be wrong for infinite animations. Indeed, the pause button will go back to play state after a while even if the animation is still playing.

[Describe test coverage new/current, TreeHerder]: Added a new browser devtools mochitest for this.

[Risks and why]: There are quite a few tests for the animation UI, so this is low risk, I don't think this introduced any new regressions. If it did, it will be something related to the play/pause button in the animation-inspector. So it's quite contained.

[String/UUID change made/needed]: None.

Comment 10

[Patrick Brosset <:pbro>]

Alright, it seems we have "blanket approval" for devtools polish bugs for dev-edition 44 (and this one is a pretty important one for the animation-inspector which is demo'd in the marketing campaign). So here it goes:

https://hg.mozilla.org/releases/mozilla-aurora/rev/56e2e69aea4d

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a88a1f9ba215

Comment 12

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/56e2e69aea4d