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)

Product:
DevTools
Component:
Inspector: Animations
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 fixed, relnote-firefox 45+)

Status:
RESOLVED FIXED
Milestone:
Firefox 45
Tracking Flags:
Tracking Status
firefox45 --- fixed
relnote-firefox --- 45+

People

(Reporter: pbro, Assigned: pbro)

References

Details

(Keywords: user-doc-needed)

Attachments

(1 file, 2 obsolete files)

Bug_1211801_-_Add_a_playback_rate_selector_to_the_.diff
(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.
Depends on: 1205681
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Blocks: 1211886
Blocks: 985861
No longer blocks: 1153271
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)
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
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)
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+
No longer blocks: 1211886
(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.
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/56e48254df7c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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
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: --- → ?
Added to the release notes with Patrick's wording (merci!) Having some docs or video would be great.
relnote-firefox: ? → 45+
Keywords: user-doc-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: