Bug 178354 – Web Inspector: [PARITY] Styles Redesign: Add color picker inline widget

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

We have five inline widgets:

- colors, e.g. `color: #abc`
- 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. `--toolbar-color`

Color picker is by far the most commonly used inline widget. I'll add it in this patch and
I'll add the rest of them in the follow-up patch.

<rdar://problem/33526420>

[Nikita Vasilyev]

Created attachment 323974 [details]
Patch

[Nikita Vasilyev]

Created attachment 323975 [details]
[Image] With patch applied

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

[Joseph Pecoraro]

Comment on attachment 323974 [details]
Patch

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

r=me, my comments are just style and nits.

> Source/WebInspectorUI/UserInterface/Models/Color.js:645
> +    "hsla"

Style: Include the trailing comma. It makes future diffs nicer. We might add a color() for p3 later.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:285
> +            let colorTokenElement = document.createElement("span");
> +            colorTokenElement.classList.add("token-color");
> +
> +            let innerElement = document.createElement("span");
> +            innerElement.classList.add("token-color-value");

Style: For elements we create where we are setting the class names ourselves I think we should use `elem.className = x` instead of `elem.classList.add(x)`. So here:

    colorTokenElement.className = "token-color";
    ...
    innerElement.className = "token-color-value";

In practice it probably doesn't matter, but it is slightly faster and easier to reason about (not having to worry about other potential class names).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:296
> +                    let swatch = event && event.target;
> +                    console.assert(swatch, "Color swatch is missing.");
> +                    if (!swatch)
> +                        return;

This is not possible. If the event happens `event` and `event.target` cannot be missing.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:310
> +                swatch.element.addEventListener("mousedown", (event) => event.stop());

Style: We prefer using braces with arrow functions if the return value is unimportant / unused. So:

    swatch.element.addEventListener("mousedown", (event) => { event.stop(); });

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:351
> +                let possiblyColor = this._stringFromTokens(tokens.slice(colorFunctionStartIndex, i + 1));
> +                let color = WI.Color.fromString(possiblyColor);
> +                if (color)
> +                    newTokens.push(createColorTokenElement(possiblyColor, color));
> +                else
> +                    newTokens.push(...tokens.slice(colorFunctionStartIndex, i + 1));

Nit: `possiblyColor` => `possibleColor`
Nit: You should stash the slice into a local to avoid re-computing it later if the fromString fails:

    let rawTokens = tokens.slice(colorFunctionStartIndex, i + 1);
    let possibleColor = this._stringFromTokens(rawTokens);
    let color = WI.Color.fromString(possibleColor);
    if (color)
        newTokens.push(createColorTokenElement(possibleColor, color));
    else
        newTokens.push(...rawTokens);

Finally, in general these blocks can share some code:

    function pushPossibleColorToken(text, ...tokens) {
        let color = WI.Color.fromString(text);
        if (color)
            newTokens.push(createColorTokenElement(text, color);
        else
            newTokens.push(...tokens);
    }

    for (let i = 0; i < tokens.length; ++i) {
        let token = tokens[i];
        if (...) {
            // Hex
            pushPossibleColorToken(token.value, token);
        } else if (...) {
            // Color Function start
            ...
        } else if (...) {
            // Nickname
            pushPossibleColorToken(token.value, token);
        } else if (...) {
            // Color Function end
            let rawTokens = tokens.slice(colorFunctionStartIndex, i + 1);
            let text = this._stringFromTokens(rawTokens);
            pushPossibleColorToken(text, rawTokens);
            colorFunctionStartIndex = NaN;
        }
    }

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:366
> +        return tokens.map((token) => {
> +            return token.value;
> +        }).join("");

Nit: Simplify:

    return tokens.map((token) => token.value).join("");

[Joseph Pecoraro]

One thing our old color swatch thing did is detect if this was a color inside of a gradient definition and didn't include a color swatch for those. I assume that is something that you'll address when adding a gradient swatch. But it might be worth a fixme so it doesn't get forgotten about.

[Nikita Vasilyev]

Created attachment 324047 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 324047 [details]
Patch

Clearing flags on attachment: 324047

Committed r223575: <https://trac.webkit.org/changeset/223575>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.