Bug 134501 – Web Inspector: Bezier curve visual editor

• Component: Web Inspector
• Status: RESOLVED FIXED

[David Farler]

Bezier curves have lots of numerical components but they have a visual effect - it would be cool to see the curve in a visual editor, maybe with some kind of preview animation to further illustrate what the curve feels like over time.

[Radar WebKit Bug Importer]

<rdar://problem/17523606>

[David Farler]

As an example: https://atom.io/packages/bezier-curve-editor

[Devin Rousso]

Created attachment 257569 [details]
Patch

[Devin Rousso]

Created attachment 257571 [details]
After Patch is applied (Sidebar)

[Devin Rousso]

Created attachment 257572 [details]
After Patch is applied (Resources)

[Joseph Pecoraro]

Comment on attachment 257569 [details]
Patch

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

Neat! Just a lot of nits in review.

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorBezierEditingController.js:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Nit: 2015

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorBezierEditingController.js:54
> +        this._bezierEditor.bezier = this._value;

Nit: _value is managed by the super class, so this sub-class should use the public method: this.value.

> Source/WebInspectorUI/UserInterface/Images/CubicBezier.svg:4
> +    <!-- <path xmlns="http://www.w3.org/2000/svg" d="M0,14 C13,13 3,3 16,2" fill="none" stroke="white" style="stroke-width: 3;"/> -->

This is commented out. Just remove it?

> Source/WebInspectorUI/UserInterface/Images/CubicBezier.svg:5
> +    <path xmlns="http://www.w3.org/2000/svg" d="M2,15.5 C2.5,1 13.5,15 14,0.5" fill="none" stroke="white" style="stroke-width: 3;"/>

Our normal style for paths is always space separated (no commas). So:

    d="M 2 15.5 C 2.5 1 13.5 ..."

See another svg in the Images folder for a reference.

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:335
> +    static fromPoints(points)

"fromPoints" is misleading. These aren't WebInspector.Point objects. Maybe fromCoordinates, fromNumbers, fromFloats?

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

I think you could use the spread operator here:

    return new WebInspector.CubicBezier(...points);

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:355
> +        var matches = trimmedText.match(/^cubic-bezier\(([-\d.]+),([-\d.]+),([-\d.]+),([-\d.]+)\)$/);

In general I worry about having CSS specific knowledge in Geometry.js.

Scientific notation is allowed in these numbers (1e2) though highly unexpected here, note you already toLowerCased the string so you won't see (1E2).

https://developer.mozilla.org/en-US/docs/Web/CSS/timing-function
https://developer.mozilla.org/en-US/docs/Web/CSS/number

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:359
> +        matches.splice(0, 1);

This should probably go through and convert these strings to numbers, otherwise you are relying on implicit coercions for later actions.

Note: parseFloat(...) could work, but it is not as strict as Number(...), so I think Number would work here:

    parseFloat("1e2") // 100
    Number("1e2") // 100

    parseFloat("1e2e") // 100
    Number("1e2e") // NaN

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:380
> +    toString()

Ditto regarding CSS specific knowledge.

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:387
> +            if (!Object.shallowEqual(WebInspector.CubicBezier.keywordValues[key], values))
> +                continue;
> +
> +            return key;

Style: Flip these and the function gets a lot shorter.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.css:32
> +    width: -webkit-calc(100% - 10px);

Nit: Drop the prefix, we support "calc(...)" now.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:45
> +        bezierPreviewContainer.title = WebInspector.UIString("Click to restart the animation.");

