Bug 117919 – Web Inspector: Display color picker in popover on color swatch click

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Holden]

Re-introduce the Spectrum.js component that was previously available in the deprecated inspector.

[Radar WebKit Bug Importer]

<rdar://problem/14242305>

[Matt Holden]

Created attachment 205271 [details]
Proposed fix

[Antoine Quint]

Cool!

Some comments about the UI and behaviour:

- I think we should remove the background color so it blends in better with the popover
- the greek alpha label is too obscure, I think it would be better to use Opacity
- I get LOCALIZED_STRING_NOT_FOUND when hovering over the color well in the Styles sidebar
- I don't know if it's the logic of the swatch itself or the CSS changes applying in the background, but mousing down and holding while mousing around the gradient swatch is laggy
- I think the color label should be clickable to cycle through various CSS values (RGB, hexa, HSV, etc.)

[Antoine Quint]

Created attachment 205282 [details]
Screenshot of popover per first patch

Added a screenshot of the first patch in action.

[Timothy Hatcher]

Comment on attachment 205271 [details]
Proposed fix

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

Great job! Just some minor issues that need addressed.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:38
> +    -webkit-user-select: none;

No need for this. We have it set globally.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:113
> +.colorpicker-dragger, .colorpicker-slider {
> +    -webkit-user-select: none;
> +}

