Bug 193166

Summary: Web Inspector: handle CSS Color 4 color syntaxes
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, joepeck, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Simon Fraser (smfr) 2019-01-04 17:35:37 PST
In the latest CSS Color 4 draft <https://drafts.csswg.org/css-color/>, the following are true:

rgb() and rgba() are aliases.
hls() and hlsa() are aliases.

rgb() can take percentage or floating-point color components
rgb() components do not have to be separated with commas. A slash can separate the r,g,b from the alpha component.
The alpha component can be a percentage.

Similar behaviors exist for the hsl() function.

Web inspector needs to be able to parse these new formats.
Comment 1 Radar WebKit Bug Importer 2019-01-04 17:38:29 PST
<rdar://problem/47062403>
Comment 2 Devin Rousso 2019-01-09 16:12:51 PST
Created attachment 358761 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-01-09 16:22:15 PST
Comment on attachment 358761 [details]
Patch

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

> LayoutTests/inspector/model/color.html:70
> +            testGood("rgb(1 2 3)", WI.Color.Format.RGB);

You need to test percentage rgb components, and decimal values. See https://drafts.csswg.org/css-color/#rgb-functions for the allowed syntax.

Also you should test a mix of percentages and non-percentages, with and without the slash syntax.
Comment 4 Devin Rousso 2019-01-09 16:25:20 PST
Comment on attachment 358761 [details]
Patch

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

>> LayoutTests/inspector/model/color.html:70
>> +            testGood("rgb(1 2 3)", WI.Color.Format.RGB);
> 
> You need to test percentage rgb components, and decimal values. See https://drafts.csswg.org/css-color/#rgb-functions for the allowed syntax.
> 
> Also you should test a mix of percentages and non-percentages, with and without the slash syntax.

Crap!  I added support for that, but I forgot to test it 😅

Reading that document, it also looks like `deg` is supported with the hue of `hsl()` functions, so we should support that too.
Comment 5 Simon Fraser (smfr) 2019-01-09 16:31:43 PST
Comment on attachment 358761 [details]
Patch

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

>>> LayoutTests/inspector/model/color.html:70
>>> +            testGood("rgb(1 2 3)", WI.Color.Format.RGB);
>> 
>> You need to test percentage rgb components, and decimal values. See https://drafts.csswg.org/css-color/#rgb-functions for the allowed syntax.
>> 
>> Also you should test a mix of percentages and non-percentages, with and without the slash syntax.
> 
> Crap!  I added support for that, but I forgot to test it 😅
> 
> Reading that document, it also looks like `deg` is supported with the hue of `hsl()` functions, so we should support that too.

Not just deg. Any angle unit.
Comment 6 Devin Rousso 2019-01-09 17:11:09 PST
Created attachment 358767 [details]
Patch
Comment 7 Simon Fraser (smfr) 2019-01-09 17:22:19 PST
Comment on attachment 358767 [details]
Patch

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

> LayoutTests/inspector/model/color.html:236
> +            testBad("rgb(255, 255, 255, 0.5, 1)"); // extra values

I think you need more here. Things with a mix of % and number. Things with both commas and slash.
Comment 8 Devin Rousso 2019-01-09 17:29:15 PST
Comment on attachment 358767 [details]
Patch

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

>> LayoutTests/inspector/model/color.html:236
>> +            testBad("rgb(255, 255, 255, 0.5, 1)"); // extra values
> 
> I think you need more here. Things with a mix of % and number. Things with both commas and slash.

WebInspector actually has traditionally been very lenient with "invalid" values (e.g. you'll still get a swatch for `color: hsl(10, 20, 30);` even though it's missing "%" for saturation/lightness).  I'm of the opinion that we should continue this tradition.

The regular expressions I've used here are pretty lenient as to whether it actually is parseable, or whether it "looks" parseable.  Mixing commas/slashes or %/number/<other unit> is somewhat irrelevant as to whether WebInspector can recognize what you've written as a color value.  I don't think we want to re-implement the CSS parser in the WebInspector frontend.

Perhaps we can add a button to the swatch UI that says something like "we figured out what color you were trying to display, but you didn't write it correctly, so click [here] to print what it should be" (e.g. it would replace `hsl(10, 20, 30)` with `hsl(10, 20%, 30%)`).
Comment 9 WebKit Commit Bot 2019-01-26 14:33:01 PST Comment hidden (obsolete)
Comment 10 Devin Rousso 2019-01-26 14:38:25 PST
Created attachment 360252 [details]
Patch
Comment 11 WebKit Commit Bot 2019-01-26 15:54:35 PST
Comment on attachment 360252 [details]
Patch

Clearing flags on attachment: 360252

Committed r240550: <https://trac.webkit.org/changeset/240550>
Comment 12 WebKit Commit Bot 2019-01-26 15:54:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Joseph Pecoraro 2019-01-27 21:44:27 PST
This doesn't update `createCodeMirrorColorTextMarkers` in CodeMirrorTextMarkers.js. That means that we won't get inline editors for colors with the new syntax in CSS resources.
Comment 14 Devin Rousso 2019-01-28 09:22:56 PST
(In reply to Joseph Pecoraro from comment #13)
> This doesn't update `createCodeMirrorColorTextMarkers` in
> CodeMirrorTextMarkers.js. That means that we won't get inline editors for
> colors with the new syntax in CSS resources.
The regex in `createCodeMirrorColorTextMarkers` is able to handle these syntaxes.  It's much more general, and really relies more on `WI.Color.fromString` to do the parsing.
Comment 15 Joseph Pecoraro 2019-01-28 13:09:56 PST
(In reply to Devin Rousso from comment #14)
> (In reply to Joseph Pecoraro from comment #13)
> > This doesn't update `createCodeMirrorColorTextMarkers` in
> > CodeMirrorTextMarkers.js. That means that we won't get inline editors for
> > colors with the new syntax in CSS resources.
> The regex in `createCodeMirrorColorTextMarkers` is able to handle these
> syntaxes.  It's much more general, and really relies more on
> `WI.Color.fromString` to do the parsing.

Interesting, you're right. I didn't realize it was so generic.