Devtools Animations: Add cubic bezier easing editor for animations

Devtools Animations: Add cubic bezier easing editor for animations

This change adds an extra icon which is visible on hover of an animation.
This icon allows you to edit the cubic bezier editor and edit the easing
of the animation in real time.

BUG=447083

[samli, 2015-05-22 07:49:13]

samli@chromium.org changed reviewers:
+ pfeldman@chromium.org

[samli, 2015-05-22 07:49:14]

ptal

Screencast: http://youtu.be/jqe5Q5ScYec

[dgozman, 2015-05-23 00:53:26]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2015-05-23 00:53:26]

https://codereview.chromium.org/1155773002/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/1155773002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:281: if
(RefPtr<TimingFunction> timingFunction =
AnimationInputHelpers::parseTimingFunction(easing))
Code up to this line may be extracted into helper function and reused in
setTiming function.

https://codereview.chromium.org/1155773002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1155773002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/elements/AnimationTimeline.js:696:
this._stylesPopoverHelper = new WebInspector.StylesPopoverHelper();
Animations should not depend on StylesPopoverHelper. I think you don't even need
it. Use plain Popover instead.

[samli, 2015-05-25 01:55:38]

ptal

https://codereview.chromium.org/1155773002/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/1155773002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:281: if
(RefPtr<TimingFunction> timingFunction =
AnimationInputHelpers::parseTimingFunction(easing))
On 2015/05/23 00:53:26, dgozman_ooo wrote:
> Code up to this line may be extracted into helper function and reused in
> setTiming function.

Done.

https://codereview.chromium.org/1155773002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1155773002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/elements/AnimationTimeline.js:696:
this._stylesPopoverHelper = new WebInspector.StylesPopoverHelper();
On 2015/05/23 00:53:26, dgozman_ooo wrote:
> Animations should not depend on StylesPopoverHelper. I think you don't even
need
> it. Use plain Popover instead.

Done. I've had to duplicate some of the code, ptal. Refactored so that the
timeline holds one editor/popover which is shared between all the animation ui
elements.

