Bug 151918 – Web Inspector: add context menu items to switch CSS color property value syntax between RGB, HSL, etc

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

[Blaze Burg]

It should be possible to change syntax by using Opt-up/down on the rgb() or hsl() outside of number tokens. Or, the Color Picker could have a drop-down for different formats.

[Radar WebKit Bug Importer]

<rdar://problem/23774841>

[Timothy Hatcher]

Shift-click the swatch. IIRC this is in the tooltip.

[Blaze Burg]

Oh, I guess it does work. It would be nice to add context menu items to the property text and color chip, too.

[Devin Rousso]

Created attachment 268000 [details]
Patch

[Blaze Burg]

Comment on attachment 268000 [details]
Patch

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

I don't have time to review this size of patch tonight, but looks good so far.

> Source/WebInspectorUI/ChangeLog:16
> +        Looks at the RGB values of each nickname to see if the current color

I believe the technical term is 'color keyword': https://drafts.csswg.org/css-color-3/

[Devin Rousso]

Created attachment 268093 [details]
Patch

[Devin Rousso]

Created attachment 268094 [details]
Patch

[Devin Rousso]

Created attachment 268206 [details]
Patch

localizedStrings.js strikes again!

[Build Bot]

Comment on attachment 268206 [details]
Patch

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

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 268210 [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 268206 [details]
Patch

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

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 268211 [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

[Build Bot]

Comment on attachment 268206 [details]
Patch

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

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 268213 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Devin Rousso]

Created attachment 268216 [details]
Patch

[Build Bot]

Comment on attachment 268216 [details]
Patch

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

New failing tests:
inspector/model/color.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 268216 [details]
Patch

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

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 268221 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Build Bot]

Comment on attachment 268216 [details]
Patch

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

New failing tests:
inspector/model/color.html

[Build Bot]

Created attachment 268224 [details]
Archive of layout-test-results from ews112 for mac-yosemite

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

[Joseph Pecoraro]

Comment on attachment 268216 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:23
> +        (WebInspector.Color.prototype.canBeSerializedAsShortHEX):
> +        Fixed to account for alpha values, since HEXAlpha is now supported.

You should consider adding a test case for this in inspector/model/color.html.

> Source/WebInspectorUI/UserInterface/Models/Color.js:358
> +    isKeyword()

You could also add tests for this.

> Source/WebInspectorUI/UserInterface/Models/Color.js:534
> +        let h = null;
> +        let s = null;

These assignments seem unnecessary. We want to treat these variables as holding primitives, but "null" is associated with Object types. `undefined` or `0` might be better defaults, but you could even just undefined in the simplest way `let h; let s;`

> Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js:27
> +WebInspector.ColorSwatch = class ColorSwatch extends WebInspector.Object

Cool class!

> Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js:175
> +        const hexFormats = [
> +            {
> +                format: WebInspector.Color.Format.ShortHEX,
> +                title: WebInspector.UIString("Format: ShortHEX")
> +            },
> +            {
> +                format: WebInspector.Color.Format.ShortHEXAlpha,
> +                title: WebInspector.UIString("Format: ShortHEXAlpha")
> +            },
> +            {
> +                format: WebInspector.Color.Format.HEX,
> +                title: WebInspector.UIString("Format: HEX")
> +            },
> +            {
> +                format: WebInspector.Color.Format.HEXAlpha,
> +                title: WebInspector.UIString("Format: HEXAlpha")
> +            }
> +        ];

These UIStrings don't seem particularly UI friendly. Things like "HEXAlpha" may want to be presented as "Hex with alpha" or something like that?

[Devin Rousso]

Created attachment 268248 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 268248 [details]
Patch

Rejecting attachment 268248 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 268248, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 at 1 with fuzz 3.
patching file LayoutTests/inspector/model/color-expected.txt
patching file LayoutTests/inspector/model/color.html
patching file Source/WebInspectorUI/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
Original content of Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js mismatches at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 283.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/650709

[Devin Rousso]

Created attachment 268249 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 268249 [details]
Patch

