Web Inspector: Hexadecimal color values in all CSS editors are purple when they should be blue

• Component: Web Inspector
• Status: RESOLVED FIXED

[Jonathan Wells]

CodeMirror 4 regression.

[Radar WebKit Bug Importer]

<rdar://problem/17348008>

[Jonathan Wells]

Created attachment 234206 [details]
[PATCH] Attempted fix.

[Joseph Pecoraro]

Comment on attachment 234206 [details]
[PATCH] Attempted fix.

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

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:185
> +        const hexColorRegex = /(#[0-9a-fA-F]{6}|#[0-9a-fA-F]{3})/g;

I would suggest:

    /#(?:[0-9a-fA-F){6}|[0-9a-fA-F){3})/g

Changes:

  1. Share the "#" because it is common
  2. Use a non-captured group (?:...) instead of (...) because you don't reference the group

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:203
> -            if (style === "atom" && stream.current() === "url") {
> +            if (style === "atom" && hexColorRegex.test(stream.current()))
> +                style = style + " hex-color";
> +            else if (style === "atom" && stream.current() === "url") {
>                  // If the current text is "url" then we should expect the next string token to be a link.
>                  state._expectLink = true;
>              } else if (state._expectLink && style === "atom") {

Nit: There are a lot of style === "atom" checks here. Can we pull that out into a bool above the if/else chain?

Also, it looks like you add style "hex-color" but the CSS below checks for .cm-hex-color. Does the ".cm-" prefix automatically get added somewhere?

[Joseph Pecoraro]

(In reply to comment #3)
> (From update of attachment 234206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234206&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:185
> > +        const hexColorRegex = /(#[0-9a-fA-F]{6}|#[0-9a-fA-F]{3})/g;
> 
> I would suggest:
> 
>     /#(?:[0-9a-fA-F){6}|[0-9a-fA-F){3})/g


In person discussion, we may want to gracefully handle something like:

    #12345678

So we might want to throw a \b on the end of the regex and not match this.

[Jonathan Wells]

I agree.

[Jonathan Wells]

Created attachment 234210 [details]
[PATCH] Attempted fix with cleanup.

Cleaned up the logic and saved the regexp test for later to lower the number of calls to the RegExp API.

[Joseph Pecoraro]

Comment on attachment 234210 [details]
[PATCH] Attempted fix with cleanup.

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

Awesome! r=me

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:185
> +        const hexColorRegex = /(#[0-9a-fA-F]{6}|#[0-9a-fA-F]{3})\b/g;

Nit: I think we still want at least the (?:...) non-capture group. As that can be a performance win with Regexs.

[Jonathan Wells]

Created attachment 234215 [details]
[PATCH] Reviewed fix with small edits.

[WebKit Commit Bot]

Comment on attachment 234215 [details]
[PATCH] Reviewed fix with small edits.

Clearing flags on attachment: 234215

Committed r170681: <http://trac.webkit.org/changeset/170681>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.