Bug 162758 – Web Inspector: Shift clicking on named color value only shows its hex form

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

[Nikita Vasilyev]

Created attachment 290253 [details]
[Animated GIF] Bug

Steps:
1. Inspect any element on this page.
2. Add "color: brown" inline style.
3. Click twice while holding Shift on the brown square.

Expected:
Shift clicking should cycle through all possible color formats (i.e. RGB, HSL, hex).

Actual:
Only hex and named color values are shown.

Also, the following assertion failed:
[Error] Assertion Failed
	_inlineSwatchValueChanged (CSSStyleDeclarationTextEditor.js:1333)
	dispatch (Object.js:170)
	dispatchEventToListeners (Object.js:177)
	_updateSwatch (InlineSwatch.js:123)
	_swatchElementClicked (InlineSwatch.js:137)
	_swatchElementClicked

[Radar WebKit Bug Importer]

<rdar://problem/28553352>

[Devin Rousso]

Created attachment 290269 [details]
Patch

[Build Bot]

Comment on attachment 290269 [details]
Patch

Attachment 290269 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2171570

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 290273 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290269 [details]
Patch

Attachment 290269 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2171647

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 290278 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290269 [details]
Patch

Attachment 290269 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2171671

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 290279 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Comment on attachment 290269 [details]
Patch

r- need to update the color test.

[Devin Rousso]

Created attachment 290465 [details]
Patch

[Matt Baker]

Comment on attachment 290465 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:18
> +        If the alpha is not 1, it will not show formats with non-alpha values.

This is true when cycling through formats with shift+click, but not when using the context menu:

1. Inspect element, add style rule "color: orange"
2. Right click swatch, select "Format: Hex"
  => Rule changes to "color: #ffa500"
3. Right click swatch
  => "Format: Hex with Alpha" is an option

[Devin Rousso]

Comment on attachment 290465 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:18
>> +        If the alpha is not 1, it will not show formats with non-alpha values.
> 
> This is true when cycling through formats with shift+click, but not when using the context menu:
> 
> 1. Inspect element, add style rule "color: orange"
> 2. Right click swatch, select "Format: Hex"
>   => Rule changes to "color: #ffa500"
> 3. Right click swatch
>   => "Format: Hex with Alpha" is an option

I actually think that this should stay as it is.  My view is that Shift+Click cycles through the different formats of the color, but only the ones that are truly necessary (it would be annoying for the user to have to click past RGB and RGBA in order to get to HSL).  The context menu, however, is more direct and lets the user pick exactly which format they want (albeit going for an alpha format on a color that doesn't have an alpha is a bit more tedious).  I see them serving two different purposes:

Shift+Click: what does this color value look like in each format?
Right-Click: I need to edit this color in HSL because I don't know how to modify HEX as easily

[Matt Baker]

Comment on attachment 290465 [details]
Patch

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

>>> Source/WebInspectorUI/ChangeLog:18
>>> +        If the alpha is not 1, it will not show formats with non-alpha values.
>> 
>> This is true when cycling through formats with shift+click, but not when using the context menu:
>> 
>> 1. Inspect element, add style rule "color: orange"
>> 2. Right click swatch, select "Format: Hex"
>>   => Rule changes to "color: #ffa500"
>> 3. Right click swatch
>>   => "Format: Hex with Alpha" is an option
> 
> I actually think that this should stay as it is.  My view is that Shift+Click cycles through the different formats of the color, but only the ones that are truly necessary (it would be annoying for the user to have to click past RGB and RGBA in order to get to HSL).  The context menu, however, is more direct and lets the user pick exactly which format they want (albeit going for an alpha format on a color that doesn't have an alpha is a bit more tedious).  I see them serving two different purposes:
> 
> Shift+Click: what does this color value look like in each format?
> Right-Click: I need to edit this color in HSL because I don't know how to modify HEX as easily

Works for me! Please update the change log so it's clear that this is by design.

[Matt Baker]

Comment on attachment 290465 [details]
Patch

r-, for the following issue with alpha value decimal places:

1. Inspect element, add style rule "color: hsla(0, 100%, 100%, 0.5)"
2. Right-click, select "Format: Hex with Alpha"
  => Value changes to "#ff000080"
3. Right-click, select "Format: HSLA"
  => Value changes to "hsla(0, 100%, 50%, 0.5019607843137255);"

[Devin Rousso]

Created attachment 290887 [details]
Patch

[Build Bot]

Comment on attachment 290887 [details]
Patch

Attachment 290887 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2235268

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 290890 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290887 [details]
Patch

Attachment 290887 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2235286

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 290891 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290887 [details]
Patch

Attachment 290887 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2235283

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 290893 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Devin Rousso]

Created attachment 292583 [details]
Patch

[Nikita Vasilyev]

Shift-Clicking on "brown" cycles through the color schemes (RGB, HSL, hex, etc.) but it never goes back to the original value, "brown".

Somehow "color: red" works as expected.

Why is that?

[Nikita Vasilyev]

Comment on attachment 292583 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/Color.js:485
> +        rgba = rgba.map((value) => value.maxDecimals(2));

Nit: would be nice to mention in the changelog that now we round numbers up to 2 decimal points.

[Timothy Hatcher]

Comment on attachment 292583 [details]
Patch

r+ Assuming the thing Nikita mentions is not a bug.

[Devin Rousso]

(In reply to comment #23)
> Shift-Clicking on "brown" cycles through the color schemes (RGB, HSL, hex,
> etc.) but it never goes back to the original value, "brown".
> 
> Somehow "color: red" works as expected.
> 
> Why is that?

So it looks like this is actually a problem.  When we display a Color in a HSL or RGB format, rounding the values will prevent any future color swatches from knowing the original color.  This is a problem because AFAICT, we regenerate all InlineSwatch elements each time the content changes, meaning that each new swatch gets its value from the text and not the original value.  Not sure how to fix this, but I'll think about it and figure something out.

[Nikita Vasilyev]

(In reply to comment #26)
> (In reply to comment #23)
> > Shift-Clicking on "brown" cycles through the color schemes (RGB, HSL, hex,
> > etc.) but it never goes back to the original value, "brown".
> > 
> > Somehow "color: red" works as expected.
> > 
> > Why is that?
> 
> So it looks like this is actually a problem.  When we display a Color in a
> HSL or RGB format, rounding the values will prevent any future color
> swatches from knowing the original color.  This is a problem because AFAICT,
> we regenerate all InlineSwatch elements each time the content changes,
> meaning that each new swatch gets its value from the text and not the
> original value.  Not sure how to fix this, but I'll think about it and
> figure something out.

Since this patch is already an improvement from what we had, I'm fine with landing it and fixing the rounding issue separately.

[WebKit Commit Bot]

Comment on attachment 292583 [details]
Patch

Clearing flags on attachment: 292583

Committed r208857: <http://trac.webkit.org/changeset/208857>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.