Bug 178354 - Web Inspector: [PARITY] Styles Redesign: Add color picker inline widget
Summary: Web Inspector: [PARITY] Styles Redesign: Add color picker inline widget
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks: 178404
  Show dependency treegraph
 
Reported: 2017-10-16 11:46 PDT by Nikita Vasilyev
Modified: 2017-10-17 13:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.63 KB, patch)
2017-10-16 19:48 PDT, Nikita Vasilyev
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
[Image] With patch applied (91.22 KB, image/png)
2017-10-16 19:51 PDT, Nikita Vasilyev
no flags Details
Patch (6.99 KB, patch)
2017-10-17 13:15 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-10-16 11:46:03 PDT
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>
Comment 1 Nikita Vasilyev 2017-10-16 19:48:15 PDT
Created attachment 323974 [details]
Patch
Comment 2 Nikita Vasilyev 2017-10-16 19:51:20 PDT
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
Comment 3 Joseph Pecoraro 2017-10-16 21:51:31 PDT
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("");
Comment 4 Joseph Pecoraro 2017-10-16 21:57:30 PDT
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.
Comment 5 Nikita Vasilyev 2017-10-17 13:15:36 PDT
Created attachment 324047 [details]
Patch
Comment 6 WebKit Commit Bot 2017-10-17 13:55:26 PDT
Comment on attachment 324047 [details]
Patch

Clearing flags on attachment: 324047

Committed r223575: <https://trac.webkit.org/changeset/223575>
Comment 7 WebKit Commit Bot 2017-10-17 13:55:27 PDT
All reviewed patches have been landed.  Closing bug.