Clearing flags on attachment: 268249

Committed r194568: <http://trac.webkit.org/changeset/194568>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 268249 [details]
Patch

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

> LayoutTests/inspector/model/color.html:158
> +            InspectorTest.expectThat(color.canBeSerializedAsShortHEX() === false, "'#11122233' should not be serializable as a short Hex");

I was hoping for: "11223345" because your change caught an issue in the `alpha` value being able to be shorted. But still good!

[Devin Rousso]

Comment on attachment 268249 [details]
Patch

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

>> LayoutTests/inspector/model/color.html:158
>> +            InspectorTest.expectThat(color.canBeSerializedAsShortHEX() === false, "'#11122233' should not be serializable as a short Hex");
> 
> I was hoping for: "11223345" because your change caught an issue in the `alpha` value being able to be shorted. But still good!

Yeah I wanted to do that, but I was having a hard time finding a hex whose alpha translated nicely into RGBA/HSLA.  The only ones that I could find that worked nicely were repeated numbers (multiples of 33 actually).  If you can find a nice alpha value (or if you don't think I even need to go that far), I'd be happy to change it :)

[Joseph Pecoraro]

Comment on attachment 268249 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js:186
> +            for (let j = ~~currentColorIsHEX; j < hexFormats.length; ++j) {

~~?

[Joseph Pecoraro]

Comment on attachment 268249 [details]
Patch

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

>>> LayoutTests/inspector/model/color.html:158
>>> +            InspectorTest.expectThat(color.canBeSerializedAsShortHEX() === false, "'#11122233' should not be serializable as a short Hex");
>> 
>> I was hoping for: "11223345" because your change caught an issue in the `alpha` value being able to be shorted. But still good!
> 
> Yeah I wanted to do that, but I was having a hard time finding a hex whose alpha translated nicely into RGBA/HSLA.  The only ones that I could find that worked nicely were repeated numbers (multiples of 33 actually).  If you can find a nice alpha value (or if you don't think I even need to go that far), I'd be happy to change it :)

Heh, you don't have to pick an example that translates to other formats nicely since here you would just be testing color.canBeSerializedAsShortHex for a HEXAlpha.

Just a quick test like this would have been fine:

    color = WebInspector.Color.fromString("#11223344");
    InspectorTest.expectThat(color.canBeSerializedAsShortHEX() === true, "'#11223344' should be serializable as a short Hex");
    color = WebInspector.Color.fromString("#11223345");
    InspectorTest.expectThat(color.canBeSerializedAsShortHEX() === false, "'#11223345' should not be serializable as a short Hex");

[Devin Rousso]

Comment on attachment 268249 [details]
Patch

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

>>>> LayoutTests/inspector/model/color.html:158
>>>> +            InspectorTest.expectThat(color.canBeSerializedAsShortHEX() === false, "'#11122233' should not be serializable as a short Hex");
>>> 
>>> I was hoping for: "11223345" because your change caught an issue in the `alpha` value being able to be shorted. But still good!
>> 
>> Yeah I wanted to do that, but I was having a hard time finding a hex whose alpha translated nicely into RGBA/HSLA.  The only ones that I could find that worked nicely were repeated numbers (multiples of 33 actually).  If you can find a nice alpha value (or if you don't think I even need to go that far), I'd be happy to change it :)
> 
> Heh, you don't have to pick an example that translates to other formats nicely since here you would just be testing color.canBeSerializedAsShortHex for a HEXAlpha.
> 
> Just a quick test like this would have been fine:
> 
>     color = WebInspector.Color.fromString("#11223344");
>     InspectorTest.expectThat(color.canBeSerializedAsShortHEX() === true, "'#11223344' should be serializable as a short Hex");
>     color = WebInspector.Color.fromString("#11223345");
>     InspectorTest.expectThat(color.canBeSerializedAsShortHEX() === false, "'#11223345' should not be serializable as a short Hex");

Good point.  Created a new bug for this: https://bugs.webkit.org/show_bug.cgi?id=152809