Show a color swatch next to CSS variable definitions that have color values
Categories
(DevTools :: Inspector: Rules, enhancement, P3)
Tracking
(firefox71 fixed)
| Tracking | Status | |
|---|---|---|
| firefox71 | --- | fixed |
People
(Reporter: pbro, Assigned: daisuke, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed, Whiteboard: [dt-q])
Attachments
(2 files)
In a lot of cases, CSS variables are colors. Unfortunately our tool does not show color swatches next to them yet.
Here's the use case:
.some-rule {
--foo: blue;
}
We should show a blue color swatch next to the value blue here.
We already have a function in DevTools to test if a given string is a valid CSS color, and we use it already in many places. So we could do this here.
We used to do this for all property values I believe, which was wrong in cases like:
font-family: Black;
Black can be a valid font name, but of course is not a color in this case.
The point here is that CSS variable are very often colors. And I think we should show color swatches next to them if the string is a valid color. Just because it probably will be used as such.
Of course, we can't know if the variable will be used in a property that does accept colors, but I think it still will be useful.
|
||
Updated•6 years ago
|
|
||
Comment 1•6 years ago
|
||
Also see bug 1222774 and/or bug 1413310
|
|
||
Updated•6 years ago
|
|
||
Updated•6 years ago
|
|
Reporter | |
Comment 2•6 years ago
|
||
To fix this bug, I believe the file that needs changing is the output-parser: /devtools/client/shared/output-parser.js. This file contains the code we use to parse CSS properties that appear in the Rules panel. When it comes across a color, it turns it into some DOM node structure that displays the color swatch, etc. But it only does so in certain cases where we are sure that some seemingly valid color expression actually is a color. This is done with the boolean options.supportsColor which is, for now, only set to true when we know the property accepts colors or gradients. The approach I have in mind is to set this boolean to true also when the property is a custom property (i.e. it starts with --). Gabriel: are you still going to fix this bug? (it is assigned to you). If not, please unassign yourself. I think this might be an easy enough bug for someone new to take a look at. Doing the fix might actually be really easy. However coming up with a good integration test for this will require a bit more work, but I will set myself as a mentor on this bug, so that anyone interested in fixing it can ask any question and I'll help them.
|
|
||
Updated•6 years ago
|
|
||
Comment 3•6 years ago
|
||
Hey folks, I'm happy to have a go at this - feel free to assign to me.
|
|
||
Updated•6 years ago
|
|
|
||
Comment 4•6 years ago
|
||
Okay,
I've had a look through the code in output-parser.js, and I've got the test harness running here, which I *think* is exercising the outputparser code:
./mach mochitest devtools/client/shared/test/browser_outputparser.js
I think the approach I'd take here would be to work out what the code is that generates the color swatch, then test for the appearance of this swatch when rendering the minimal html page with a css variable declared, and an element with a corresponding rule using it.
But I think I'll need a few pointers about where the code corresponding to the circular swatch exists, and how I'd test for the existence of it.
### Making the page
It looks like Firefox can load data urls like in `browser_keybindings_01.js`, so I'm guessing we might be able to do that to set up page:
```
const TEST_URL = "data:text/html,<html><head><title>Css Var test</title></head><body>" +
"<style> (some code here declaring a css var, then using it in rule used below</style>" +
"<h1 class='colored-red-with-a-css-variable'></h1></body></html>";
```
Then I'm assuming we could hit load this like we do there too:
```
const {gDevToolsBrowser} = require("devtools/client/framework/devtools-browser");
add_task(async function() {
await addTab(TEST_URL);
await new Promise(done => waitForFocus(done));
// highlight/select the corresponding DOM element with a class using the css variable
```
Once we have this, if there's a way to inspect whatever the inspector is made from (is it another DOM?), I reckon I can use typical dom querying methods to see if the correct swatch element is there.
But I'm not sure how I'd do this part. Are there any examples I can refer to, to see how this is done/
let onToolboxReady = gDevTools.once("toolbox-ready");
let toolbox = await onToolboxReady;
```|
|
||
Comment 5•6 years ago
|
||
BTW - I'm using markdown out of habit, but if there are ways to make this issue more readable with some other text dialect (i.e. textile, something specific to bugzilla I haven't used before etc., I'm happy to use it).
|
|
||
Comment 6•6 years ago
|
||
(In reply to Chris Adams from comment #4) > I've had a look through the code in output-parser.js, and I've got the test > harness running here, which I *think* is exercising the outputparser code: > > ./mach mochitest devtools/client/shared/test/browser_outputparser.js Yep. > I think the approach I'd take here would be to work out what the code is > that generates the color swatch, then test for the appearance of this swatch > when rendering the minimal html page with a css variable declared, and an > element with a corresponding rule using it. You can probably add a test to browser_outputparser.js. It may require a bit of modification to work, offhand I'm not sure. > But I think I'll need a few pointers about where the code corresponding to > the circular swatch exists, and how I'd test for the existence of it. See makeColorTest in that file. It's probably simpler to add things here as needed than to try to write a new test from scratch.
|
|
Reporter | |
Comment 7•5 years ago
|
||
Unassigning this bug since it has received no activity over the past 8 months.
Feel free to pick it up again if you did intend to work on it still.
Just trying to clean our backlog of bugs so they are available to people.
hi, i am interested in solving it. will u please assign it to me??
|
|
Reporter | |
Comment 9•5 years ago
|
||
Hi, thanks for your interest. Yes I'll assign it to you now.
Make sure to go through our contribution documentation first: https://docs.firefox-dev.tools to set up your dev environment and know how to make and test changes in the code.
Once done, please take a look at comment 2, I believe that even if it's 8 months old, it still applies, and should give you a good idea on how to get started.
You can then also take a look at comment 4 where Chris shared some details regarding how to write a test for this.
You might also be interested in our documentation about tests: https://docs.firefox-dev.tools/tests/writing-tests.html
|
||
Comment 10•5 years ago
|
||
This is done with the boolean options.supportsColor which is, for now, only
set to true when we know the property accepts colors or gradients.
The approach I have in mind is to set this boolean to true also when the
property is a custom property (i.e. it starts with --).
hi, trying this approach i found out that there hasn't been an custom property hasn't been defined in the
devtools/shared/css/constants.js file. so do i manually change the contents of this file by adding a custom property, or am i missing something. please help.
i am sorry if the question isn't clear or sounds childish, i am really new to firefox development and am trying to learn
|
|
Reporter | |
Comment 11•5 years ago
•
|
||
(In reply to asish7295 from comment #10)
hi, trying this approach i found out that there hasn't been an custom property hasn't been defined in the
devtools/shared/css/constants.js file. so do i manually change the contents of this file by adding a custom property, or am i missing something. please help.
I don't think it's necessary to add anything in the constants.js file for this. CSS custom properties always start with --, so it might be enough to simply check if the name variable starts with these -- character. And if so, set the options.supportsColor to true.
i am sorry if the question isn't clear or sounds childish, i am really new to firefox development and am trying to learn
Oh, don't worry at all about asking questions, no matter if they may sound childish, stupid, whatever, or even if they have been asked before. There is no stupid questions. A question means that something isn't clear, and it's always always valid to clarify things for people, so people can go ahead and do the change they intended to do.
|
|
||
Comment 12•5 years ago
|
||
while trying to write the tests for this , i found that there was a function in the devtools/client/shared/test/browser_outputparser.js file which has a function testParseCssProperty which has a tests array where it makes use of makeColorTest function . so maybe we can do something like :
makeColorTest("--someproperty" , "blue" , {name : "blue"}) and add it to the tests array
is this the right approach being taken or am i missing something
|
|
Reporter | |
Comment 13•5 years ago
|
||
Yes I think this is the right approach!
|
|
Reporter | |
Comment 14•5 years ago
|
||
Hi Asish. Do you need more time to work on this bug? If so, that's totally ok.
If you, however, don't plan on working on it anymore, please let me know so I can make it available for others to pick up.
|
|
Reporter | |
Comment 15•5 years ago
|
||
Making this bug unassigned again.
|
|
Reporter | |
Updated•5 years ago
|
|
Assignee | |
Comment 16•5 years ago
|
||
|
|
Assignee | |
Comment 17•5 years ago
|
||
Depends on D44943
|
||
Comment 18•5 years ago
|
||
Pushed by dakatsuka.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/25dba9986075 Support color swatch for css variable which can be color. r=gl https://hg.mozilla.org/integration/autoland/rev/deeb30ce64e9 Add test for CSS variable which can be color. r=pbro
|
||
Comment 19•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/25dba9986075
https://hg.mozilla.org/mozilla-central/rev/deeb30ce64e9
|
||
Updated•5 years ago
|
|
||
Updated•5 years ago
|
Description
•