Bug 154738 - Web Inspector: Custom transition bezier curve editor preview should loop when not editing curve
Summary: Web Inspector: Custom transition bezier curve editor preview should loop when...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-02-26 10:55 PST by Timothy Hatcher
Modified: 2016-02-28 22:23 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.78 KB, patch)
2016-02-26 19:09 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2016-02-26 10:55:52 PST
Right now the only way I can see to preview the animation is to edit the curve or click on the animation itself.

It would be nice if it looped when editing was inactive. Clicking could toggle the animation looping?

<rdar://problem/24861563>
Comment 1 Devin Rousso 2016-02-26 13:18:18 PST
(In reply to comment #0)
> Right now the only way I can see to preview the animation is to edit the
> curve or click on the animation itself.

I may be misunderstanding what you mean by "animation itself", but clicking on the preview area will restart the animation (regardless of its current progress).

> It would be nice if it looped when editing was inactive. Clicking could
> toggle the animation looping?

So instead of clicking to restart, make clicking toggle looping? Why not just make it loop by default and allow clicking to restart?
Comment 2 Timothy Hatcher 2016-02-26 14:14:22 PST
(In reply to comment #1)
> (In reply to comment #0)
> > Right now the only way I can see to preview the animation is to edit the
> > curve or click on the animation itself.
> 
> I may be misunderstanding what you mean by "animation itself", but clicking
> on the preview area will restart the animation (regardless of its current
> progress).
> 
> > It would be nice if it looped when editing was inactive. Clicking could
> > toggle the animation looping?
> 
> So instead of clicking to restart, make clicking toggle looping? Why not
> just make it loop by default and allow clicking to restart?

Looping by default would likely be fine.
Comment 3 Devin Rousso 2016-02-26 19:09:26 PST
Created attachment 272391 [details]
Patch
Comment 4 Joseph Pecoraro 2016-02-26 21:07:50 PST
Comment on attachment 272391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272391&action=review

Yay! I wanted this the last time I used it.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:41
> -        this._bezierPreviewAnimationStyleText = "bezierPreview 2s 250ms forwards ";
> +        this._bezierPreviewAnimationStyleText = "bezierPreview 2.5s 250ms infinite ";

Is this member necessary?

It seems to be used here:

        this._bezierPreview.style.animation = this._bezierPreviewAnimationStyleText + this._bezier.toString();

But perhaps we can just use the CSS version and set the bezier.toString on the more specific property:

        this._bezierPreview.style.animationTimingFunction = this._bezier.toString();

Would this work? It would avoid the duplication, and better match the actual intention.
Comment 5 WebKit Commit Bot 2016-02-26 21:40:45 PST
Comment on attachment 272391 [details]
Patch

Clearing flags on attachment: 272391

Committed r197235: <http://trac.webkit.org/changeset/197235>
Comment 6 WebKit Commit Bot 2016-02-26 21:40:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Devin Rousso 2016-02-28 22:23:03 PST
(In reply to comment #4)
> Comment on attachment 272391 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272391&action=review
> 
> Yay! I wanted this the last time I used it.
> 
> > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:41
> > -        this._bezierPreviewAnimationStyleText = "bezierPreview 2s 250ms forwards ";
> > +        this._bezierPreviewAnimationStyleText = "bezierPreview 2.5s 250ms infinite ";
> 
> Is this member necessary?
> 
> It seems to be used here:
> 
>         this._bezierPreview.style.animation =
> this._bezierPreviewAnimationStyleText + this._bezier.toString();
> 
> But perhaps we can just use the CSS version and set the bezier.toString on
> the more specific property:
> 
>         this._bezierPreview.style.animationTimingFunction =
> this._bezier.toString();
> 
> Would this work? It would avoid the duplication, and better match the actual
> intention.

Nice catch! Not really sure why I didn't do this in the first place :P
Addressed in <https://bugs.webkit.org/show_bug.cgi?id=154809>