Not needed.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:115
> +.colorpicker-sat {

You should spell out saturation.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:119
> +.colorpicker-val {

value? luminance? Whatever it is, spell it out.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:39
> +    // TODO: ported from Spectrum.js Not sure if this stays?
> +    //this._element.tabIndex = 0;

I don't think it is needed.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:49
> +    this._dragHelperElement = this._draggerElement
> +    .createChild("div", "colorpicker-sat colorpicker-fill-parent")
> +    .createChild("div", "colorpicker-val colorpicker-fill-parent")
> +    .createChild("div", "colorpicker-dragger");

I'm not sold on using the createChild helper in a chain like this. At the very least, the last three lines should be indented.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:57
> +    //alphaLabel.textContent = WebInspector.UIString(
> +    alphaLabel.textContent = "\u03B1:";

Use "Opacity:" and put the string in localizableStrings.js in alphabetical order.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:76
> +    function makeDraggable(element, onmove, onstart, onstop) {
> +        var doc = document;

{ should be on a new line.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:90
> +            if (dragging) {

This should return early to prevent indenting the whole function body.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:101
> +            var rightClick = e.which ? (e.which === 3) : (e.button === 2);

No need for ().

We prefer:
if (event.button !== 0 || event.ctrlKey)
    return;

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:105
> +            if (!rightClick && !dragging) {
> +
> +                if (onstart)

Remove empty line here.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:138
> +            if (dragging) {
> +                doc.removeEventListener("selectstart", consume, false);
> +                doc.removeEventListener("dragstart", consume, false);
> +                doc.removeEventListener("mousemove", move, false);
> +                doc.removeEventListener("mouseup", stop, false);
> +
> +                if (onstop)
> +                    onstop(element, e);
> +            }
> +
> +            dragging = false;

This should return early to prevent indenting the whole function body. No need for dragging = false then either.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:180
> +    }
> +
> +};

Remove empty line.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:246
> +            if (format === cf.RGBA)
> +                format = cf.RGB;
> +            else if (format === cf.HSLA)
> +                format = cf.HSL;
> +            // Don't output ShortHex
> +            else if (format === cf.ShortHEX) 
> +                format = cf.HEX;
> +        } else {
> +            // Everything except HSL(A) should be returned as RGBA if transparency is involved.
> +            if (format === cf.HSL || format === cf.HSLA)
> +                format = cf.HSLA;
> +            else
> +                format = cf.RGBA;

Use the full WebInspector.Color.Format... constant in all these places. Shortening them hinders find and replace for renaming in the future.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:290
> +                        Math.min(this.dragWidth - this._dragHelperElementHeight, dragX - this._dragHelperElementHeight));

Only indent 4 spaces. Don't line up.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:292
> +                        Math.min(this.dragHeight - this._dragHelperElementHeight, dragY - this._dragHelperElementHeight));

Ditto.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:324
> +WebInspector.CSSColorPicker.hsvaToRGBA = function(h, s, v, a)

Should this be moved to WebInspector.Color as a helper function?

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:358
> +WebInspector.CSSColorPicker.rgbaToHSVA = function(r, g, b, a)

Ditto?

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:431
> -                    swatchElement.title = WebInspector.UIString("Click to toggle color format");
> +                    swatchElement.title = WebInspector.UIString("Click to open a color picker. Shift-click to toggle color format.");

Need to update localizableStrings.js.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:672
> +            function _update()

A more descriptive name without the underscore would be better.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:688
> +            }
> +            this._codeMirror.operation(_update.bind(this))

Empty line between these two lines.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:708
> +        } else {
> +
> +            if (this._colorPickerPopover) {

Remove empty line.

Also I think there is a bug if the popover dismisses by clicking away (more common than clicking the swatch again). Only if the user clicks the swatch again will this._colorPickerPopover be deleted. Clicking another swatch would not show the popover on first click. You shouldn't need to hold onto the Popover, but clicking the same swatch again should dismiss the current popover and not show another one again. Perhaps Popover.js's handleEvent should preventDefault and/or stopPropagation when it calls dismiss? Then this code will not be called.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:716
> +            var colorPicker = new WebInspector.CSSColorPicker();

Omit the () when there are no params.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:724
> +            this._colorPickerPopover.content = colorPicker.element;
> +            var bounds = WebInspector.Rect.rectFromClientRect(swatch.getBoundingClientRect());

Empty line between these lines would help readability.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:726
> +            this._colorPickerPopover.present(bounds, [WebInspector.RectEdge.MIN_X]);
> +            colorPicker.shown();

Ditto.

[Timothy Hatcher]

Comment on attachment 205271 [details]
Proposed fix

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

>> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:138
>> +            dragging = false;
> 
> This should return early to prevent indenting the whole function body. No need for dragging = false then either.

Actually you still need dragging = false; but you can return early if dragging is already false.

[Timothy Hatcher]

Comment on attachment 205271 [details]
Proposed fix

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

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:34
> +    background: rgba(230, 230, 230, 1) !important;
> +    border: 1px solid #CCC;

These should be removed.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:35
> +    padding: 10px;

Padding could be reduced since the popover has a padding and there is no background and border anymore.

[Matt Holden]

Created attachment 205358 [details]
Screenshot of colorpicker with background color and border removed

I removed the container's background color and border -- and as you can see it makes top-left corner of the color square blend into the background.

It appears to me that there needs to be some differentiation around the color square.  Any ideas?

[Timothy Hatcher]

(In reply to comment #8)
> Created an attachment (id=205358) [details]
> Screenshot of colorpicker with background color and border removed
> 
> I removed the container's background color and border -- and as you can see it makes top-left corner of the color square blend into the background.
> 
> It appears to me that there needs to be some differentiation around the color square.  Any ideas?

I agree. A border around the boxes would help.

[Matt Holden]

Created attachment 205458 [details]
Patch

[Matt Holden]

I added a new patch this evening.  

Antoine -- I'm not sure what to do about the lag you reported.  Do you have any suggestions for where to start looking?

[WebKit Commit Bot]

Attachment 205458 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebInspectorUI/ChangeLog', u'Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js', u'Source/WebInspectorUI/UserInterface/CSSColorPicker.css', u'Source/WebInspectorUI/UserInterface/CSSColorPicker.js', u'Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js', u'Source/WebInspectorUI/UserInterface/Main.html']" exit_code: 1
Source/WebInspectorUI/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

(In reply to comment #11)
> I added a new patch this evening.  
> 
> Antoine -- I'm not sure what to do about the lag you reported.  Do you have any suggestions for where to start looking?

I think it's fine to deal with this as a follow-up bug. Just make sure you raise one when this bug lands.

[Build Bot]

Comment on attachment 205458 [details]
Patch

Attachment 205458 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/930416

New failing tests:
fast/media/mq-transform-03.html
fast/forms/search-event-delay.html
fast/media/mq-transform-02.html
platform/win/accessibility/multiple-select-element-role.html

[Build Bot]

Created attachment 205473 [details]
Archive of layout-test-results from APPLE-EWS-5 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-5  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Timothy Hatcher]

Comment on attachment 205458 [details]
Patch

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

Looking good. Found some more minor things to fix.

>> Source/WebInspectorUI/ChangeLog:8
>> +		This patch implements suggests from the previous review.
> 
> Line contains tab character.  [whitespace/tab] [5]

Replace the tab with spaces.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:100
> +            if (e.button != 0 || e.ctrlKey)

!==

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:147
> +        this.hsv[0] = (dragY / this.slideHeight);

No need for the ().

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:156
> +        initialHelperOffset = { x: this._dragHelperElement.offsetLeft, y: this._dragHelperElement.offsetTop };

No need for the spaces inside {}, except after : and ,.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:193
> +    ColorChanged: "colorChanged"

The event text should be: "css-color-picker-color-changed"

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:208
> +        if (!this._originalFormat) {
> +            this._originalFormat = color.format;
> +        }

No need for the { }

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:231
> +        var color;
> +
> +        if (rgba[3] === 1)
> +            color = WebInspector.Color.fromRGB(rgba[0], rgba[1], rgba[2]);
> +        else
> +            color = WebInspector.Color.fromRGBA(rgba[0], rgba[1], rgba[2], rgba[3]);
> +
> +        var colorValue = color.toString(this.outputColorFormat);
> +        if (!colorValue)
> +            colorValue = color.toString(); // this.outputColorFormat can be invalid for current color (e.g. "nickname").
> +        return new WebInspector.Color(colorValue);

I think this can be simplified to:

var color = WebInspector.Color.fromRGBA(rgba[0], rgba[1], rgba[2], rgba[3]);
color.format = this.outputColorFormat;
if (!color.toString())
    color.format = color.nextFormat();
return color;

WebInspector.Color should be tweaked to allow this code to be less roundabout. Can be a follow up.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:243
> +            else if (format === WebInspector.Color.Format.HSVA)
> +                format = WebInspector.Color.Format.HSV;

These constants do not exist. Maybe the old Inspector had it?

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:248
> +            // Don't output ShortHex
> +            else if (format === WebInspector.Color.Format.ShortHEX)
> +                format = WebInspector.Color.Format.HEX;

This comment breaks the flow of the else ifs. It should go inside of a block, or at the end of a line instead, or merged with the "Simplify transparent formats." comment.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:250
> +            // Everything except HSL(A)&HSV(A) should be returned as RGBA if transparency is involved.

There isn't hsv() in CSS. THis comment should only mention HSLA.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:253
> +                WebInspector.Color.Format.HSVA,

This constant does not exist.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:257
> +            if (printableTransparentFormats.indexOf(format) == -1)

===

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:285
> +    _onchange: function()

Can we come up with a better name here? Maybe just _update? This should have a // Private comment before it separated by blank lines to match other files.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:288
> +        this.dispatchEventToListeners(WebInspector.CSSColorPicker.Event.ColorChanged, { color: this.color });

No needed for spaces inside {}, except after :.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:315
> +    _updateUI: function()

We prefer to not abbreviate. _updateInterface? _updateDisplay?

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:336
> +        var colorValue = this.color.toString(this.outputColorFormat);
> +        if (!colorValue)
> +            colorValue = color.toString(); // this.outputColorFormat can be invalid for current color (e.g. "nickname").

We should fix WebInspector.Color so this isn't needed. Maybe use nextFormat() internally if the current format is invalid. Can be a follow up.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:192
> +    // WebInspector.Popover delegate protocol method

Comment not needed.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:193
> +    didDismissPopover: function(popover) {

{ should be on new line. This should also be inside the Protected section below.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:194
> +        if (popover == this._colorPickerPopover)

===

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:681
> +            this._codeMirror.operation(function() {

{ should be on a new line.

[Matt Holden]

Created attachment 205566 [details]
Patch

[Timothy Hatcher]

Thanks for the going through all the iterations. This looks good!

[WebKit Commit Bot]

Comment on attachment 205566 [details]
Patch

Clearing flags on attachment: 205566

Committed r152092: <http://trac.webkit.org/changeset/152092>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Holden]

Awesome!

Thanks for the helpful feedback.