Bug 159315 – Web Inspector: Add visual editor for spring() timing-function

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

We should add a visual editor like we have for the cubic-bezier editor.

[Radar WebKit Bug Importer]

<rdar://problem/27114739>

[Devin Rousso]

Created attachment 286029 [details]
Patch

[Devin Rousso]

Created attachment 286030 [details]
[Image] After Patch is applied

This will appear inside "Style - Rules", "Style - Visual", and files in the Resources tab

[Joseph Pecoraro]

Comment on attachment 286029 [details]
Patch

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

Cool! I think this is a great first pass. I'd like to get someone more familiar with spring animations to review. I just played with it briefly and it looks good to me.

Ultimately it would be really nice to show the spring curve when changing values. That makes it much easier to see what changes when you toggle values, without having to watch the ball animation, change a value, watch the animation again, and try to determine what changed. I do think the ball animation is worth keeping. It may even make sense to make it more interactive. Often times spring animations are used for interactive animations. Clicking to move the ball between different points, like the default example at <https://webkit.org/demos/spring/>, will be a quick way for developers to better feel and understand the values.

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:518
> +        matches.splice(0, 1);
> +        return WebInspector.Spring.fromValues(matches);

Perhaps an easier way to do this would be to slice, not splice:

    return WebInspector.Spring.fromValues(matches.slice(1));

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:531
> +        let values = [this.mass, this.stiffness, this.damping, this.initialVelocity];
> +        return `spring(${values.join(" ")})`;

Avoiding the array and join might be easier to read and be less typing:

    return `spring(${this.mass} ${this.stiffness} ${this.damping} ${this.initialVelocity})`;

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1330
> +                    if (type !== WebInspector.TextMarker.Type.Color
> +                    && type !== WebInspector.TextMarker.Type.Gradient
> +                    && type !== WebInspector.TextMarker.Type.CubicBezier
> +                    && type !== WebInspector.TextMarker.Type.Spring) {

Style: The "&&" lines should be indented one.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorSpringEditingController.js:31
> +    constructor(codeMirror, marker)
> +    {
> +        super(codeMirror, marker);
> +    }

Style: Since this constructor does nothing but call the super constructor, you can drop it entirely and it will just work! The default constructor ends up being: constructor() { super(...arguments); }

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorTextMarkers.js:201
> +    var springRegex = /(spring\([^)]+\))/g;

Style: `let`, `const` , or inline it!

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.css:3
> + * Copyright (C) 2016 Devin Rousso <[e-mail]>. All rights reserved.

Nit: Might as well make this 2015-2016.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:295
> -    Bezier: "inline-swatch-type-bezier"
> +    Bezier: "inline-swatch-type-bezier",
> +    Spring: "inline-swatch-type-spring"

This is why I suggest adding the trailing comma, so that future diffs are nicer!

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.css:52
> +@keyframes springPreview {

Style: All other inspector animation names are lowercase "-" separated. You could name this "spring-preview".

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:62
> +        createInputsForParameter.call(this, "mass", WebInspector.UIString("Mass"));
> +        this._massInput.min = this._massSlider.min = 0;

Mass must be greater than 0, it should not be equal to zero. Lets make the min 1.

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:65
> +        createInputsForParameter.call(this, "stiffness", WebInspector.UIString("Stiffness"));
> +        this._stiffnessInput.min = this._stiffnessSlider.min = 0;

Stiffness must be greater than 0, it should not be equal to zero. Lets make the min 1.

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:70
> +        createInputsForParameter.call(this, "initialVelocity", WebInspector.UIString("Initial Velocity"));

Initial Velocity can be negative. It may make sense to provide a -100...100 slider with the default being 0 in the middle.

My naive thinking is that developers will rarely need to input an initial velocity in the popover, so its fine if the animation preview doesn't look optimal for negative values.

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:100
> +    get spring()

Style: Put getters before setters.

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:155
> +            if (value <= 0)
> +                value = 1;
> +            this._spring.mass = this._massInput.value = this._massSlider.value = value;
> +            break;

Love the `a = b = c` style! You could also inline the min value. `a = b = c = Math.min(1, value)`, which makes the checks clearer anyways.

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:183
> +        // Add extra 25% to ensure that the "delay" time of the animation does not shorten the spring

Style: End comment with a period.

Adding 25% might be overkill. For a very slow spring, 25% could be massive. It may be better to add a fixed 1s or 2s to the duration.

Given you have duration, does that seem worth displaying somewhere? In testing I had some seemingly very slow springs. Showing a running clock 0...5.5 out of 5.5 timer would have been nice so I could see that it was running!

[Devin Rousso]

Comment on attachment 286029 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:183
>> +        // Add extra 25% to ensure that the "delay" time of the animation does not shorten the spring
> 
> Style: End comment with a period.
> 
> Adding 25% might be overkill. For a very slow spring, 25% could be massive. It may be better to add a fixed 1s or 2s to the duration.
> 
> Given you have duration, does that seem worth displaying somewhere? In testing I had some seemingly very slow springs. Showing a running clock 0...5.5 out of 5.5 timer would have been nice so I could see that it was running!

Is it possible to add a fixed amount to an animation that is iterating infinitely?  That is the reason that I add the 25% "padding" (see "spring-preview" in SpringEditor.css).

[Devin Rousso]

Created attachment 286253 [details]
Patch

[Devin Rousso]

Created attachment 286254 [details]
[Image] After Patch is applied

[Devin Rousso]

Created attachment 286255 [details]
Patch

[Devin Rousso]

Created attachment 286323 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 286323 [details]
Patch

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

r=me! I didn't apply this new version, but the code changes look good to me.

> Source/WebInspectorUI/ChangeLog:109
> +        * UserInterface/Views/VisualStyleTimingEditor.js:

Everywhere where there are newlines in this ChangeLog is an opportunity to provide a description about the changes you've added. Particularly for new files.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1041
> +    value: function(decimals)

Style: value(decimals)

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1043
> +        let power = 10 ** decimals;

I didn't know we supported exponentiation yet! Wow!

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorSpringEditingController.js:28
> +    constructor(codeMirror, marker)

You can drop this constructor, the default should work just fine.

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:485
> +        this.mass = mass > 0 ? mass : 1;
> +        this.stiffness = stiffness > 0 ? stiffness : 1;

I think Math.max(1, ...) would be clearer here.

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:501
> +        return new WebInspector.Spring(values[0], values[1], values[2], values[3]);

Style: new WebInspector.Spring(...values)

> Source/WebInspectorUI/UserInterface/Models/TextMarker.js:87
> +    Spring: "text-marker-type-spring"

Style: Add trailing comma.

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.css:72
> +    content: attr(data-duration) "s";

Clever!

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:63
> +            this[`_${id}Slider`] = row.createChild("input");
> +            this[`_${id}Slider`].type = "range";
> +            this[`_${id}Slider`].addEventListener("input", this._handleNumberSliderInput.bind(this));
> +            this[`_${id}Slider`].addEventListener("mousedown", this._handleNumberSliderMousedown.bind(this));
> +            this[`_${id}Slider`].addEventListener("mouseup", this._handleNumberSliderMouseup.bind(this));
> +
> +            this[`_${id}Input`] = row.createChild("input");
> +            this[`_${id}Input`].type = "number";
> +            this[`_${id}Input`].addEventListener("input", this._handleNumberInputInput.bind(this));
> +            this[`_${id}Input`].addEventListener("keydown", this._handleNumberInputKeydown.bind(this));

Nit: I don't like the idea of unnecessarily generating these property key strings lots of times. lets put them into a local.

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:169
> +            this._spring.damping = Math.max(1, value);

Damping minimum is 0.

[Devin Rousso]

Created attachment 286684 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286684 [details]
Patch

Clearing flags on attachment: 286684

Committed r204775: <http://trac.webkit.org/changeset/204775>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.