Closed Bug 1211886 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Depends on: 1211801
Blocks: de-44-polish
Blocks: 985861
No longer blocks: 1153271
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.
No longer depends on: 1211801
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.
Attachment #8679968 - Flags: review?(mratcliffe)
Comment on attachment 8679968 [details] [diff] [review]
Bug_1211886_-_Make_infinite_animations_that_iterat.diff

Wrong patch, sorry.
Attachment #8679968 - Attachment is obsolete: true
Attachment #8679968 - Flags: review?(mratcliffe)
Attached patch bug1211886.patchSplinter Review
Attachment #8679969 - Flags: review?(mratcliffe)
Attachment #8679969 - Flags: review?(mratcliffe) → review+
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.
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a88a1f9ba215
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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.
Attachment #8679969 - Flags: approval-mozilla-aurora?
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
Attachment #8679969 - Flags: approval-mozilla-aurora?
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.