Closed
Bug 1211801
Opened 9 years ago
Closed 9 years ago
Add playback rate selection to the animation inspector panel v3
Categories
(DevTools :: Inspector: Animations, defect)
DevTools
Inspector: Animations
Tracking
(firefox45 fixed, relnote-firefox 45+)
RESOLVED
FIXED
Firefox 45
People
(Reporter: pbro, Assigned: pbro)
References
Details
(Keywords: user-doc-needed)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
The v3 of the animation-inspector panel has landed in FF43. This means we now have a toolbar above the timeline that can be used to control the animations displayed in the timeline.
We should add a playback rate selector in this toolbar.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
I think this can go through a first round of review.
I've added tests.
The code changes are quite small, but I'm especially interested in getting someone else to play with the UI a bit and report problems.
Things you need to know:
- the plabackrate selector is in the animation-inspector toolbar, next to the rewind and play buttons
- it's a drop-down, but I made it look like the other buttons in the toolbar
- choosing a different value in the drop-down will set the playback rate on all animations currently displayed in the timeline
- this means that you can, for instance, select just one animated element and set its playback rate independently from other elements
- you can then also select <body> for instance and set the rate for all nested animated elements at once
- you can change the rate whether the animations are playing or paused.
One page I often use to test these kinds of features is: http://tympanus.net/Development/DialogEffects/ricky.html
because the animation-inspector is most suited for inspecting these kinds of animations.
But it should work on any kind of animations.
Attachment #8674122 -
Flags: review?(zer0)
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Last try build had some failing tests. These are due to the new test I added. This test simulates mousedown/up events somewhere over the animation panel and that, somehow, causes later tests to receive mouseover events which make them fail.
I made a fix (patch not uploaded yet): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c28b5593837
Assignee | ||
Comment 4•9 years ago
|
||
Here's an updated patch with the test fix mentioned earlier.
Matteo, your input on this would be great. If possible, I'd like to land this in 44.
Attachment #8674122 -
Attachment is obsolete: true
Attachment #8674122 -
Flags: review?(zer0)
Attachment #8676200 -
Flags: review?(zer0)
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8676200 [details] [diff] [review]
Bug_1211801_-_Add_a_playback_rate_selector_to_the_.diff
Matteo seems busy right now, Mike has agreed to take a look at this.
Attachment #8676200 -
Flags: review?(zer0) → review?(mratcliffe)
Comment on attachment 8676200 [details] [diff] [review]
Bug_1211801_-_Add_a_playback_rate_selector_to_the_.diff
Review of attachment 8676200 [details] [diff] [review]:
-----------------------------------------------------------------
Looks great.
It is up to you whether to change your catches but please remove the whitespace.
I have already logged bug 1218760 for creating no-trailing-whitespace.
::: devtools/client/animationinspector/animation-panel.js
@@ +120,5 @@
> gToolbox.target.on("navigate", this.onTabNavigated);
>
> this.animationsTimelineComponent.on("timeline-data-changed",
> this.onTimelineDataChanged);
> +
Whitespace?
@@ +142,5 @@
> gToolbox.target.off("navigate", this.onTabNavigated);
>
> this.animationsTimelineComponent.off("timeline-data-changed",
> this.onTimelineDataChanged);
> +
Whitespace?
@@ +201,5 @@
> + */
> + onRateChanged: function(e, rate) {
> + AnimationsController.setPlaybackRateAll(rate)
> + .then(() => this.refreshAnimationsStateAndUI())
> + .catch(e => console.error(e));
You can use:
.catch(console.error);
::: devtools/client/animationinspector/components.js
@@ +235,4 @@
> return;
> }
> + this.highlighterUtils.highlightNodeFront(this.nodeFront)
> + .catch(e => console.error(e));
You can use:
.catch(console.error);
@@ +243,4 @@
> return;
> }
> + this.highlighterUtils.unhighlight()
> + .catch(e => console.error(e));
You can use:
.catch(console.error);
Attachment #8676200 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> Comment on attachment 8676200 [details] [diff] [review]
> Bug_1211801_-_Add_a_playback_rate_selector_to_the_.diff
>
> Review of attachment 8676200 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Looks great.
>
> It is up to you whether to change your catches but please remove the
> whitespace.
>
> I have already logged bug 1218760 for creating no-trailing-whitespace.
Sorry about the whitespaces, this must have come from a manual merge in kdiff3 (in sublime I have both the eslint rule enabled to warn me about them *and* auto trailing-whitespace removal on save).
About catch(console.error), I can't do this as this would lead to this error: "TypeError: 'error' called on an object that does not implement interface Console.", so catch(e => console.error(e)) is required here.
Assignee | ||
Comment 8•9 years ago
|
||
Addressed review feedback. Rebased.
Pushed to try one last time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68c53bf802a9
Attachment #8676200 -
Attachment is obsolete: true
Attachment #8681888 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Comment 10•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment 11•9 years ago
|
||
bugherder uplift |
status-b2g-v2.5:
--- → fixed
Comment 12•9 years ago
|
||
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Assignee | ||
Updated•9 years ago
|
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Assignee | ||
Comment 13•9 years ago
|
||
Release Note Request (optional, but appreciated)
[Why is this notable]: This lets users of the firefoxdevtools change the playback rate of css animations
[Suggested wording]: The playback rate of animations displayed in the animation-inspector's timeline can now be changed. Making animations run slower is useful to fine tune them.
[Links (documentation, blog post, etc)]:
relnote-firefox:
--- → ?
Comment 14•9 years ago
|
||
Added to the release notes with Patrick's wording (merci!)
Having some docs or video would be great.
Keywords: user-doc-needed
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•