Bug 178404 – Web Inspector: [PARITY] Styles Redesign: Add color gradient, bezier curve, and spring inline widgets

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Add the following inline widgets:
- Gradients, e.g. `background-image: linear-gradient(yellow, orange)`;
- Bezier curves, e.g. `transition-timing-function: cubic-bezier(0.68, -0.55, 0.265, 1.55)`;
- Spring functions;
- Variables, e.g. `var(--toolbar-color)`.

[Radar WebKit Bug Importer]

<rdar://problem/35035992>

[Nikita Vasilyev]

Created attachment 324071 [details]
Patch

Static page for manual testing: http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/color.html

This patch adds gradients, bezier curves, and spring functions. CSS variables are a little different, I'll add them in a follow up patch.

[Devin Rousso]

Comment on attachment 324071 [details]
Patch

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

r- for the issue with nested var()

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:338
> +                let text = rawTokens.map((token) => token.value).join("");
> +
> +                let gradient = WI.Gradient.fromString(text);

Style: unnecessary newline.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403
> +            } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") {

Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`.  You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even.  Gradient already has this logic.

This will also be the case for Color and Spring.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:422
> +    _addSpringTokens(tokens)

The logic in `_addSpringTokens` is almost identical to `_addCubicBezierTokens`, with the exception of:

    (429): token.value === "spring"
    (437): let spring = WI.Spring.fromString(text);
    (439): newTokens.push(this._createInlineSwatch(WI.InlineSwatch.Type.Spring, text, spring));

Considering that CubicBezier and Spring are both used for the same CSS properties, can you combine them and avoid this logic?

[Matt Baker]

Comment on attachment 324071 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:295
> +            let value = event.data && event.data.value && event.data.value.toString();

InlineSwatch always includes event data when the value changes, so that check can be removed. I think it's also safe to assume that event.data implies event.data.toString(). With the exception of Color.prototype.toString (which can throw), all the InlineSwatch value objects (Color, CubicBezier, Spring, and the Gradient subclasses) will always return a string.

It's not as concise, but you could rewrite this as:

if (!event.data.value)
    return;

try {
    let value = event.data.value.toString();
    console.assert(value, "Expected value string.", event.data.value);
    if (!value)
        return;
    this._handleValueChange();
} catch (e) {
    console.error("Could not convert value to string.", e);
}

It's weird that we throw an exception here, but since we do it should be handled.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:321
> +                // Gradient start

Drop this comment. The variable name `gradientStartIndex` makes it clear.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:327
> +                // Gradient end

This one too. Also, this isn't necessarily the end of the gradient, as the comment on line 331 indicates.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:331
> +                    // Color function inside of a gradient.

Since this could also be a var(), I'd make this comment more general: "// Matched a CSS function inside of the gradient."

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:401
> +                // Cubic-bezier start

Remove comment.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:404
> +                // Cubic-bezier end

Ditto.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:430
> +                // Spring start

Ditto.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:433
> +                // Spring end

Ditto.

[Nikita Vasilyev]

Comment on attachment 324071 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403
>> +            } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") {
> 
> Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`.  You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even.  Gradient already has this logic.
> 
> This will also be the case for Color and Spring.

Good catch!