[dgozman, 2015-05-26 12:02:24]

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:156: animationType =
cssAnimations.isAnimationForInspector(player) ? AnimationType::CSSAnimation :
AnimationType::WebAnimation;
This looks suspicious. If it was wrong, we have no test coverage - let's add
tests.

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:280: // Update easing of the
transition, represented by the second keyframe
Full stop please.

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:282:
newFrames[1]->setEasing(timingFunction);
If not parsed correctly, return errorString.

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:287: }
else fill errorString.

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/elements/AnimationTimeline.js:1066: if (!this._node ||
this._animation.source().easing() == value)
===

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/elements/AnimationTimeline.js:1076:
target.animationAgent().setEasing(this._animation.id(), value);
UI should not call animationAgent() and operate with target. Instead, we should
have |this._animationModel| and call methods on it.

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/elements/AnimationTimeline.js:1194:
WebInspector.AnimationTimeline.BezierEditor.prototype = {
That's quite a lot of code duplication. Perhaps, we should rename
StylesPopoverHelper into EditorPopoverHelper, place it in ui module and reuse
here. Sorry for not spotting this earlier.

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
File Source/devtools/front_end/elements/animationTimeline.css (right):

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/elements/animationTimeline.css:338: .bezier-icon.shown
{
We usually use ".hidden" instead of ".shown". Doesn't it work for you here?

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/AnimationModel.js (right):

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/sdk/AnimationModel.js:364: this._easing = value;
This should call protocol method.

[samli, 2015-05-27 05:21:59]

PTAL

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:156: animationType =
cssAnimations.isAnimationForInspector(player) ? AnimationType::CSSAnimation :
AnimationType::WebAnimation;
On 2015/05/26 at 12:02:23, dgozman wrote:
> This looks suspicious. If it was wrong, we have no test coverage - let's add
tests.

Done.

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:280: // Update easing of the
transition, represented by the second keyframe
On 2015/05/26 at 12:02:23, dgozman wrote:
> Full stop please.

Done.

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:282:
newFrames[1]->setEasing(timingFunction);
On 2015/05/26 at 12:02:23, dgozman wrote:
> If not parsed correctly, return errorString.

Done.

https://codereview.chromium.org/1155773002/diff/60001/Source/core/inspector/I...
Source/core/inspector/InspectorAnimationAgent.cpp:287: }
On 2015/05/26 at 12:02:23, dgozman wrote:
> else fill errorString.

Done.

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/elements/AnimationTimeline.js:1066: if (!this._node ||
this._animation.source().easing() == value)
On 2015/05/26 at 12:02:23, dgozman wrote:
> ===

Done.

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/elements/AnimationTimeline.js:1076:
target.animationAgent().setEasing(this._animation.id(), value);
On 2015/05/26 at 12:02:23, dgozman wrote:
> UI should not call animationAgent() and operate with target. Instead, we
should have |this._animationModel| and call methods on it.

Ack.

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/elements/AnimationTimeline.js:1194:
WebInspector.AnimationTimeline.BezierEditor.prototype = {
On 2015/05/26 at 12:02:23, dgozman wrote:
> That's quite a lot of code duplication. Perhaps, we should rename
StylesPopoverHelper into EditorPopoverHelper, place it in ui module and reuse
here. Sorry for not spotting this earlier.

Reluctantly done.

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
File Source/devtools/front_end/elements/animationTimeline.css (right):

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/elements/animationTimeline.css:338: .bezier-icon.shown
{
On 2015/05/26 at 12:02:23, dgozman wrote:
> We usually use ".hidden" instead of ".shown". Doesn't it work for you here?

In this case it wouldn't since I want to animate the opacity and it has a long
delay before hiding.

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/AnimationModel.js (right):

https://codereview.chromium.org/1155773002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/sdk/AnimationModel.js:364: this._easing = value;
On 2015/05/26 at 12:02:24, dgozman wrote:
> This should call protocol method.

Done.

[samli, 2015-05-28 07:28:48]

ping

[dgozman, 2015-05-28 11:04:04]

https://codereview.chromium.org/1155773002/diff/100001/Source/core/inspector/...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/1155773002/diff/100001/Source/core/inspector/...
Source/core/inspector/InspectorAnimationAgent.cpp:156: animationType =
cssAnimations.isAnimationForInspector(player) ? AnimationType::CSSAnimation :
AnimationType::WebAnimation;
What I meant is to add tests that cover the functionality. For example,
|setTiming| method didn't work for WebAnimation (because it was marked as
CSSAnimation), right? We should cover existing functionality first before adding
more methods, since it will be harder to test everything later.

https://codereview.chromium.org/1155773002/diff/100001/Source/core/inspector/...
Source/core/inspector/InspectorAnimationAgent.cpp:292: *errorString = "Given
animation is not a transition or web animation.";
I think, we should have similar error reporting everywhere in protocol methods.

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/elements/AnimationTimeline.js:1065: _toggleBezierIcon:
function(show)
Code below this line looks like exact copy of BezierPopoverIcon. Why not reuse
it? The only difference is how to set value (|_setEasing| method here), we can
make it a parameter.

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/sdk/AnimationModel.js (right):

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/sdk/AnimationModel.js:367: if (callProtocol)
What's the usecase for callProtocol == false?
When user changed the value, you have to call protocol method, right?

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/ui/EditorPopoverHelper.js (right):

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/ui/EditorPopoverHelper.js:44: anchorElement:
function()
Who uses this?

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/ui/EditorPopoverHelper.js:75: this._scrollerElement =
anchorElement.enclosingNodeOrSelfWithClass("style-panes-wrapper");
I guess this doesn't work in animation timeline. We should pass it as a
parameter.

[samli, 2015-05-28 11:48:14]

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/sdk/AnimationModel.js (right):

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/sdk/AnimationModel.js:367: if (callProtocol)
On 2015/05/28 11:04:03, dgozman wrote:
> What's the usecase for callProtocol == false?
> When user changed the value, you have to call protocol method, right?

CSS animations respond to updated styles and hence don't require a protocol
call. Invoking protocol will result in the protocol error we just added.

We could alternatively allow it to always call protocol and do nothing for CSS
animations.

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/ui/EditorPopoverHelper.js (right):

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/ui/EditorPopoverHelper.js:44: anchorElement:
function()
On 2015/05/28 11:04:03, dgozman wrote:
> Who uses this?

I used this in AnimationTimeline to allow us to reuse the same
EditorPopoverHelper but close and immediately reopen on a different
anchorElement.

[samli, 2015-05-29 04:32:12]

https://codereview.chromium.org/1155773002/diff/100001/Source/core/inspector/...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/1155773002/diff/100001/Source/core/inspector/...
Source/core/inspector/InspectorAnimationAgent.cpp:292: *errorString = "Given
animation is not a transition or web animation.";
On 2015/05/28 11:04:03, dgozman wrote:
> I think, we should have similar error reporting everywhere in protocol
methods.

Acknowledged.

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/elements/AnimationTimeline.js:1065: _toggleBezierIcon:
function(show)
On 2015/05/28 11:04:03, dgozman wrote:
> Code below this line looks like exact copy of BezierPopoverIcon. Why not reuse
> it? The only difference is how to set value (|_setEasing| method here), we can
> make it a parameter.

This function is not in BezierPopoverIcon.

BezierPopoverIcon uses StylePropertyTreeElement throughout. Toggle behavior is
subtly different. Styles/timeline will have different icon behavior. The
timeline one has a fade in, fade out and enabled states. We'd have to refactor
out and use inheritance and I don't see how that's going to be an advantage.

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/ui/EditorPopoverHelper.js (right):

https://codereview.chromium.org/1155773002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/ui/EditorPopoverHelper.js:75: this._scrollerElement =
anchorElement.enclosingNodeOrSelfWithClass("style-panes-wrapper");
On 2015/05/28 11:04:03, dgozman wrote:
> I guess this doesn't work in animation timeline. We should pass it as a
> parameter.

Done.

[samli, 2015-06-04 08:08:04]

Rebased over http://crrev.com/1154083005

TODO: Add a test for setTiming/setEasing, but I ran out of time today.

[pfeldman, 2015-06-29 12:14:02]

pfeldman@chromium.org changed reviewers:
- pfeldman@chromium.org

[dgozman, 2015-06-30 14:28:19]

dgozman@chromium.org changed reviewers:
- dgozman@chromium.org

[samli, 2015-07-06 08:16:39]

samli@chromium.org changed reviewers:
+ dgozman@chromium.org

[samli, 2015-07-06 08:16:39]

Ping. Likewise, a large portion of this is still relevant.

[dgozman, 2015-07-08 14:09:21]

On 2015/07/06 08:16:39, samli wrote:
> Ping. Likewise, a large portion of this is still relevant.

What do you mean by large portion? Perhaps, patch  should be updated then?

[samli, 2015-07-09 05:49:29]

On 2015/07/08 at 14:09:21, dgozman wrote:
> On 2015/07/06 08:16:39, samli wrote:
> > Ping. Likewise, a large portion of this is still relevant.
> 
> What do you mean by large portion? Perhaps, patch  should be updated then?

I'm not sure what is acceptable and what isn't. I would like to be able to
update inline style for now and have this functionality there, but pfeldman@ has
blocked me in the past.

[pfeldman, 2015-07-09 05:59:23]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2015-07-09 05:59:24]

Sam, given that we are re-implementing this feature, I don't want to do any
reviews until we have a clear plan for action that we agree upon. I thought it
was clear. The effort should start with the product story that we can agree
upon, followed by a brief design doc. What I don't want to see is another
iteration of the implementation that goes nowhere.

My goal here is to get the right thing shipped, but I haven't seen the right
thing defined yet. Your design doc has too many open questions. You call some
things minor, other things as discussed with Paul. But that all does not help
unless the reviewers are in agreement with that. So I would recommend that you
address open questions in the doc first, get back to reviewers with the mocks of
the intended functionality. Once we agree on that and discuss the design, we can
start landing patches.

[samli, 2015-07-09 07:18:03]

On 2015/07/09 at 05:59:24, pfeldman wrote:
> Sam, given that we are re-implementing this feature, I don't want to do any
reviews until we have a clear plan for action that we agree upon. I thought it
was clear. The effort should start with the product story that we can agree
upon, followed by a brief design doc. What I don't want to see is another
iteration of the implementation that goes nowhere.

This was 100% not clear at all. Two weeks ago when we chatted over VC, you
mentioned that I should go away and start prototyping this to get a sense of how
this would actually feel as a user. From Paul's meeting notes: "Start with the
capture. Edit+rules as 2nd phase. Sam: prototype the experiment with identity
matching and effect buffer." The takeaway from this meeting was that there were
some ideas around the editing aspect of the implementation, but the proposal
regarding the viewing of the effects was given the green light to begin with an
implementation before a re-evaluation once we had something to tangibly play
with. My interpretation of resolution was that I would implement and check-in
this updated viewing model. Clearly this was not your interpretation of the
result of this meeting, what did you think was the result of the meeting and
what I should have been doing instead?

I don't think these are 'iterations which have gone nowhere'. There have been
continual discussions, design docs and action plans. What has been the clear
story here is that despite all reviewers being in agreement on a design, we were
not able to effectively evaluate what we had designed without something to play
with. I don't think this has all been in vain and that we could have arrived at
where we are today without ever having prototyped something first.

> 
> My goal here is to get the right thing shipped, but I haven't seen the right
thing defined yet. Your design doc has too many open questions. You call some
things minor, other things as discussed with Paul. But that all does not help
unless the reviewers are in agreement with that. So I would recommend that you
address open questions in the doc first, get back to reviewers with the mocks of
the intended functionality. Once we agree on that and discuss the design, we can
start landing patches.

I have only worked on implementing parts of this design which do not have any
open questions and given the dialogue has been historically slow, it seems
unproductive to block myself from doing any work waiting on the entire design to
be specified and reviewed.  If you have certain open questions/issues which you
deem as blocking, please don't think this is immediately obvious to me, because
I think there will always be a given number of open questions and this is okay -
some of these aspects require experimentation to be able to properly evaluate.

I'm happy to close this patch and defer this feature till later. I've addressed
the open questions in the doc and added more detailed implementation design at
the bottom. Could you detail exactly what aspects of the design you would like
to see resolved now and what is missing?