1145527 - [Inspector] Useful CSS Variables support

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, enhancement, P2)

Description

[Julien Wajsberg [:julienw]]

Attachment: testcase-variables.html

STR:
1. load the attachment.
2. Inspect the div (containing the text)
3. Look at the "color" property

Expected:
* We can see which variable is used (you see that --var1 does not exist, and thus --var2 is used).
* We can see the actual value used with the variable, likely on hover. Yet for some value types (eg: colors) we could see it besides just like a normal color.
* We could somehow access where the variable is defined (if it's defined)

Comment 1

[Tom Tromey :tromey]

Attachment: Screenshot from 2015-12-16 13-09-46.png

Screenshot showing rule view with new test.

Comment 2

[Tom Tromey :tromey]

Attachment: fix most eslint warnings in output-parser.js

Comment 3

[Tom Tromey :tromey]

Attachment: clean up uses of _strings in styleinspector

Comment 4

[Tom Tromey :tromey]

Attachment: display css variable values in rule view

Comment 5

[Tom Tromey :tromey]

Comment on attachment 8699159 [details] [diff] [review]
fix most eslint warnings in output-parser.js

This cleans up most of the eslint warnings in output-parser.js.
One remains, but it is a false positive from the removeEventListener rule.

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8699160 [details] [diff] [review]
clean up uses of _strings in styleinspector

This cleans up some use of _strings in styleinspector.

rule-view is already importing _strings by itself; so
referring to CssLogic._strings is mildly confusing, and also
IMO a bit wrong as I would expect the leading underscore to
indicate a private member.

The _strings definition in styleinspector.js seems to be unused,
so this garbage collects it.

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8699161 [details] [diff] [review]
display css variable values in rule view

The attached screen shot shows what this looks like in operation.

The basic idea is to notice which branch of a var(,) is taken.
The not-taken branch is dimmed, the same way that some selectors
are dimmed.

Hovering over the variable name in a var() will show the current value.

The test case here caught a latent bug in output-parser due to a typo:
"token.token" was supposed to be "token.text".

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8699161 [details] [diff] [review]
display css variable values in rule view

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

That's a clear improvement from what we have today. Nice work.
Some general comments first:

- How hard would it be to show color swatches nest to variable definitions when they are of color values?
Like, show a red color switch next to "red" in: "--var2: red;"
This is most likely a separate bug.
- Similarly, how hard would it be to show a read-only color swatch next to where a variable is used?
With this: "color: var(--var1);" we could show a color swatch before "var" but make it read-only or something.
Again, most likely an other bug. And I wonder how this would look when you use variables as default values.
Related, I just discovered that you can nest default values like crazy: "color: var(--var1, var(--var2, var(--var3, var(--var4))));"
I don't know why anyone would do that, but anyway ...
- I found a bug: 
On this page: https://bug1145527.bmoattachments.org/attachment.cgi?id=8580588
Select the <div>
In the rule view, change "--var2: red" to "--var2: green" *BUT* don't press TAB, just click elsewhere to blur the field.
Now hover over "var(--var2)" on the line below ==> The tooltip still says "--var2 = red".
Note that if you pressed TAB at the step before, it would correctly show "--var2 = green" because you'd have entered the edit mode on the property and that would have refreshed the output.
It's easier to reproduce if the variable definitions are in another rule, like ":root {--var2: red;}".

::: devtools/client/shared/output-parser.js
@@ +107,5 @@
> +   * that were seen.  The stopping token (paren or comma) will not
> +   * be included.
> +   * |result| is the text of that was collected.  The stopping token's
> +   * text will not be included.
> +   * |sawComma| is true if the stop was due to a command, or false

s/command/comma

@@ +173,5 @@
> +   * tokens up to and including the ")" that closes the "var("
> +   * invocation, and will add nodes to |this.parsed| as necessary.
> +   *
> +   * @param {CSSToken} initialToken the "var(" token that was already
> +   *seen.

nit: add a space between "*" and "seen"

@@ +210,1 @@
>      }

The firstOpts and secondOpts objects confused me a little at first, I had to spend quite some time reading through the code to understand.
I would almost not use them at all in fact, I think there's a way to make this code more easily readable:

let isVariableValid = typeof varValue === "string";