The old styles sidebar didn't detect `linear-gradient(#fff, var(--myBlue))` as a gradient, but I intent to fix it in the new one.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #5)
> Comment on attachment 324071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324071&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403
> >> +            } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") {
> > 
> > Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`.  You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even.  Gradient already has this logic.
> > 
> > This will also be the case for Color and Spring.
> 
> Good catch!
> 
> The old styles sidebar didn't detect `linear-gradient(#fff, var(--myBlue))`
> as a gradient, but I intent to fix it in the new one.

This case is tricky:

    linear-gradient(#fff, var(--myBlue))

This is one case when a text range of one widget, var(--myBlue), is inside of a text range of another widget, linear-gradient(...). At very least, when after modifying #fff using the gradient widget, the variable widget for var(--myBlue) should still work.

When modifying var(--myBlue) using the gradient widget, should it replace var(--myBlue) with a new value, or should it update the variable?

I'm not going to address these issue here, but it is something to think in the future.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #5)
> Comment on attachment 324071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324071&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403
> >> +            } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") {
> > 
> > Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`.  You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even.  Gradient already has this logic.
> > 
> > This will also be the case for Color and Spring.
> 
> Good catch!
> 
> The old styles sidebar didn't detect `linear-gradient(#fff, var(--myBlue))`
> as a gradient, but I intent to fix it in the new one.

None of this function can parse CSS variables, calc() or min/max() correctly:

    WI.CubicBezier.fromString
    WI.Spring.fromString
    WI.Color.fromString
    WI.Gradient.fromString

E.g.:
    
    -> WI.Gradient.fromString("linear-gradient(#fff2db, var(--myVar))")
    <- null

This seems non-trivial to fix and it isn't required for parity with the old styles sidebar,
so I'm not going to do it in this task.

[Nikita Vasilyev]

Created attachment 324430 [details]
Patch

(In reply to Matt Baker from comment #4)
> Comment on attachment 324071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324071&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:295
> > +            let value = event.data && event.data.value && event.data.value.toString();
> 
> InlineSwatch always includes event data when the value changes, so that
> check can be removed. I think it's also safe to assume that event.data
> implies event.data.toString(). With the exception of
> Color.prototype.toString (which can throw), all the InlineSwatch value
> objects (Color, CubicBezier, Spring, and the Gradient subclasses) will
> always return a string.
> 
> It's not as concise, but you could rewrite this as:
> 
> if (!event.data.value)
>     return;
> 
> try {
>     let value = event.data.value.toString();
>     console.assert(value, "Expected value string.", event.data.value);
>     if (!value)
>         return;
>     this._handleValueChange();
> } catch (e) {
>     console.error("Could not convert value to string.", e);
> }

The old styled sidebar didn't have try/catch, see CSSStyleDeclarationTextEditor.propotype._inlineSwatchValueChanged:

    let value = event.data && event.data.value && event.data.value.toString();

We rarely use try/catch and it seems unnecessary here. I think it's better to replace it with console.error.

[Devin Rousso]

Comment on attachment 324071 [details]
Patch

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

>>>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403
>>>>> +            } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") {
>>>> 
>>>> Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`.  You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even.  Gradient already has this logic.
>>>> 
>>>> This will also be the case for Color and Spring.
>>> 
>>> Good catch!
>>> 
>>> The old styles sidebar didn't detect `linear-gradient(#fff, var(--myBlue))` as a gradient, but I intent to fix it in the new one.
>> 
>> This case is tricky:
>> 
>>     linear-gradient(#fff, var(--myBlue))
>> 
>> This is one case when a text range of one widget, var(--myBlue), is inside of a text range of another widget, linear-gradient(...). At very least, when after modifying #fff using the gradient widget, the variable widget for var(--myBlue) should still work.
>> 
>> When modifying var(--myBlue) using the gradient widget, should it replace var(--myBlue) with a new value, or should it update the variable?
>> 
>> I'm not going to address these issue here, but it is something to think in the future.
> 
> None of this function can parse CSS variables, calc() or min/max() correctly:
> 
>     WI.CubicBezier.fromString
>     WI.Spring.fromString
>     WI.Color.fromString
>     WI.Gradient.fromString
> 
> E.g.:
>     
>     -> WI.Gradient.fromString("linear-gradient(#fff2db, var(--myVar))")
>     <- null
> 
> This seems non-trivial to fix and it isn't required for parity with the old styles sidebar,
> so I'm not going to do it in this task.

That is a good point.  Deriving and/or modifying the value of a variable would not be simple, especially if the variable has multiple overrides.  I would think that in this case the `var(...)` would just get replaced with the resulting value, instead of changing the variable (the user could do that in the variable's declaration), but we could always make it work the other way.  calc/min/max would probably be easier, but not as much if they involved multiple units.  I think that we can just add a FIXME and follow up later.

[Nikita Vasilyev]

Created attachment 324506 [details]
Patch

Added FIXME with Bug 178636 - Web Inspector: Styles: Make inline widgets work with CSS functions (var(), calc(), etc.)

[Devin Rousso]

Comment on attachment 324506 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:339
> +                    newTokens.push(...tokens);

It seems incorrect to push all of the tokens to newTokens, as we are already using some of them in the next else-if (343).  Should we instead be using `rawTokens`?

    newTokens.push(...rawTokens);

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:358
>                  newTokens.push(...tokens);

Ditto (339).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:425
> +                    newTokens.push(...tokens);

Ditto (339).

[Nikita Vasilyev]

Created attachment 324883 [details]
Patch

(In reply to Devin Rousso from comment #11)
> Comment on attachment 324506 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324506&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:339
> > +                    newTokens.push(...tokens);
> 
> It seems incorrect to push all of the tokens to newTokens, as we are already
> using some of them in the next else-if (343).  Should we instead be using
> `rawTokens`?

Whoops, good catch!

[WebKit Commit Bot]

Comment on attachment 324883 [details]
Patch

Clearing flags on attachment: 324883

Committed r223978: <https://trac.webkit.org/changeset/223978>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.