Tooltips do not normally end in periods.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:74
> +        this._controls = [];
> +        for (var i = 0; i < 2; ++i) {

Instead of two loops, this might be clearer as a function:

    function createControl(x1, y1) {
        ...
        return {point:null, line, handle}
    }

    this._controls[0] = createControl.call(this, 0, this._bezierHeight);
    this._controls[1] = createControl.call(this, this._bezierWidth, 0);

Likewise the controls don't need to be in an array, it could be this._inControl and this._outControl.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:108
> +        this._bezier = bezier;

Lets console.assert(bezier instanceof WebInspector.CubicBezier)

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:142
> +    _handleMousedown(event)
> +    {
> +        window.addEventListener("mousemove", this, true);

Ignore right click? See other mousedown handles in Web Inspector:

    // Only handle left mouse clicks.
    if (event.button !== 0)
        return;

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:157
> +        this._controls[this._selectedControlIndex].handle.setAttribute("r", this._controlHandleRadius - 2);
> +        this._triggerPreviewAnimation();

Lets clear the selectedControl as well.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:178
> +                this._selectedControlIndex = i;

Seems it would be simpler to store the selectedControl instead of the selected control index.

    this._selectedControl = ...;

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:217
> +        var startPoint = this._controlHandleRadius + "," + (this._bezierHeight + this._controlHandleRadius);
> +
> +        var curvePoints = "";
> +        for (var control of this._controls) {
> +            curvePoints += (control.point.x + this._controlHandleRadius) + "," + (control.point.y + this._controlHandleRadius) + " ";
> +        }
> +
> +        var endPoint = (this._bezierWidth + this._controlHandleRadius) + "," +  this._controlHandleRadius;
> +
> +        this._bezierCurve.setAttribute("d", "M" + startPoint + " C" + curvePoints + endPoint);

This is hard to read with all the string logic. How about building an array and joining with spaces at the end, something like:

    var path = [];
    path.concat(["M", this._controlHandleRadius, this._bezierHeight + this._controlHandleRadius]);
    path.push("C");
    for (var control of this._controls) {
        path.push(control.point.x + this._controlHandleRadius);
        path.push(control.point.y + this._controlHandleRadius);
    }
    path.push(this._bezierWidth + this._controlHandleRadius);
    path.push(this._controlHandleRadius);
    path.join(" ");

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:242
> +        return document.createElementNS("http://www.w3.org/2000/svg", tagName);

We should make this a general utility function instead of just in here.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1417
> +        if (this._cubicBezierEditorPopover)
> +            return;

Ignore right click? Do color markers ignore right click?

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:662
> +                // We're not interested if the color value is not inside a keyword.

Nit: "color value" => "bezier value". I believe this is trying to exclude comments, so maybe that is a better comment.

[Timothy Hatcher]

Very cool!

One small comment, I think the include curve icon in the style sidebar should have similar margins and size to the color swatches.

[Devin Rousso]

Created attachment 257636 [details]
Patch

[Devin Rousso]

Created attachment 257804 [details]
Patch

[Timothy Hatcher]

Comment on attachment 257804 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:897
> +            // Look for color strings and add swatches in front of them.

Look for cubic-bezier and timing functions?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1263
>              // Update the line for any color swatches that got removed.

Add cubic-bezier.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1303
>              // Update the line for any color swatches that got removed.

Add cubic-bezier.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1414
> +    _cubicBezierMarkerClicked(event)

We should find a better way to make this generic enough to share with the color and gradient code.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:633
> +    CodeMirror.defineExtension("createCubicBezierMarkers", function(range, callback) {

This and the ones for color and gradients should move to another file. The others should have never been added here, since they use WebInspector namespaces objects internally, it is a layering violation.

Maybe a CodeMirrorTextMarkers.js file with global functions instead of extending CodeMirror.

[Joseph Pecoraro]

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

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:338
> +    static fromCoordinates(coordinates)
> +    {
> +        if (!coordinates || coordinates.length < 4)
> +            return;

It seems we expect to return a value here, lets return null.

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:340
> +        return new WebInspector.CubicBezier(Number(coordinates[0]), Number(coordinates[1]), Number(coordinates[2]), Number(coordinates[3]));

Might be good to verify none of these are NaN. That would cause confusion later.

    coordinates = coordinates.map((x) => Number(x));
    if (coordinates.includes(NaN))
        return null;

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:346
> +    static fromString(text)
> +    {
> +        if (!text || !text.length)
> +            return;

Same with these, lets return null.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:900
> +                bezierMarker.title = WebInspector.UIString("Click to open a cubic-bezier editor.");

Not sure if we should end this tooltip in a period.

[Devin Rousso]

Created attachment 257861 [details]
Patch

[Brian Burg]

Comment on attachment 257804 [details]
Patch

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

I can't comment on the CodeMirror/TextEditor portions, but the rest looks pretty good. I would like to play with the UI, can you please post a patch that applies to TOT?

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:325
> +WebInspector.CubicBezier = class CubicBezier

When you get a chance, maybe file a follow-up bug to consolidate this with the existing bezier curve solver code.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:35
> +        var width = 184;

editorWidth?

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:75
> +        function createControl(x1, y1)

should this take WebInspector.Point?

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:81
> +            svgGroup.appendChild(line);

I would explicitly set x2 and y2 as well, I have had problems in the past in some engines that do not assume x1 = x2.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:137
> +    handleEvent(event)

This is weird, we usually just attach individual listeners since some elements may already have listeners.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:175
> +        this._selectedControl.handle.setAttribute("r", this._controlHandleRadius - 2);

This seems awkwardly low level, can you fiddle with a "selected" class instead? You can set the 'r' property using CSS.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:186
> +        point.x = clamp(0, point.x - this._controlHandleRadius, this._bezierWidth);

Where did 'clamp' come from?

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:223
> +        path.push("M", this._controlHandleRadius, this._bezierHeight + this._controlHandleRadius);

You should use a template string for this, since it doesn't use a loop.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:394
> +            delete this._cubicBezierEditorPopover;

I think we have changed to never use delete, so do:

this._cubicBezierEditorPopover = undefined;

[Brian Burg]

Comment on attachment 257861 [details]
Patch

Marking patch r-; does not apply to TOT.

[Devin Rousso]

Comment on attachment 257804 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Geometry.js:325
>> +WebInspector.CubicBezier = class CubicBezier
> 
> When you get a chance, maybe file a follow-up bug to consolidate this with the existing bezier curve solver code.

Good idea.

>> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:75
>> +        function createControl(x1, y1)
> 
> should this take WebInspector.Point?

I think that that just adds another level of complexity that isn't really necessary here.

>> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:137
>> +    handleEvent(event)
> 
> This is weird, we usually just attach individual listeners since some elements may already have listeners.

I was emulating how the ColorPicker and Slider are written, since they are also popover elements.

>> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:186
>> +        point.x = clamp(0, point.x - this._controlHandleRadius, this._bezierWidth);
> 
> Where did 'clamp' come from?

It's a function in Utilities.js

>> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:223
>> +        path.push("M", this._controlHandleRadius, this._bezierHeight + this._controlHandleRadius);
> 
> You should use a template string for this, since it doesn't use a loop.

Could you add a bit more info please?  I haven't dealt with template strings much.

[Devin Rousso]

Created attachment 258334 [details]
Patch

[Timothy Hatcher]

Comment on attachment 258334 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.css:91
> +    cursor: pointer;

Pointer is for links, not UI.

[Devin Rousso]

Created attachment 258347 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258347 [details]
Patch

Clearing flags on attachment: 258347

Committed r188028: <http://trac.webkit.org/changeset/188028>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.