this.parsed.push(this._createNode("span", {
  "class": isVariableValid ? "" : options.unmatchedVariableClass,
  "title": isVariableValid
           ? _strings.formatStringFromName("rule.variableValue",
                                           [tokens[0].text, varValue], 2)
           : _strings.formatStringFromName("rule.variableUnset",
                                           [tokens[0].text], 1)
}, result));

@@ +221,5 @@
> +      subOptions.expectFilter = false;
> +      let saveParsed = this.parsed;
> +      this.parsed = [];
> +      let rest = this._doParse(text, subOptions, tokenStream, true);
> +      this.parsed = saveParsed;

Having to temporarily empty this.parsed so that you can do a sub-parse feels bad, maybe we should change the way parsed CSS parts are stored.
Or, you could almost instantiate a new OutputParser here:

let subParser = new OutputParser(this.doc);

and use this to parse the remainder of the var function. This way you wouldn't have to re-initialize this.parsed, but would just append the result of subParser to it.

@@ +223,5 @@
> +      this.parsed = [];
> +      let rest = this._doParse(text, subOptions, tokenStream, true);
> +      this.parsed = saveParsed;
> +
> +      let span = this._createNode("span", secondOpts);

Removing the secondOpts variable as commented earlier means you'd have to do this here too:

let span = this._createNode("span", {
  class: isVariableValid ? options.unmatchedVariableClass : ""
});

@@ +680,5 @@
>     *                                    // "filter" property, causing the
>     *                                    // parser to skip the call to
>     *                                    // _wrapFilter.  Used only for
>     *                                    // previewing with the filter swatch.
> +   *           - isVariableInUse        // A function taking a single

The name is a bit misleading, without the description, I would expect it to return a boolean. But reading the description and the code, I understand that it needs to return the value if any.
The name should make this self explanatory.
getVariableValue ?

::: devtools/client/styleinspector/rule-view.js
@@ +3480,5 @@
>        defaultColorType: !propDirty,
>        urlClass: "theme-link",
> +      baseURI: this.sheetURI,
> +      unmatchedVariableClass: "ruleview-variable-unmatched",
> +      isVariableInUse: (varName) => this.rule.elementStyle.getVariable(varName),

nit: no need for parens around varName, and no need for the last comma

::: devtools/client/styleinspector/test/browser_ruleview_variables.js
@@ +13,5 @@
> +  let {inspector, view} = yield openRuleView();
> +  yield selectNode("#target", inspector);
> +
> +  let unsetColor = getRuleViewProperty(view, "div", "color").valueSpan.
> +      querySelector(".ruleview-variable-unmatched");

nit: we usually place the dot '.' on the next line:
let unsetColor = getRuleViewProperty(view, "div", "color".valueSpan
                 .querySelector(".ruleview-variable-unmatched");

::: devtools/shared/locales/en-US/styleinspector.properties
@@ +80,5 @@
> +# LOCALIZATION NOTE (rule.variableValue): Text displayed in a tooltip
> +# when the mouse is over a variable use (like "var(--something)") in
> +# the rule view.  The first argument is the variable name and the
> +# second argument is the value.
> +rule.variableValue=%S = %S

I think it's better to use %1$S and %2$S as per https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_ordered_variables_in_string_with_multiple_variables

Comment 9

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8)

> - How hard would it be to show color swatches nest to variable definitions
> when they are of color values?

That's bug 1222774.  It's a bit difficult in that it isn't always clear exactly
where the swatch should appear, or what behavior it should have.  Let's discuss there.

> - Similarly, how hard would it be to show a read-only color swatch next to
> where a variable is used?
> With this: "color: var(--var1);" we could show a color swatch before "var"
> but make it read-only or something.

It's maybe mildly difficult because we don't know that much about the parsing
of shorthand properties.

> Or, you could almost instantiate a new OutputParser here:
> 
> let subParser = new OutputParser(this.doc);
> 
> and use this to parse the remainder of the var function. This way you
> wouldn't have to re-initialize this.parsed, but would just append the result
> of subParser to it.

We want to keep using the same token stream, so this wouldn't work directly.
But I'll find a nicer way to encapsulate the save/restore here.

Comment 10

[Tom Tromey :tromey]

A couple more notes on this -

> let subParser = new OutputParser(this.doc);

I looked at this but the parser also maintains other state, like colorSwatches
and the mouse-down event handler.  So, I think it's probably better not to
make a new parser.  Instead I put the save/restore code directly into _doParse,
which I think is at least a bit cleaner.

> > +      isVariableInUse: (varName) => this.rule.elementStyle.getVariable(varName),
> 
> nit: no need for parens around varName, and no need for the last comma

Brian pointed out in a different review some time ago that including the trailing
comma means that future additions don't need to modify the previous line, thus
preserving the history a bit more nicely.  So since that I've generally added
the trailing comma.

Comment 11

[Tom Tromey :tromey]

This also does the wrong thing when the var use is inside some other function
that is specially treated, like

color: rgb(x, y, var(--z))

This happens because _collectFunctionText isn't handling variable uses.
I think a bit more refactoring is needed here.

Comment 12

[Julian Descottes [:jdescottes]]

Inspector bug triage. Filter on CLIMBING SHOES.

Going to flag P2/enhancement, because while it's not a bug, better support for CSS variables would be really nice.

See also Bug 1262425.

Comment 13

[Julian Descottes [:jdescottes]]

Tom: do you want to keep working on this bug ? 
(I know you submitted patches, but since there was no activity for a few months, I prefer to check :) )

Comment 14

[Tom Tromey :tromey]

Sorry about the delay.
Realistically I'm not working on this right now.
I can always reacquire it if things change.

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8699159 [details] [diff] [review]
fix most eslint warnings in output-parser.js

Setting this patch as obsolete since it seems most of the eslint cleanup are already done.

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8699160 [details] [diff] [review]
clean up uses of _strings in styleinspector

Seems like this was also fixed. Marking as obsolete

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: attachment.cgi?id=8699161

Rebased the existing patch.

Tom, can you comment on what needs to be done to get this working as well as describe the current state of the patch? I know from playing around with it that we still aren't able to display the set CSS variables unless I am mistaken.

Comment 18

[Tom Tromey :tromey]

Attachment: css-tn.html

> Tom, can you comment on what needs to be done to get this working as well as describe the current state of the patch? 

This new example shows one problem.  Open the test case and select the <span>
in the inspector.  Here, "--foo" is set and visibly affects the style, but it can't
be seen in the rule view.

Comment #11 points out another problem and is still applicable.

> I know from playing around with it that we still aren't able to display the set CSS variables unless I am mistaken.

I didn't understand this, sorry - can you say more?

Comment 19

[Tom Tromey :tromey]

:gl asked me on irc to provide some more details.

I think the main remaining problem is that empty rules are filtered out.
However, tracking back from there, I think the problem is here:

https://dxr.mozilla.org/mozilla-central/rev/b06968288cff469814bf830aa90f1c84da490f61/devtools/client/inspector/rules/models/rule.js#456

This bit of code decides that a variable definition is an invisible property.
Things seem to work a bit better if I hack in:

+      if (/^--/.test(name)) {
+        invisible = false;
+      }

However, aside from its ugliness (and I think there's a predicate somewhere for
"is this a css variable name"), I suspect that the fix should rather be in
the definition of isInherited:

https://dxr.mozilla.org/mozilla-central/rev/b06968288cff469814bf830aa90f1c84da490f61/devtools/shared/fronts/css-properties.js#152

