1222774 - Rule view swatches should appear next to CSS variable values

Status: NEW

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

Comment 1

[Tom Tromey :tromey]

The main issue with this is that there's no way to tell which variables refer
to colors.  Variables are defined to be just an arbitrary sequence of tokens.

In some cases one can tell ("--var: #fff") but in some one cannot -- one
example being if you have a font with a color-like name ("--var: Black").

Comment 2

[Tom Tromey :tromey]

This keeps coming up in discussion so I thought I'd enumerate the cases.
In each case I think the two important questions are (1) where should the
color swatch be placed, and (2) what text-editing should be done when
using the tooltip to modify the value.

One possible answer is to use some heuristic and only handle some subset of
cases.


The most basic case is a variable used in a longhand property:

--color: red;
background-color: var(--color);


Replacing that with a shorthand property causes some issues:

--color: red;
background: var(--color);

Here we'd need to do more complicated property parsing in the rule-view
to get a sensible result.  This is more obvious if we make the variable
serve two purposes:

--var: red 7% 7%;
background: var(--var);


Another possibility is that the variable is used only as a component of the color.

--alpha: 0.7
color: rgb(50, 50, 50, var(--alpha));


It's also possible do supply several tokens in the variable definition:

--maybe-alpha: , 0.7;  /* some other definition could be empty */
color: rgb(50, 50, 50 var(--maybe-alpha));


Variable defaults also raise the question of whether there should be a separate
swatch for the default:

color: var(--something, seagreen);

Comment 3

[Patrick Brosset <:pbro>]

Triaging (filter on CLIMBING SHOES).

Comment 4

[Sebastian Zartner [:sebo]]

This is also true for other values having swatches assigned like the animation timing functions, angles or filters. So, while colors are surely the most common case, I believe this should also be made available for the other values stored inside CSS variables.
Will that be covered by this bug or should I create another one for them or even one for each type of value?

Sebastian

Comment 5

[Tim Nguyen :ntim]

(In reply to Sebastian Zartner [:sebo] from comment #4)
> This is also true for other values having swatches assigned like the
> animation timing functions, angles or filters. So, while colors are surely
> the most common case, I believe this should also be made available for the
> other values stored inside CSS variables.
> Will that be covered by this bug or should I create another one for them or
> even one for each type of value?
> 
> Sebastian

I believe they are the same underlying issue.

Comment 6

[Tom Tromey :tromey]

> One possible answer is to use some heuristic and only handle some subset of cases.

These days I think I was overthinking this in the past and it would be useful to users
to just display a swatch in any variable definition where the value can be parsed
as a color.

Comment 7

[Darren Hobin [:hobindar]]

Attachment: Bug 1222774 - Part 1: Add swatches for CSS variables

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

Comment 8

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

Comment on attachment 8919931 [details]
Bug 1222774 - Part 1: Add swatches for CSS variables

https://reviewboard.mozilla.org/r/190876/#review196984

This is looking pretty good. I think the next step for you is to apply the patches in https://bugzilla.mozilla.org/show_bug.cgi?id=1102464 and use the same mechanism to display the TextTooltip.

::: devtools/client/shared/output-parser.js:1209
(Diff revision 1)
>      } else {
>        this._appendTextNode(color);
>      }
>    },
>  
> +  _appendVariable: function (variable, node) {

This function needs some JSDocs.

::: devtools/client/shared/output-parser.js:1211
(Diff revision 1)
>      }
>    },
>  
> +  _appendVariable: function (variable, node) {
> +    let swatch;
> +    if (variable === null) {

if statement blocks should be formatted with new lines before and after the if statement blocks

::: devtools/client/shared/output-parser.js:1211
(Diff revision 1)
>      }
>    },
>  
> +  _appendVariable: function (variable, node) {
> +    let swatch;
> +    if (variable === null) {

We can probably switch the checks around to avoid checking for null:

if (variable) {

} else {

}

Comment 9

[Tim Nguyen :ntim]

I don't think the patch is doing what the bug was originally about (although I apologize for the lack of description in the bug).

This bug is about showing color/filter/timing function swatches next to variables that are relevant.

Comment 10

[Darren Hobin [:hobindar]]

Per discussion with Gabriel [:gl], I've moved my patch into bug 1413310.