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.
<rdar://problem/15469527>
Created attachment 216936 [details] Screenshot of new color picker
Looks good!
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
Created attachment 216942 [details] Patch
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 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 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.
(In reply to comment #8) > Also, awesome color wheel generation. Courtesy of Dean Jackson.
Created attachment 217031 [details] Patch for landing
Comment on attachment 217031 [details] Patch for landing Clearing flags on attachment: 217031 Committed r159332: <http://trac.webkit.org/changeset/159332>
All reviewed patches have been landed. Closing bug.
Oo, this removes the "Opacity:" localized string. I'll update localizedStrings.js.