It used to return true only if colorToRGBA wasn't returning null.
We changed it to be an alias of isValidCSSColor but this had unintended effect
We need to consider a color valid if we can get the rgba tuples from it, as we
need them to run the different methods/operation in this module.
Details
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Lint Not Applicable - Unit
Tests Not Applicable - Build Status
Buildable 508458 Build 604839: arc lint + arc unit
Event Timeline
devtools/shared/css/color.js | ||
---|---|---|
15 | How did you come up with this precise list? I can see many more special cases in this rust component: Unfortunately, I couldn't find any method which would return "accentcolortext" in a list of special predefined colors. I see that you are trying to align to what chrome does. Having said that, given that maintaining the list of all these special colors sounds complicated/impossible, we should probably handle this differently and handle an exception somewhere more nicely? | |
15 | Did you mean to add accentcolor as a special value? |
devtools/shared/css/color.js | ||
---|---|---|
15 |
I looked it up on the spec: https://w3c.github.io/csswg-drafts/css-color/#css-system-colors
Oh right, InspectorUtils.isValidCSSColor("-moz-menubartext") does return true
Yeah. So I think before, color.js had a isValidCSSColor method that was calling InspectorUtils.colorToRGBA and checking that the result was not null, because it does restrict the colors to the ones we can actually properly handle.
Yes it would be nice. We'd need to have something on the platform so we can pass a window, as the rendered color might be impacted by light/dark theme.
|
devtools/shared/css/color.js | ||
---|---|---|
15 | oops, not sure how I did that, I'll revert that |
This looks good to me, but note that there is an "half" regression :p
For data:text/html,<meta charset=utf8><style>body {color: graytext;}</style>Hello
On nightly I get:
With your patch I get:
But on nightly the color picker is broken... so this is an half regression.
We could also tell you hide this broken feature. But it would be nice to make the color picker work.
devtools/shared/css/color.js | ||
---|---|---|
145–149 | Note that currentColor and transparent will return a non-null object. | |
385–386 | It doesn't seem to break anything at first sight.... but I would have expected to return the valid CSS colors here, like accentcolor, -moz-*, ... |
yes, so I basically just rolled back to previous behaviour.
In Firefox 110, you won't see the swatch for graytext either.
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1816904 to handle system colors
devtools/shared/css/color.js | ||
---|---|---|
385–386 | no idea :/ |