Bug 124354 – Web Inspector: New color picker

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

[Antoine Quint]

The existing color picker doesn't match the rest of the user interface very well, this task covers adding a new color picker look and functionality.

[Radar WebKit Bug Importer]

<rdar://problem/15469527>

[Antoine Quint]

Created attachment 216936 [details]
Screenshot of new color picker

[Timothy Hatcher]

Looks good!

[Antoine Quint]

This is only a start, some upcoming features are tracked elsewhere:

- http://webkit.org/b/124355: color wheel should support Retina displays
- http://webkit.org/b/124356: color picker should feature an editable CSS value
- http://webkit.org/b/124357: color picker should allow picking a color for any pixel on screen
- http://webkit.org/b/124358: color picker should suggest swatches to use based on edited property and colors used in page

[Antoine Quint]

Created attachment 216942 [details]
Patch

[Antoine Quint]

A couple more related tasks:

http://webkit.org/b/124363: contextual menu on hover for actionable tokens
http://webkit.org/b/124364: allow editing of colors in CSS resources

[Timothy Hatcher]

Comment on attachment 216942 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Color.js:843
> +    var min = Math.min(Math.min(r, g), b),
> +        max = Math.max(Math.max(r, g), b),
> +        delta = max - min;

We prefer multiple var declarations and not stringing them.

> Source/WebInspectorUI/UserInterface/Color.js:847
> +    var v = max,
> +        s,
> +        h;

Ditto.

> Source/WebInspectorUI/UserInterface/Color.js:849
> +    if (max == min)

===

> Source/WebInspectorUI/UserInterface/Color.js:851
> +    else if (max == r)

===

> Source/WebInspectorUI/UserInterface/Color.js:853
> +    else if (max == g)

===

> Source/WebInspectorUI/UserInterface/Color.js:855
> +    else if (max == b)

===

> Source/WebInspectorUI/UserInterface/Color.js:862
> +    if (max == 0)

===

> Source/WebInspectorUI/UserInterface/Color.js:873
> +        return [v,v,v];

Spaces after the commas.

> Source/WebInspectorUI/UserInterface/ColorPicker.js:49
> +    this._opacityPattern = 'url("data:image/svg+xml;base64,' + btoa('<svg xmlns="http://www.w3.org/2000/svg" width="6" height="6" fill="rgb(204, 204, 204)"><rect width="3" height="3" /><rect x="3" y="3" width="3" height="3"/></svg>') + '")';

Neat! It would be nice it this was declared in CSS or at least in an SVG file. Then we could use it for the swatch background too.

> Source/WebInspectorUI/UserInterface/ColorPicker.js:101
> +        this._opacitySlider.value = color.alpha || 1;

This will cause a color that has alpha === 0 to turn into alpha === 1. Is that intended?

> Source/WebInspectorUI/UserInterface/ColorPicker.js:139
> +        this._opacitySlider.element.style.backgroundImage = "linear-gradient(90deg, " + transparent + ", " + tintedColor + "), " + this._opacityPattern;
> +
> +        this._brightnessSlider.element.style.backgroundImage = "linear-gradient(90deg, black, " + rawColor + ")";

Grouping these two lines would read better, with the blank line before both.

> Source/WebInspectorUI/UserInterface/ColorWheel.js:58
> +        this._radius = dimension / 2 - 2;

A comment on why it is 2px smaller would be good.

> Source/WebInspectorUI/UserInterface/Slider.js:125
> +        return window.webkitConvertPointFromPageToNode(this._element, new WebKitPoint(event.pageX, event.pageY));

Add a comment why this is needed (since this class allows transforms).

> Source/WebInspectorUI/UserInterface/Slider.js:131
> +            this.__maxX = this._element.offsetWidth - WebInspector.Slider.KnobWidth / 2 - 1;

Comment on why it is 1 smaller.

> Source/WebInspectorUI/WebInspectorUI.vcxproj/WebInspectorUI.vcxproj.filters:1
> -<?xml version="1.0" encoding="utf-8"?>
> +<?xml version="1.0" encoding="utf-8"?>

Might want to revert the BOM change.

[Joseph Pecoraro]

Comment on attachment 216942 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:73
> +        the colors themslves being retrieved via the public properties tintedColor and rawColor.

Typo: "themslves" => "themselves"

> Source/WebInspectorUI/UserInterface/Color.js:424
> +        s = [
> +            l += s *= l < .5 ? l : 1 - l,
> +            l - h % 1 * s * 2,
> +            l -= s *= 2,
> +            l,
> +            l + h % 1 * s,
> +            l + s
> +        ];

Gross reuse of s (which was a float, and now changes to an array). I wonder if that might trip up our JITs (ggaren says maybe). I'd prefer if you used a new local variable for the array instead of reusing s.

> Source/WebInspectorUI/UserInterface/Color.js:835
> +WebInspector.Color.rgb2hsv = function (r, g, b)

Style: no space in "function ("

> Source/WebInspectorUI/UserInterface/Color.js:839
> +    r = (r / 255);
> +    g = (g / 255);
> +    b = (b / 255);

/=?

> Source/WebInspectorUI/UserInterface/Color.js:875
> +    h = h / 60;

/=?

> Source/WebInspectorUI/UserInterface/ColorWheel.js:220
> +                var pos = (j * dimension + i) * 4;
> +                var color = this._colorAtPointWithBrightness(i, j, 1);
> +                if (!color)
> +                    continue;
> +                data[pos] = color.rgb[0];
> +                data[pos + 1] = color.rgb[1];
> +                data[pos + 2] = color.rgb[2];

You could move `var pos` after the if statement, to before line 218. It is not needed before the if.

Also, awesome color wheel generation.

[Antoine Quint]

(In reply to comment #8) 
> Also, awesome color wheel generation.

Courtesy of Dean Jackson.

[Antoine Quint]

Created attachment 217031 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 217031 [details]
Patch for landing

Clearing flags on attachment: 217031

Committed r159332: <http://trac.webkit.org/changeset/159332>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Oo, this removes the "Opacity:" localized string. I'll update localizedStrings.js.