Web Inspector: Styles Redesign: apply syntax highlighting to property values

[Nikita Vasilyev]

Chrome DevTools only highlight URLs:
  - url(foo.png)
  - url('foo.png')
  - url("foo.png")

At the very least we should do the same, but it would be also nice to highlight:

Strings:
  - content: 'blah'
  - font-family: "Myriad Set Pro", "Helvetica Neue", sans-serif;

Variables (highlight the --my-var part):
  - var(--my-var)

Non matching pair characters:
  - content: 'blah
  - font-family: "Myriad Set Pro, "Helvetica Neue", sans-serif;
  - calc(var(--my-var + 5)

---

It's possible to feed CodeMirror a CSS string and get a list of tokens back. There's no need to write our own parser.

https://codemirror.net/doc/manual.html#modeapi

<rdar://problem/33526532>

[Nikita Vasilyev]

Created attachment 323508 [details]
Patch

This patch highlights links and CSS strings. I think it's a good start.

[Nikita Vasilyev]

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

[Nikita Vasilyev]

Created attachment 323615 [details]
Patch

[Nikita Vasilyev]

Created attachment 323820 [details]
Patch

[Nikita Vasilyev]

Created attachment 323821 [details]
[Image] Long URI truncated

I addressed Bug 124779 - Web Inspector: Styles: Truncate long URLs (e.g. base64)
in this patch since it was pretty straightforward to do once we detect URL tokens.

Reduction page: http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/base64.html

I truncated URLs over 150 characters. This is the same length as in Chrome DevTools.

I've learned that there's no pure-CSS way to truncate an inline element :(

[Matt Baker]

Comment on attachment 323820 [details]
Patch

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

Looking good, just a minor correction and some nits.

> Source/WebInspectorUI/ChangeLog:26
> +        Highlight links and CSS strings in values when isn't editing.

Type: when not editing.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:686
> +    let cssRule = `*{X:${cssValue}}`;

I was going to suggest a different label for the fake property name, to make ita little more obvious, but then the ignore column index would need to change, and it just gets harder to figure out (and more regression prone).

How about just replacing the magic number 4 with a const, which can be defined after the definition of `cssRule`. Then the meaning of each will be reinforced.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:695
> +            // Ignore '*{X:'

This comment could move to be inline with the definition of the const mentioned above.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:244
> +                let maxLength = 150;

Just to make it stand out, I'd move this to the top of the function:

_renderValue(value)
{
    const maxValueLength = 150;

    ...
}

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:247
> +            } else

You can remove the else, since both branches return.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:82
> +        if (this._delegate && typeof this._delegate.spreadsheetTextFieldWillStartEditing)

=== "function"

[Nikita Vasilyev]

Comment on attachment 323820 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:686
>> +    let cssRule = `*{X:${cssValue}}`;
> 
> I was going to suggest a different label for the fake property name, to make ita little more obvious, but then the ignore column index would need to change, and it just gets harder to figure out (and more regression prone).
> 
> How about just replacing the magic number 4 with a const, which can be defined after the definition of `cssRule`. Then the meaning of each will be reinforced.

What about:

    const prefix = "*{X:";
    let cssRule = prefix + cssValue + "}";

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:695
>> +            // Ignore '*{X:'
> 
> This comment could move to be inline with the definition of the const mentioned above.

...and

    if (column < prefix.length)

?

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:82
>> +        if (this._delegate && typeof this._delegate.spreadsheetTextFieldWillStartEditing)
> 
> === "function"

Whoops!

[Nikita Vasilyev]

Created attachment 323968 [details]
Patch

[Matt Baker]

Comment on attachment 323968 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 323968 [details]
Patch

Clearing flags on attachment: 323968

Committed r223453: <https://trac.webkit.org/changeset/223453>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.