1423701 - Show CSS variable (custom property) "tree" in the css variable tooltip

Status: NEW

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

Description

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

Given markup like:

<style>
:root {
  --foo: red;
}

div {
  --foo: blue;
}

span {
  color: foo;
}
</style>
<div><span>foo</span></div>

It'd be great to see:

(1) what is the current assignment of foo when inspecting the span
(2) how did it get to be that way

Just brainstorming how this info could be displayed:
1) could be displayed in the rule view somewhere next to the value, or in a tooltip upon hovering the value
2) could be displayed in the computed view where we already have this sort of 'trace' feature.  We would probably want to show all of the variables in scope in the computed view if we did this, and maybe link to the particular one from the rule view.

Getting (2) to work in the computed view should hopefully hook into the normal machinery, since running this in the browser console returns what I'd expect it to:

let require = Cu.import('resource://gre/modules/devtools/Loader.jsm', {}).devtools.require;
let {CssLogic} = require("devtools/styleinspector/css-logic");
let cl = new CssLogic();
cl.highlight(document.documentElement);
cl.getPropertyInfo("--space-above-tabbar");

Comment 1

[Lawson C]

Attachment: 1423701.patch

Comment 2

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

Comment on attachment 8944198 [details] [diff] [review]
1423701.patch

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

::: devtools/client/inspector/rules/rules.js
@@ +344,4 @@
>      } else if ((classes.contains("ruleview-variable") ||
>                  classes.contains("ruleview-unmatched-variable")) && prop) {
>        type = VIEW_NODE_VARIABLE_TYPE;
> +     // console.log("hello");

Remove console.log()

::: devtools/client/inspector/shared/highlighters-overlay.js
@@ +371,4 @@
>      try {
>        // Save grid highlighter state.
>        let { url } = this.inspector.target;
> +

This can be removed.

@@ +375,2 @@
>        let selector = await node.getUniqueSelector();
> +      this.state.grid = { selector, options, url};

This can be removed.

::: devtools/client/inspector/shared/tooltips-overlay.js
@@ +236,4 @@
>      }
>  
>      if (type === TOOLTIP_VARIABLE_TYPE && nodeInfo.value.value.startsWith("--")) {
> +      let variable = [];

variables = []

@@ +236,5 @@
>      }
>  
>      if (type === TOOLTIP_VARIABLE_TYPE && nodeInfo.value.value.startsWith("--")) {
> +      let variable = [];
> +      if (this.view._elementStyle.getVariable(nodeInfo.value.variable) != null) {

let variable = this.view._elementStyle.getVariable(nodeInfo.value.variable);

variable != null
!variable

@@ +241,5 @@
> +        let currentNode = this.view._elementStyle.getVariable(nodeInfo.value.variable);
> +        variable.push(nodeInfo.value.variable + " = " + currentNode);
> +
> +        while (currentNode.startsWith("--")) {
> +          let prevnode = currentNode;

currentNode = this.view._elementStyle.getVariable(currentNode) || "";

@@ +244,5 @@
> +        while (currentNode.startsWith("--")) {
> +          let prevnode = currentNode;
> +          if (this.view._elementStyle.getVariable(currentNode) != null) {
> +            currentNode = this.view._elementStyle.getVariable(currentNode);
> +            variable.push(prevnode + " = " + currentNode);

STYLE_INSPECTOR_L10N.getFormatStr("rule.variableValue", prevNode, currentValue)

s/prevNode/previousVariable
s/currentNode/currentVariable

@@ +247,5 @@
> +            currentNode = this.view._elementStyle.getVariable(currentNode);
> +            variable.push(prevnode + " = " + currentNode);
> +          } else {
> +            currentNode = "";
> +            variable.push(prevnode + " is not set");

STYLE_INSPECTOR_L10N.getFormatStr("rule.variableUnset", prevNode);

::: devtools/client/shared/output-parser.js
@@ +231,5 @@
>        // The variable value is valid, set the variable name's title of the first argument
>        // in var() to display the variable name and value.
> +      console.log("!!!!!");
> +      console.log(varName);
> +      firstOpts["data-variable"] = varName;

1. firstOpts["data-variable"] = varName can be set before the if statement after we assign varName. We only need the variable name now to get all the variable values.
2. firstOpts["data-variable"] = STYLE_INSPECTOR_L10N.getFormatStr("rule.variableUnset", varName); can be removed because we only need the variable name to get variable values and we can also set the "is not set" string in the tooltips-overlay.js

You can see that firstOpts.class will set a class to indicate that the first variable matches to a variable value, and therefore all the other variables don't match so we set secondOpts.class to an unmatched variable class. We should still preserve these matched and unmatched class assignments in this if statement.

Also, check attributes at https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js
@@ +21,3 @@
>    // Create tooltip content
>    let div = doc.createElementNS(XHTML_NS, "div");
>    div.classList.add("devtools-monospace", "devtools-tooltip-css-variable");

Look for devtools-tooltip-css-variable it is in tooltips.css to add css to your tooltip container

@@ +21,5 @@
>    // Create tooltip content
>    let div = doc.createElementNS(XHTML_NS, "div");
>    div.classList.add("devtools-monospace", "devtools-tooltip-css-variable");
> +  let textElement = "";
> +  for (textElement in text) {

for (let textElement of text) {

}

@@ +22,5 @@
>    let div = doc.createElementNS(XHTML_NS, "div");
>    div.classList.add("devtools-monospace", "devtools-tooltip-css-variable");
> +  let textElement = "";
> +  for (textElement in text) {
> +    let span = doc.createElementNS(XHTML_NS, "span");

You can use a div instead of span here to avoid the br

Comment 3

[Lawson C]

Attachment: 1401847.patch

Comment 4

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

Comment on attachment 8946722 [details] [diff] [review]
1401847.patch

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

::: devtools/client/inspector/shared/tooltips-overlay.js
@@ +240,4 @@
>      }
>  
>      if (type === TOOLTIP_VARIABLE_TYPE && nodeInfo.value.value.startsWith("--")) {
> +      let variable = this.view._elementStyle.getVariable(nodeInfo.value.variable);

We can actually declare this as currentVariable. Also we can also add the fallback as you have down below by appending the following to the end:
this.view._elementStyle.getVariable(nodeInfo.value.variable) || "";

That way we don't have to check for null in the if statement and simplify that to:
if (variable) {}

@@ +241,5 @@
>  
>      if (type === TOOLTIP_VARIABLE_TYPE && nodeInfo.value.value.startsWith("--")) {
> +      let variable = this.view._elementStyle.getVariable(nodeInfo.value.variable);
> +      let variables = [];
> +      if (variable != null) {

Add a new line before the if statement

@@ +245,5 @@
> +      if (variable != null) {
> +        let currentVariable = variable;
> +        variables.push(STYLE_INSPECTOR_L10N.getFormatStr("rule.variableValue",
> +            nodeInfo.value.variable, currentVariable));
> +        while (currentVariable.startsWith("--")) {

Add a new line before this while block to separate out the logical blocks

@@ +248,5 @@
> +            nodeInfo.value.variable, currentVariable));
> +        while (currentVariable.startsWith("--")) {
> +          let previousVariable = currentVariable;
> +          currentVariable = this.view._elementStyle.getVariable(currentVariable) || "";
> +          if (currentVariable != "") {

This can be simplified to be 

if (currentVariable) {}

@@ +259,5 @@
> +        }
> +      } else {
> +        variables.push(STYLE_INSPECTOR_L10N.getFormatStr("rule.variableUnset",
> +          nodeInfo.value.variable));
> +      }

Add a new line after this if statement closure

::: devtools/client/shared/output-parser.js
@@ +22,4 @@
>  loader.lazyRequireGetter(this, "CSS_TYPES",
>    "devtools/shared/css/properties-db", true);
>  
> +// const STYLE_INSPECTOR_PROPERTIES = "devtools/shared/locales/styleinspector.properties";

We can remove all of this.

@@ +280,4 @@
>     *         A document fragment.
>     */
>    _doParse: function (text, options, tokenStream, stopAtCloseParen) {
> +    

Remove this line

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js
@@ +20,5 @@
>  function setVariableTooltip(tooltip, doc, text) {
>    // Create tooltip content
>    let div = doc.createElementNS(XHTML_NS, "div");
> +  div.classList.add("devtools-monospace", "devtools-tooltip-css-variable",
> +    "variable-node-parent");

We can remove variable-node-parent and just add the styles change to devtools-tooltip-css-variable class.

::: devtools/client/themes/common.css
@@ +39,4 @@
>  .devtools-monospace {
>    font-family: var(--monospace-font-family);
>  }
> +.variable-node-parent{

This should go in tooltips.css. We can simplify this by add the styles here to devtools-tooltip-css-variable

See https://searchfox.org/mozilla-central/source/devtools/client/themes/tooltips.css#66

Comment 5

[Lawson C]

Attachment: 1401847.patch

Comment 6

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

Comment on attachment 8948740 [details] [diff] [review]
1401847.patch

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

So, I found a bug which might be my fault. 

For the given example,
--red: --blue;
--blue: yellow;
color: var(--red);
This actually isn't valid.

We would need --red: var(--blue) in order to get the color yellow. So, we actually need to check that the variable starts with "var(--" I think, but we need to actually computed what var() is.

::: devtools/client/inspector/shared/tooltips-overlay.js
@@ +201,4 @@
>     */
>    _onPreviewTooltipTargetHover: Task.async(function* (target) {
>      let nodeInfo = this.view.getNodeInfo(target);
> +

Remove this new line

@@ +249,5 @@
> +
> +        while (currentVariable.startsWith("--")) {
> +          let previousVariable = currentVariable;
> +          currentVariable = this.view._elementStyle.getVariable(currentVariable) || "";
> +          if (currentVariable != "") {

This can be if (currentVariable)

@@ +267,2 @@
>        return true;
>      }

Don't remove this new line.

::: devtools/client/shared/output-parser.js
@@ +226,1 @@
>      if (typeof varValue === "string") {

Add a new line before this if statement

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js
@@ +21,4 @@
>    // Create tooltip content
>    let div = doc.createElementNS(XHTML_NS, "div");
>    div.classList.add("devtools-monospace", "devtools-tooltip-css-variable");
> +  for (let textElement of text) {

Add a new line before and after this for loop block.

Comment 7

[Lawson C]

Attachment: 1401847.patch

switched over to the new design. 
It now only shows tracing if the variable is referenced with "var(--value)" and doesn't show any trace when the variable is referenced with just "--value" (ie. the old design)

Comment 8

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

Comment on attachment 8951111 [details] [diff] [review]
1401847.patch

Clearing the review for this because of the issue we discussed over Slack.

Comment 9

[Lawson C]

Attachment: 1401847.patch

Many things are commented out, this is a very incomplete patch. Just look over my lexer, how it is being imported and called, etc. 

To test the lexer, throw in some css variables into the inspector. Here is a sample: 
--yellow: green;
--blue: var(--yellow);
--red: var(--blue, var(--yellow));
background-color: var(--red, var(--yellow, pink));

The lexer will console log an array of the variable names

Comment 10

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

Comment on attachment 8956552 [details] [diff] [review]
1401847.patch

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

This doesn't use getCSSLexer on the input string at all. I can't say this is the right approach since you are manually parsing the strings.

Comment 11

[Lawson C]

Attachment: 1401847.patch

My second lexer attempt. 

To test the lexer, throw in some css variables into the inspector. Here is a sample: 
--yellow: green;
--blue: var(--yellow);
--red: var(--blue, var(--yellow));
background-color: var(--red, var(--yellow, pink));

The lexer will console log an array of the variable names

Comment 12

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

Comment on attachment 8956661 [details] [diff] [review]
1401847.patch

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

I am not seeing much inside of parsing-utils.js. You should be pushing the entire var(..) string into the lexer.

Comment 13

[Lawson C]

Attachment: 1401847.patch

First fully implemented iteration of the variable tracing. 

In theory, this should cover all variable tracing cases.

Comment 14

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

Comment on attachment 8957766 [details] [diff] [review]
1401847.patch

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

Clearing review based on the comments we spoke of in Slack.

Comment 15

[Lawson C]

Attachment: 1401847.patch

I wrote a test file and did a significant refactor of my lexer

Comment 16

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

Comment on attachment 8959648 [details] [diff] [review]
1401847.patch

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

::: devtools/client/inspector/shared/tooltips-overlay.js.rej
@@ +1,1 @@
> +--- tooltips-overlay.js

Remove this file.

::: devtools/client/shared/test/unit/test_css-token-parsing.js
@@ +18,5 @@
> +  { input: "2.6", output: ["2.6"] },
> +  { input: "25%", output: ["25%"] },
> +  { input: "30px", output: ["30px"] },
> +  { input: "2em", output: ["2em"] },
> +  { input: "url(www.sample.com)", output: ["www.sample.com"] },

I would expect all of these base cases to be false from the top to this point.

@@ +23,5 @@
> +  { input: "var(--properVar)", output: ["--properVar"] },
> +
> +  // Multiple Token Cases
> +  { input: "var(--firstVar, var(--secondVar))",
> +    output: ["--firstVar", "--secondVar"] },

I expect this to be ["--firstVar", "var(--secondVar)"]

@@ +27,5 @@
> +    output: ["--firstVar", "--secondVar"] },
> +  { input: "var(--firstVar, 20%)",
> +    output: ["--firstVar", "20%"] },
> +  { input: "var(--firstVar, var(--secondVar, var(--thirdVar)))",
> +    output: ["--firstVar", "--secondVar", "--thirdVar"] },

["--firstVar", "var(--secondVar, var(--thirdVar))"]

Same for the rest of the multiple token cases.

@@ +43,5 @@
> +    output: ["--firstVar", "--secondVar", "2em"] },
> +  { input: "var(--firstVar, var(--secondVar, url(\"www.sample.com\")))",
> +    output: ["--firstVar", "--secondVar", "www.sample.com"] },
> +
> +  // Calculation Cases

I would expect false for all of these calculation cases.

Comment 17

[Lawson C]

Attachment: 1401847.patch

Hopefully closer to the final design

Comment 19

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.