Closed Bug 1456167 Opened 7 years ago Closed 6 years ago

Show a color swatch next to CSS variable definitions that have color values

Categories

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

Product:
DevTools
Component:
Inspector: Rules
Type:
enhancement

Tracking

(firefox71 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 71
Tracking Flags:
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)

Bug 1456167: Support color swatch for css variable which can be color. r?pbro!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1456167: Add test for CSS variable which can be color. r?pbro!
47 bytes, text/x-phabricator-request
Details | Review
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.
Assignee: nobody → gl
Status: NEW → ASSIGNED
Also see bug 1222774 and/or bug 1413310
Priority: -- → P3
Product: Firefox → DevTools
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.
Mentor: pbrosset
Flags: needinfo?(gl)
Assignee: gl → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(gl)
Hey folks, I'm happy to have a go at this - feel free to assign to me.
Assignee: nobody → chris
Status: NEW → ASSIGNED
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; ```
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).
(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.

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.

Assignee: chris → nobody
Status: ASSIGNED → NEW

hi, i am interested in solving it. will u please assign it to me??

Flags: needinfo?(pbrosset)

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

Assignee: nobody → asishkrramuk
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)

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

(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.

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

Yes I think this is the right approach!

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.

Flags: needinfo?(asishkrramuk)

Making this bug unassigned again.

Assignee: asishkrramuk → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(asishkrramuk)
Whiteboard: [dt-q]
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

https://hg.mozilla.org/mozilla-central/rev/25dba9986075
https://hg.mozilla.org/mozilla-central/rev/deeb30ce64e9

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
Assignee: nobody → daisuke
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: