Closed Bug 1049436 Opened 7 years ago Closed 7 years ago

CSS variables containing name of a CSS color in their name display a colorpicker in the name

Categories

(DevTools :: Inspector, defect)

32 Branch
x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: romaric.pascal, Assigned: miker)

References

Details

Attachments

(3 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140731191115

Steps to reproduce:

1. Go to this JSfiddle, which shows a simple piece of text styled with:

:root {
  --some-kind-of-green: hsl(100, 50%, 50%);
}

body {
    color: var(--some-kind-of-green);
}

2. Open the dev tools and inspect the body tag to peek its color


Actual results:

The dev tools display the following (see attached screenshot):

body {
  color: var(--some-kind-of-<colorpicker set to CSS `green`>)
}


Expected results:

The dev tools should have displayed the name of the variable properly and not interpreted the `green` part as the name of a color.

Ideally, they'd also have displayed a color picker to edit that color, but that's probably a completely different issue.
Component: Untriaged → Developer Tools
Component: Developer Tools → Developer Tools: Inspector
This is not only true if they contain a color name, but in some other circumstances as well. (unless "highlight" is actually a valid color??)

Definition:
--color-highlight: hsl(200, 80%, 70%);
(this is actually #75C6EF in rgb!)
Usage:
color: var(--color-highlight);

Actual result in devtools (note that the color value itself is presented with a color picker):
color: var(--color-#4A90D9);

Clicking the value and closing it creates an invalid property value.
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 1054604
Depends on: 977063
> unless "highlight" is actually a valid color??

It's a valid color, yes; one of the system colors.  See http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp?rev=c3c9eea58fa2&mark=918-918#907
That said, the proposed fix in bug 977063 will NOT help this bug, as far as I can tell, since the variable value is being used in the "color" property....
Flags: needinfo?(mratcliffe)
Duplicate of this bug: 1056435
I have a way to fix this but have been busy getting devbrowser ready. The same fix also improves autocomplete and our detection of CSS property types.
Assignee: nobody → mratcliffe
Flags: needinfo?(mratcliffe)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> I have a way to fix this but have been busy getting devbrowser ready. The
> same fix also improves autocomplete and our detection of CSS property types.

Mike, do you have update / patch for this?
Flags: needinfo?(mratcliffe)
(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> > I have a way to fix this but have been busy getting devbrowser ready. The
> > same fix also improves autocomplete and our detection of CSS property types.
> 
> Mike, do you have update / patch for this?

Yes... the main problem is that the output parser has to rely upon bz's APIs for checking which data types a property supports. Some properties have custom parsers which cause these APIs to report the wrong data types resulting in bad rendering from the output parser and missing a bunch of autocompletions.

I have rewritten these APIs to fix these issues and added APIs that make it unnecessary to use hidden windows to validate CSS properties. This results in better performance and considerably shorter test logs.

I need to rewrite the output parser but because of the new APIs this should be a simple task and the result should be 100% accurate.

Of course, this needs fixing, especially now that we have CSS variables in our codebase.

I will work on this as soon as I finish my current patch.
Duplicate of this bug: 1112302
Attached file cssVarSwatch.html
Attached simple testcase.

I also have a very simple workaround that I will attach soon.
Flags: needinfo?(mratcliffe)
Depends on: 1112700
No longer depends on: 977063
Simple workaround for now... we can improve on this with my output parser work.
Attachment #8538433 - Flags: review?(bgrinstead)
Comment on attachment 8538433 [details] [diff] [review]
1049436-stop-color-swatch-for-css-vars.patch

Review of attachment 8538433 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/output-parser.js
@@ +11,5 @@
>  const HTML_NS = "http://www.w3.org/1999/xhtml";
>  
>  const MAX_ITERATIONS = 100;
>  const REGEX_QUOTES = /^".*?"|^".*|^'.*?'|^'.*/;
> +const REGEX_CSS_VAR = /\bvar\(--[a-zA-Z-]+\)/;

the name can also include 0-9 and there can be whitespace in between the beginning and end parens.  There are surely other cases that aren't captured by this regex, but it seems like an alright workaround.

Where is the output parser work you are referring to that can improve on this?
Attachment #8538433 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #12)
> Comment on attachment 8538433 [details] [diff] [review]
> 1049436-stop-color-swatch-for-css-vars.patch
> 
> Review of attachment 8538433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/output-parser.js
> @@ +11,5 @@
> >  const HTML_NS = "http://www.w3.org/1999/xhtml";
> >  
> >  const MAX_ITERATIONS = 100;
> >  const REGEX_QUOTES = /^".*?"|^".*|^'.*?'|^'.*/;
> > +const REGEX_CSS_VAR = /\bvar\(--[a-zA-Z-]+\)/;
> 
> the name can also include 0-9 and there can be whitespace in between the
> beginning and end parens.  There are surely other cases that aren't captured
> by this regex, but it seems like an alright workaround.
> 

I have changed this to follow the spec as _ and a unicode range were missing:
// CSS variable names are identifiers which the spec defines as follows:
//   In CSS, identifiers (including element names, classes, and IDs in
//   selectors) can contain only the characters [a-zA-Z0-9] and ISO 10646
//   characters U+00A0 and higher, plus the hyphen (-) and the underscore (_).
const REGEX_CSS_VAR = /\bvar\(\s--[-_a-zA-Z0-9\u00A0-\u10FFFF]+\s*\)/;

> Where is the output parser work you are referring to that can improve on
> this?

It is across a bunch of bugs but will eventually land as the fix for bug 971234 where I have added APIs to DOMUtils. I have improved property type detection, autocomplete etc. This means that we can use propertySupportsType() to choose what type of parsing can be done to property values depending on the property name.

I will also rename the output-parser.js to something like output-parser-css.js because it is only used for css.

Don't worry, I will ping you for review soon ;)
Attachment #8538433 - Attachment is obsolete: true
Attachment #8539384 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/52d616db7d5a
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Duplicate of this bug: 1087242
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.