Bug 124354

Summary: Web Inspector: New color picker
Product: WebKit Reporter: Antoine Quint <graouts>
Component: Web InspectorAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Screenshot of new color picker
none
Patch
none
Patch for landing none

Description Antoine Quint 2013-11-14 07:55:37 PST
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.
Comment 1 Radar WebKit Bug Importer 2013-11-14 07:55:50 PST
<rdar://problem/15469527>
Comment 2 Antoine Quint 2013-11-14 07:55:58 PST
Created attachment 216936 [details]
Screenshot of new color picker
Comment 3 Timothy Hatcher 2013-11-14 08:05:16 PST
Looks good!
Comment 4 Antoine Quint 2013-11-14 08:18:32 PST
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
Comment 5 Antoine Quint 2013-11-14 08:44:04 PST
Created attachment 216942 [details]
Patch
Comment 6 Antoine Quint 2013-11-14 09:12:48 PST
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
Comment 7 Timothy Hatcher 2013-11-14 09:46:40 PST
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.
Comment 8 Joseph Pecoraro 2013-11-14 11:18:41 PST
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.
Comment 9 Antoine Quint 2013-11-14 13:57:40 PST
(In reply to comment #8) 
> Also, awesome color wheel generation.

Courtesy of Dean Jackson.
Comment 10 Antoine Quint 2013-11-15 02:29:44 PST
Created attachment 217031 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2013-11-15 03:00:51 PST
Comment on attachment 217031 [details]
Patch for landing

Clearing flags on attachment: 217031

Committed r159332: <http://trac.webkit.org/changeset/159332>
Comment 12 WebKit Commit Bot 2013-11-15 03:00:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Joseph Pecoraro 2013-11-18 15:39:02 PST
Oo, this removes the "Opacity:" localized string. I'll update localizedStrings.js.