Closed Bug 1413310 Opened 7 years ago Closed 7 months ago

Add info swatches in rule view for all CSS variables

Categories

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

Product:
DevTools
Component:
Inspector: Rules
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: hobindar, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1413310 - Part 1: Add swatches for CSS variables
59 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1413310 - Part 2: Add unit tests for CSS variable swatches
59 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1413310 - Part 3: Update tooltip for CSS variable swatches
59 bytes, text/x-review-board-request
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.78 Safari/537.36
Component: Untriaged → Developer Tools: CSS Rules Inspector
Assignee: nobody → darrenhobin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Comment on attachment 8923989 [details]
Bug 1413310 - Part 1: Add swatches for CSS variables

https://reviewboard.mozilla.org/r/195166/#review202012

I only reviewed the general style since the patch seems to be cutting off "var("

::: devtools/client/shared/output-parser.js:65
(Diff revision 1)
>    this.doc = document;
>    this.supportsType = supportsType;
>    this.isValidOnClient = isValidOnClient;
>    this.colorSwatches = new WeakMap();
>    this.angleSwatches = new WeakMap();
> +  this._computedVariable = null;

Add a comment about this variable. I would probably just call it this.computedVariable

::: devtools/client/shared/output-parser.js:393
(Diff revision 1)
>            } else if (angleOK(token.text)) {
>              this._appendAngle(token.text, options);
>            } else {
> +            if (options.expectVar) {
> +              let variableNode = this._createNode("span", {});
> +              if (text.substring(token.startOffset, token.endOffset).startsWith("--")) {

Add a new line before and after this if statement

::: devtools/client/shared/output-parser.js:1218
(Diff revision 1)
> +   * @param  {Node} node
> +   *         Node to append swatch to.
> +   */
> +  _appendVariable: function (variable, node) {
> +    let swatch;
> +    if (variable === null) {

Add a new line before and after this if statement
Attachment #8923989 - Flags: review?(gl)
Attachment #8923989 - Flags: review?(gl) → review?(ttromey)
Hey Tom,

Can you do an initial review of the output-parser code. This bug is about adding a swatch in front of var(..) to tell you the computed value of that var(..) function. Similar to the "=" swatch you see in Safari's handling of CSS variables https://twitter.com/malyw/status/910510393990303744.
Comment on attachment 8923989 [details]
Bug 1413310 - Part 1: Add swatches for CSS variables

https://reviewboard.mozilla.org/r/195166/#review204272

Thank you for the patch.  I think the basics of this patch are fine, so I marked it r+.  I found a few minor things to address.  I've only examined output-parser.js.

Also, earlier :gl said that the `var(` text was now cut off.  I looked at the interdiff but didn't see anything obvious that would have fixed this.  Did you track this down?

::: devtools/client/shared/output-parser.js:229
(Diff revision 2)
>      }
>  
>      // Get the variable name.
>      let varName = text.substring(tokens[0].startOffset, tokens[0].endOffset);
>  
> +    // Reset swatch variable

I think this comment should mention that this might be modified by `_doParse`.

::: devtools/client/shared/output-parser.js:393
(Diff revision 2)
> +              let variableNode = this._createNode("span", {});
> +
> +              if (text.substring(token.startOffset, token.endOffset).startsWith("--")) {

This always creates a new node, but then discards it on one branch.  I think it would be better to only create it when it will actually be used.

Also, there's a more spec-compliant test for whether a string is a css variable in devtools/shared/fronts/css-properties.js.  I'm not 100% sure it matters in this context -- what happens if you use CSS with `var(mumble)` where `mumble` starts with "--" but isn't actually valid?  E.g., `--23`?

::: devtools/client/shared/output-parser.js:1235
(Diff revision 2)
> +        title: variable
> +      });
> +    }
> +
> +    node.appendChild(swatch);
> +    return node;

Nothing uses this value, and it isn't documented; so I suggest just dropping the `return`.
Attachment #8923989 - Flags: review?(ttromey) → review+
Comment on attachment 8923989 [details]
Bug 1413310 - Part 1: Add swatches for CSS variables

https://reviewboard.mozilla.org/r/195166/#review204272

Hi Tom. After talking with :gl about the missing var(, it appears that wasn't actually an issue with this patch, but rather a bad application of the patch on top of other (not yet landed) work. Likely there was a conflict that wasn't resolved correctly when applying the patch. At the very least, the issue isn't reproducible.

I've addressed all of your other comments in a new revision.
Comment on attachment 8930783 [details]
Bug 1413310 - Part 2: Add unit tests for CSS variable swatches

https://reviewboard.mozilla.org/r/201864/#review208640

Thank you for the patch.  This looks good to me.
Attachment #8930783 - Flags: review?(ttromey) → review+
Depends on: 1102464
Attachment #8937291 - Flags: review?(ttromey) → review?(gl)
Comment on attachment 8937291 [details]
Bug 1413310 - Part 3: Update tooltip for CSS variable swatches

https://reviewboard.mozilla.org/r/207992/#review215902

::: devtools/client/shared/output-parser.js:1266
(Diff revision 2)
>    _appendVariable: function (variable, node) {
> -    let swatch;
> +    let opts = {};
>  
>      if (variable === null) {
> -      swatch = this._createNode("span", {
> -        class: "ruleview-swatch ruleview-variableswatch-unmatched",
> +      opts.class = "ruleview-swatch ruleview-variableswatch-unmatched";
> +      opts["data-variable"] = "Cannot be resolved";

Unrelated, but is this "Cannot be resolved" string shown in the UI somewhere ? If so, it should probably be localized.
Product: Firefox → DevTools
Attachment #8937291 - Flags: review?(gl)

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.

Assignee: darrenhobin → nobody
Status: ASSIGNED → NEW
Type: defect → enhancement

I wonder what value this feature has beyond what was added in bug 1145527. As far as I can see, it only adds a swatch for each var() value that shows the value of the variable.
Currently, that information is already exposed via a tooltip shown when hovering the var() value.

I believe adding a swatch for each usage isn't necessary except it provides additional features besides the tooltip. So I vote to close this.

Sebastian

Flags: needinfo?(jdescottes)

(In reply to Sebastian Zartner [:sebo] from comment #17)

I wonder what value this feature has beyond what was added in bug 1145527. As far as I can see, it only adds a swatch for each var() value that shows the value of the variable.
Currently, that information is already exposed via a tooltip shown when hovering the var() value.

I believe adding a swatch for each usage isn't necessary except it provides additional features besides the tooltip. So I vote to close this.

Sebastian

You can see an example of what this feature is trying to accomplish in https://bugzilla.mozilla.org/show_bug.cgi?id=1413310#c4. Bug 1145527 adds a hover over each variable, but it doesn't necessarily compute the value of the var() function. I think the bug is still relevant, but probably just considered as a feature request/enhancement.

Flags: needinfo?(jdescottes)
Severity: normal → S3

Expanding the current tooltip (see Bug 1423701) may be better

Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: