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
32.77 KB, 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: