Page MenuHomePhabricator

Bug 1816342 - [devtools] Fix `valid` getter in color.js. r=ochameau.
ClosedPublic

Authored by nchevobbe on Feb 13 2023, 7:54 AM.
Referenced Files
Unknown Object (File)
Thu, May 1, 7:04 AM
Unknown Object (File)
Mon, Apr 21, 12:03 PM
Unknown Object (File)
Thu, Apr 17, 9:54 AM
Unknown Object (File)
Wed, Apr 16, 10:53 PM
Unknown Object (File)
Apr 9 2025, 10:26 PM
Unknown Object (File)
Mar 1 2025, 7:05 AM
Unknown Object (File)
Feb 16 2025, 7:31 AM
Unknown Object (File)
Feb 1 2025, 7:35 AM
Subscribers
None

Details

Summary

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.

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

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
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:
https://searchfox.org/mozilla-central/source/servo/components/style/values/specified/color.rs#580
(color: -moz-menubartext; is also problematic, so I suspect there is many more case being an issue :/)

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.
I'm wondering if that's the right call... Why would we translate predefined color and not these ones?
But I could understand that these special colors are slightly more like css color variables, which you don't want to change and have the whole UI unified around the original value.

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?
Also I see that with data:text/html,<meta charset=utf8><style>body {color: AccentColor;}</style>Hello we do not display the color in a circle... it would be nice if we could show it.
For some reason it works with data:text/html,<meta charset=utf8><style>body {color: graytext;}</style>Hello... (but clicking on it would also throw can't access property "a", tuple is null)

15

Did you mean to add accentcolor as a special value?

nchevobbe added inline comments.
devtools/shared/css/color.js
15

How did you come up with this precise list?

I looked it up on the spec: https://w3c.github.io/csswg-drafts/css-color/#css-system-colors

I can see many more special cases in this rust component:
https://searchfox.org/mozilla-central/source/servo/components/style/values/specified/color.rs#580
(color: -moz-menubartext; is also problematic, so I suspect there is many more case being an issue :/)

Oh right, InspectorUtils.isValidCSSColor("-moz-menubartext") does return true

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.
I'm wondering if that's the right call... Why would we translate predefined color and not these ones?
But I could understand that these special colors are slightly more like css color variables, which you don't want to change and have the whole UI unified around the original value.

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?

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.
I'll probably roll back to a similar version of what we had, only renaming it so it would be like colorHasRgbaTuples

Also I see that with data:text/html,<meta charset=utf8><style>body {color: AccentColor;}</style>Hello we do not display the color in a circle... it would be nice if we could show it.

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.

For some reason it works with data:text/html,<meta charset=utf8><style>body {color: graytext;}</style>Hello... (but clicking on it would also throw can't access property "a", tuple is null)

nchevobbe updated this revision to Diff 680321.
nchevobbe retitled this revision from Bug 1816342 - [devtools] Handle system color values in color.js. r=ochameau. to Bug 1816342 - [devtools] Fix `valid` getter in color.js. r=ochameau..
nchevobbe edited the summary of this revision. (Show Details)
nchevobbe marked 2 inline comments as done.

roll back to previous version of valid getter

devtools/shared/css/color.js
15

oops, not sure how I did that, I'll revert that

ochameau added a project: testing-approved.

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:

image.png (137×413 px, 9 KB)

With your patch I get:

image.png (140×369 px, 9 KB)

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.
I imagine that's fine as we check for this.specialValue first in _getInvalidOrSpecialValue...

385–386

thought: 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-*, ...
Why would we return "transparent" and not "accentcolor"?

This revision is now accepted and ready to land.Feb 15 2023, 10:55 AM

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:

image.png (137×413 px, 9 KB)

With your patch I get:

image.png (140×369 px, 9 KB)

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.

yes, so I basically just rolled back to previous behaviour.
In Firefox 110, you won't see the swatch for graytext either.

devtools/shared/css/color.js
385–386

no idea :/