Bug 193166 – Web Inspector: handle CSS Color 4 color syntaxes

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

[Simon Fraser (smfr)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/47062403>

[Devin Rousso]

Created attachment 358761 [details]
Patch

[Simon Fraser (smfr)]

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.

[Devin Rousso]

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.

[Simon Fraser (smfr)]

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.

[Devin Rousso]

Created attachment 358767 [details]
Patch

[Simon Fraser (smfr)]

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.

[Devin Rousso]

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%)`).

[WebKit Commit Bot]

Comment on attachment 358767 [details]
Patch

Rejecting attachment 358767 [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-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 358767, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=358767&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=193166&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 358767 from bug 193166.
Fetching: https://bugs.webkit.org/attachment.cgi?id=358767
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/ChangeLog' is out of date
W: fb5ce102a854a5fbe3ed20114179a623a23e671c and refs/remotes/origin/master differ, using rebase:
:040000 040000 5c6fa086a506eaf3ff0e8a1b325a94fbc5dce80d 588baa2b22fc6923312211f40013fe0e4f6a6647 M	LayoutTests
:040000 040000 ac7862ab69fef5d2bb84a4baa38727b049620324 14d551313a92449e3f18ff06fffe5925efe69e3b M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/ChangeLog' is out of date
W: fb5ce102a854a5fbe3ed20114179a623a23e671c and refs/remotes/origin/master differ, using rebase:
:040000 040000 5c6fa086a506eaf3ff0e8a1b325a94fbc5dce80d 588baa2b22fc6923312211f40013fe0e4f6a6647 M	LayoutTests
:040000 040000 ac7862ab69fef5d2bb84a4baa38727b049620324 14d551313a92449e3f18ff06fffe5925efe69e3b M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: https://webkit-queues.webkit.org/results/10904419

[Devin Rousso]

Created attachment 360252 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 360252 [details]
Patch

Clearing flags on attachment: 360252

Committed r240550: <https://trac.webkit.org/changeset/240550>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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.

[Devin Rousso]

(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.

[Joseph Pecoraro]

(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.