... but that would require auditing other users, if any (I didn't check).


The other thing to change in this patch is to rework the output-parser code to
address comment #11.  Maybe _parseMatchingParens could call _parseVariable when
appropriate, or something like that.

Comment 20

[Rahul Chaudhary [:rahulc]]

Attachment: Bug 1145527 - [Inspector] CSS Variable support to see CSS variable values on hover;

Features implemented:

> We can see which variable is used (you see that --var1 does not exist, and thus --var2 is used).

> We can see the actual value used with the variable, likely on hover.

Review commit: https://reviewboard.mozilla.org/r/121292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121292/

Comment 21

[Rahul Chaudhary [:rahulc]]

Attachment: testcase-variables.html

Added a fix for the bugs mentioned above in comment #11, comment #18, and comment #19. A lot of the code is from :tromney rebased by :gl in comment #17 and I haven't looked through all of it, so I apologize for any errors.
Inspect the 2 divs in the new .html file, as well as the div in css-tn.html in comment #18 to check and see if the behavior is as intended.

Comment 22

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8848385 [details]
Bug 1145527 - [Inspector] CSS Variable support to see CSS variable values on hover;

Passing this over to Tom

Comment 23

[Tom Tromey :tromey]

Comment on attachment 8848385 [details]
Bug 1145527 - [Inspector] CSS Variable support to see CSS variable values on hover;

https://reviewboard.mozilla.org/r/121292/#review123398

Thank you for working on this.

I have a number of comments.  Please don't be discouraged -- this is a complicated sort of patch and I think really most of my comments come down to clarifying the parseMatchinParens function contract.

::: commit-message-3d341:1
(Diff revision 1)
> +Bug 1145527 - [Inspector] Useful CSS Variables support; r?gl

I think rather than the bug title here, this should be   more closely related to what the patch does.  (I think this bug title isn't really that descriptive of the bug contents...)

::: devtools/client/inspector/rules/models/element-style.js:295
(Diff revision 1)
>          (!!computedProp.overridden !== overridden);
>        computedProp.overridden = overridden;
>        if (!computedProp.overridden && computedProp.textProp.enabled) {
>          taken[computedProp.name] = computedProp;
> +
> +        if (computedProp.name.startsWith("--")) {

Maybe this could use css-properties.js:isCssVariable instead.  That would have to be exported there, but that seems reasonable to do.

::: devtools/client/shared/output-parser.js:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const Services = require("Services");

The move of this line seems unnecessary.

::: devtools/client/shared/output-parser.js:113
(Diff revision 1)
> +   * |tokens| is a list of the non-comment, non-whitespace tokens
> +   * that were seen. The stopping token (paren or comma) will not
> +   * be included.
> +   * |result| is the text of that was collected. The stopping token's
> +   * text will not be included.
> +   * |sawComma| is true if the stop was due to a comma, or false otherwise.
> +   * |variableData| is an object containing texts and nodes of CSS
> +   * variables received from _parseVariable() and used by _appendVariable().
> +   * |sawVariable| is true if a variable was seen while parsing the text.
>     */

This comment doesn't explain the meaning of `functionTexts`.

I have some difficulty understanding the output of this function; like why the tokens and the result text are both needed here, and what exactly is going on with functionTexts and variableData.  I wonder if there's maybe a simpler way to do this.  And, I wonder if this does the right thing in cases where functions and variables are interleaved, like if this called with a token stream like "var(...) nonfunction rgb(...) othernonfunction var(...)".

One thing that would help would be more complete test coverage, I'll note that in the appropriate spot.

::: devtools/client/shared/output-parser.js:149
(Diff revision 1)
>          } else if (token.text === ")") {
>            --depth;
> +          if (depth === 0) {
> +            break;
> -        }
> +          }
> -      } else if (token.tokenType === "function") {
> +        } else if (token.tokenType === "function") {

It looks to me like this "if" can no longer be taken now, because now it is nested in `if (token.tokenType === "symbol")`.  I think it has to be pulled out of one more level of braces.

::: devtools/client/shared/output-parser.js:152
(Diff revision 1)
> +            break;
> -        }
> +          }
> -      } else if (token.tokenType === "function") {
> +        } else if (token.tokenType === "function") {
> -        ++depth;
> +          ++depth;
> -      }
> +        }
> +      } else if (token.text === "var" && options.isVariableInUse) {

It seems like this should check the token type against "function", to exclude other uses of the string or symbol "var".

::: devtools/client/shared/output-parser.js:159
(Diff revision 1)
> +        let {resultTexts, resultNodes} = this._parseVariable(token, text,
> +                                                             tokenStream, options);
> +        variableData[functionTexts.length - 1] = {resultTexts, resultNodes};
> -    }
> +      }
> -    return result;
> +
> +      if (token.text !== "var") {

Will this do the right thing when `isVariableInUse` is false?

::: devtools/client/shared/output-parser.js:192
(Diff revision 1)
> +   * |functionTexts| is a list of strings containing the function call texts.
> +   * |variableData| is an object containing texts and nodes of CSS
> +   * variables received from _parseVariable() and used by _appendVariable().
> +   * |sawVariable| is true if a variable was seen while parsing the text.
> +   */
> +  _collectFunctionText: function (initialToken, text, tokenStream, options) {

initialToken doesn't seem to be used here, so it should just be removed

::: devtools/client/shared/output-parser.js:293
(Diff revision 1)
> +   *
> +   * @param {String} text the original input text
> +   * @param {Object} options the options object in use; @see
> +   *_mergeOptions.
> +   * @param {CSSLexer} tokenStream the token stream from which to read
> +   * @param {Boolean} stopAtComma If true, stop at an umatched close

This parameter is called stopAtCloseParen

::: devtools/client/shared/output-parser.js:360
(Diff revision 1)
> +              if (variableData.hasOwnProperty(-1)) {
> +                let {resultTexts, resultNodes} = variableData[-1];
> +                this._appendVariable(resultTexts, resultNodes);
> +              }

I find this -1 pretty mysterious.  I think there must be a clearer way to handle this, but I don't really know what it is, since I don't have a clear view of where the -1 entry comes from.

::: devtools/client/shared/output-parser.js:368
(Diff revision 1)
> +              }
> +
> +              for (let i = 0; i < functionTexts.length; i++) {
> +                this._appendTextNode(functionTexts[i]);
> +
> +                if (variableData.hasOwnProperty(i)) {

I'd like it if there were a way to avoid the hasOwnProperty calls.

::: devtools/client/shared/output-parser.js:672
(Diff revision 1)
>    /**
> +   * Append a CSS variable to the output state such that the values of the
> +   * variables can be seen on hover, adds nodes to |this.parsed| as necessary.
> +   *
> +   * @param {Object} texts
> +   *        List of strings cotaining the text of a CSS variable besides the variables

Typos, "containing"; and "variables" should probably be "variable"

::: devtools/client/shared/output-parser.js:685
(Diff revision 1)
> +    if (nodes.length > 0) {
> +      this._appendTextNode(texts[nodes.length]);
> +    }

I found this segment perplexing for a bit.  Maybe still.  I think it would be clearer perhaps if the jsdoc for this function explained the relationship between the lengths of the |text| and |nodes| arrays.

::: devtools/client/shared/test/browser_outputparser.js:320
(Diff revision 1)
> +    {
> +      text: "var(--not-seen, var(--seen))",
> +      variables: {"--seen": "chartreuse" },
> +      expected: "var(<span class=\"unmatched-class\" title=\"--not-seen is not set\">--not-seen</span>,<span> var(<span title=\"--seen = chartreuse\">--seen</span>)</span>)"
> +    },
> +  ];

I think it would be good to have several more tests here, like tests involving other functions, nested function calls, and text of various kinds that is between or nested in function calls.  Given the "var" test in the patch it would be good to have a test involving a use of var in a string constant to make sure this is doing the right thing as well.

Comment 24

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8848385 [details]
Bug 1145527 - [Inspector] CSS Variable support to see CSS variable values on hover;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121292/diff/1-2/

Comment 25

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8848385 [details]
Bug 1145527 - [Inspector] CSS Variable support to see CSS variable values on hover;

https://reviewboard.mozilla.org/r/121292/#review123398

Thanks for all the feedback. I was a bit hesitant in touching too many different function headers and tried to avoid it, but I think that just made the code more complicated.
I refactored it again, hopefully it should be much easier to read now, although certain parts of the code may need comments for further clarification.
I haven't added any more tests yet, but will do so in the next few commits.

Comment 26

[Tom Tromey :tromey]

Comment on attachment 8848385 [details]
Bug 1145527 - [Inspector] CSS Variable support to see CSS variable values on hover;

https://reviewboard.mozilla.org/r/121292/#review128658

Thank you again for working on this.
This is looking better to me.  I have a few more comments.
Also I still think this needs more tests than it currently has, basically just adding some more cases to the test already being modified.

::: commit-message-3d341:6
(Diff revision 2)
> +Bug 1145527 - [Inspector] CSS Variable support to see CSS variable values on hover; r?tromey
> +
> +Features implemented:
> +
> +> We can see which variable is used (you see that --var1 does not exist, and thus --var2 is used).
> +> We can see the actual value used with the variable, likely on hover.

You should remove the "likely" here

::: devtools/client/shared/output-parser.js:153
(Diff revision 2)
>            --depth;
> +          if (depth === 0) {
> +            break;
> +          }
>          }
> +      } else if (token.text === "var" && options.isVariableInUse) {

I think this ought to check the token's type as well.

::: devtools/client/shared/output-parser.js:161
(Diff revision 2)
> +        functionData.push(variableNode);
>        } else if (token.tokenType === "function") {
>          ++depth;
>        }
> +
> +      if (token.text !== "var" || !options.isVariableInUse) {

I think this ought to check the token's type as well.

::: devtools/client/shared/output-parser.js:202
(Diff revision 2)
> +    let variableNode = this._createNode("span", {}, varText);
> +
> +    // Now emit the first part of the variable. If the variable is
> +    // not valid, mark it unmatched.
> +    let {tokens, functionData, sawComma, sawVariable} =
> +        this._parseMatchingParens(text, tokenStream, options, true);

If I understand correctly, this call is expected to just parse a single token -- the variable name -- and then return.  If so then it seems to me that this could be done much more simply inline, not involving a call to a complicated function.

::: devtools/client/shared/output-parser.js:204
(Diff revision 2)
> +    // Now emit the first part of the variable. If the variable is
> +    // not valid, mark it unmatched.
> +    let {tokens, functionData, sawComma, sawVariable} =
> +        this._parseMatchingParens(text, tokenStream, options, true);
> +    let result = [];
> +    if (sawVariable) {

I dont't really understand this.  I think a comment explaining the reason for this check would be helpful.

::: devtools/client/shared/output-parser.js:214
(Diff revision 2)
> +
> +    let varValue;
> +    let firstOpts = {};
> +    let secondOpts = {};
> +
> +    if (tokens && tokens.length === 1) {

FWIW this is the line that made me think that the earlier call to `_parseMatchingParens` isn't really needed.

::: devtools/client/shared/output-parser.js:220
(Diff revision 2)
> +      varValue = options.isVariableInUse(tokens[0].text);
> +    }
> +
> +    if (typeof varValue === "string") {
> +      firstOpts.title =
> +        STYLE_INSPECTOR_L10N.getFormatStr("rule.variableValue", tokens[0].text, varValue);

I think it's a little better here to use the token's start and end locations to extract the original text from |text|.  This approach preserves the original source spelling... a bit odd perhaps but there's a request in another bug that we try not to change tokens when displaying them.

::: devtools/client/shared/output-parser.js:225
(Diff revision 2)
> +        STYLE_INSPECTOR_L10N.getFormatStr("rule.variableValue", tokens[0].text, varValue);
> +      secondOpts.class = options.unmatchedVariableClass;
> +    } else {
> +      firstOpts.class = options.unmatchedVariableClass;
> +      firstOpts.title = STYLE_INSPECTOR_L10N.getFormatStr("rule.variableUnset",
> +                                                          tokens[0].text);

Here too.

::: devtools/client/shared/output-parser.js:323
(Diff revision 2)
> +
> +            if (sawVariable) {
> +              // if function contains variable, we need to add both strings
> +              // and nodes.
> +              this._appendTextNode(functionName);
> +              for (let i = 0; i < functionData.length; i++) {

I think a for-of here would be cleaner.

::: devtools/client/shared/output-parser.js:335
(Diff revision 2)
> +              }
> +              this._appendTextNode(")");
> +            } else {
> +              // if no variable in function, join the text together and add
> +              // to DOM accordingly.
> +              let functionText = functionName + functionData.join("");

Does this properly handle the closing ")"?

::: devtools/shared/fronts/css-properties.js:355
(Diff revision 2)
>    CssPropertiesFront,
>    CssProperties,
>    getCssProperties,
>    getClientCssProperties,
> -  initCssProperties
> +  initCssProperties,
> +  isCssVariable

Trailing comma here, please.

Comment 27

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8848385 [details]
Bug 1145527 - [Inspector] CSS Variable support to see CSS variable values on hover;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121292/diff/2-3/

Comment 28

[Tom Tromey :tromey]

Comment on attachment 8848385 [details]
Bug 1145527 - [Inspector] CSS Variable support to see CSS variable values on hover;

https://reviewboard.mozilla.org/r/121292/#review168856

Thank you for the update.  This is looking very good.

I found a couple of nits.  Once those are fixed we can push through try and land it.  Do you have try access?  If not, let me know, and when there's a new patch, I will take care of the testing.

::: devtools/client/shared/output-parser.js:162
(Diff revisions 2 - 3)
> -      if (token.text !== "var" || !options.isVariableInUse) {
> +      if ((token.tokenType !== "function" && token.text !== "var")
> +          || !options.isVariableInUse) {

This test looks wrong.  It seems like it should be the inverse of the earlier test, so

if (token.tokenType !== "function" || token.text !== "var" || ...)

::: devtools/client/shared/output-parser.js:203
(Diff revision 3)
> +    // The call to _parseMatchingParens is to support multiple levels
> +    // of variable nesting (eg. var(x, var(y, var(z))))
> +    let {tokens, functionData, sawComma, sawVariable} =
> +        this._parseMatchingParens(text, tokenStream, options, true);

It seems like this comment is a response to a comment of mine from an earlier review.

I guess calling `_parseMatchingParens` here is
ok; I've changed my mind a bit from earlier because I guess this at least skips comments and whitespace, and so inlining the logic isn't really that simple.

However, I think this comment is incorrect, because this doesn't actually handle multiple levels of variable nesting.  If I understand correctly, this call is just fetching the variable name (the `x` in `var(x ...`), which can't be nested.

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1145527 - Display the inherited and used CSS Variable in the rule view.

- Displays which CSS variable is used in the rule view.
- Displays the actual variable value on the variable name's title property.
- Displays all the inherited CSS variables on the selected element in the rule view.

Review commit: https://reviewboard.mozilla.org/r/174128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174128/

Comment 30

[Tom Tromey :tromey]

Comment on attachment 8902528 [details]
Bug 1145527 - Display the inherited and used CSS Variable in the rule view.

https://reviewboard.mozilla.org/r/174128/#review179554

Thanks for doing this.  This looks good.

I think it would be good to change the authorship on the commit back to Rahul, since he did most of the work.  This is ok with that change.

Comment 31

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Tom Tromey :tromey from comment #30)
> Comment on attachment 8902528 [details]
> Bug 1145527 - Display the inherited and used CSS Variable in the rule view.
> 
> https://reviewboard.mozilla.org/r/174128/#review179554
> 
> Thanks for doing this.  This looks good.
> 
> I think it would be good to change the authorship on the commit back to
> Rahul, since he did most of the work.  This is ok with that change.

Thanks for the review Tom. No worries about the authorship since that was always my intent.

Comment 32

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f94413e7a50bddac7925d0e5c37ded1625b413a

Comment 33

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1145527 - Display the inherited and used CSS Variable in the rule view [1.0]

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5842f3376fc
Display the inherited and used CSS Variable in the rule view. r=tromey

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b5842f3376fc

Comment 36

[Francesco Lodolo [:flod]]

(In reply to Patrick Brosset <:pbro> from comment #8)
> ::: devtools/shared/locales/en-US/styleinspector.properties
> @@ +80,5 @@
> > +# LOCALIZATION NOTE (rule.variableValue): Text displayed in a tooltip
> > +# when the mouse is over a variable use (like "var(--something)") in
> > +# the rule view.  The first argument is the variable name and the
> > +# second argument is the value.
> > +rule.variableValue=%S = %S
> 
> I think it's better to use %1$S and %2$S as per
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_content_best_practices#Use_ordered_variables_in_string_with_mult
> iple_variables

For the record, it is. In this case locales won't need to invert the placeholders, but that's pretty common in a sentence.

Comment 37

[Sebastian Zartner [:sebo]]

I could reproduce the issue using 57.0a1 (2017-08-20) and can confirm that it's fixed in 57.0a1 (2017-09-01), using Windows 10 64-bit.

Great work! That's a big step forward in CSS Variables inspection. Hope to see more soon!

Sebastian

Comment 38

[Andy Mercer]

I just found this bug because I decided to try CSS variables for the first time today. I'm running Firefox 59.0, on Windows 10. The color dot is not showing up.

Screenshot: https://imgur.com/a/p2Oik

Comment 39

[Andy Mercer]

Here is a short screencast. When I change the direct value away from using the variable, to instead using a raw color value, the color picker appears.

https://imgur.com/a/C6ZGT

Comment 40

[Sebastian Zartner [:sebo]]

(In reply to Andy Mercer from comment #38)
> I just found this bug because I decided to try CSS variables for the first
> time today. I'm running Firefox 59.0, on Windows 10. The color dot is not
> showing up.

That's probably because CSS variables are currently just interpreted as strings. I assume this issue is handled in bug 1413310.

Note: You should rather file new bug reports instead of commenting on ones that are marked as fixed.

Sebastian