Web Inspector: "white" and "black" aren't recognized as color keywords

[Nikita Vasilyev]

Color picker doesn't show up for "color: white".

[Radar WebKit Bug Importer]

<rdar://problem/47068595>

[Nikita Vasilyev]

That's because "white" is the value keyboard for -apple-pay-button-style:

    -apple-pay-button-style: Array (3)
    0 "black"
    1 "white"
    2 "white-outline"

Value keywords have precedence over color keywords in External/CodeMirror/css.js:

    function wordAsValue(stream) {
      var word = stream.current().toLowerCase();
      if (valueKeywords.hasOwnProperty(word))
        override = "atom";
      else if (colorKeywords.hasOwnProperty(word))
        override = "keyword";
      else
        override = "variable";
    }

[Nikita Vasilyev]

Created attachment 358445 [details]
Patch

[Nikita Vasilyev]

Created attachment 358446 [details]
[Image] With patch applied

[Devin Rousso]

Comment on attachment 358445 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:47
> +            acceptedKeywords.push(...WI.CSSKeywordCompletions._colors);

NIT: the `concat` approach is faster <https://jsperf.com/array-concat-vs-push-2/17>.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:425
> +            let isVariable = this._property.name.startsWith("--");

NIT: you can use the `this._properety.variable` getter and inline it in the `if`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:570
> +            } else if (isNaN(colorFunctionStartIndex) && token.type && (token.type.includes("atom") || token.type.includes("keyword"))) {

What would happen with the following if this wasn't changed?

    -apple-pay-button-style: white;
    color: white;
    --foo: white;

[Nikita Vasilyev]

Comment on attachment 358445 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:47
>> +            acceptedKeywords.push(...WI.CSSKeywordCompletions._colors);
> 
> NIT: the `concat` approach is faster <https://jsperf.com/array-concat-vs-push-2/17>.

Good to know!

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:425
>> +            let isVariable = this._property.name.startsWith("--");
> 
> NIT: you can use the `this._properety.variable` getter and inline it in the `if`.

Neat!

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:570
>> +            } else if (isNaN(colorFunctionStartIndex) && token.type && (token.type.includes("atom") || token.type.includes("keyword"))) {
> 
> What would happen with the following if this wasn't changed?
> 
>     -apple-pay-button-style: white;
>     color: white;
>     --foo: white;

It wouldn't show the color picker anywhere.

[Nikita Vasilyev]

Created attachment 358465 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 358465 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:422
>              // FIXME: <https://webkit.org/b/178636> Web Inspector: Styles: Make inline widgets work with CSS functions (var(), calc(), etc.)

Does this address this FIXME? Maybe not completely?

[Nikita Vasilyev]

Comment on attachment 358465 [details]
Patch

(In reply to Joseph Pecoraro from comment #8)
> Comment on attachment 358465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358465&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:422
> >              // FIXME: <https://webkit.org/b/178636> Web Inspector: Styles: Make inline widgets work with CSS functions (var(), calc(), etc.)
> 
> Does this address this FIXME? Maybe not completely?

It doesn't address this FIXME at all. The FIXME is about cases like `linear-gradient(#fff2db, var(--myVar))`.

[WebKit Commit Bot]

Comment on attachment 358465 [details]
Patch

Clearing flags on attachment: 358465

Committed r239690: <https://trac.webkit.org/changeset/239690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.