System.Drawing.Color is possibly missing 8 colors

[HumanEquivalentUnit]

Background and Motivation

It appears that the System.Drawing.Color names are intended to track the CSS color names; if that is correct, there are some colors which need to be added. Two reasons for thinking the names are supposed to track CSS colors are:

• The Documentation for System.Drawing.Color says "For more information about these colors, see Colors by Name." which links to Mozilla's documentation on CSS color names.

• Pull Request Update the hex value for DarkSeaGreen color corefx#41561 where one of the color values differed from the CSS value, and the .Net color value was changed to agree with CSS.

Proposed API

Affecting at least KnownColor.cs and KnownColorNames.cs and KnownColorTable.cs and Affecting at least Color.cs, the significant differences are the addition of the new RebeccaPurple color, and the variant spellings of grAy colors as grEy:

namespace System.Drawing
{
    enum KnownColor
    {
+    RebeccaPurple,
+    DarkGrey,
+    DarkSlateGrey,
+    DimGrey,
+    Grey,
+    LightGrey,
+    LightSlateGrey,
+    SlateGrey,
     }

    public readonly struct Color : IEquatable<Color>
    {
+    public static Color RebeccaPurple;
+    public static Color DarkGrey;
+    public static Color DarkSlateGrey;
+    public static Color DimGrey;
+    public static Color Grey;
+    public static Color LightGrey;
+    public static Color LightSlateGrey;
+    public static Color SlateGrey;
    }
}

Possibly related, System.Windows.Media.Colors

Usage Examples

using System.Drawing;

string text = String.Format(
    "The CSS color 'RebeccaPurple' has Red component of {0}", 
    Color.RebeccaPurple.R);

Alternative Designs

I have not considered any particular designs, only intending to report this as a bug where the existing behaviour differs from the intended behaviour.

Risks

This will change the total number of items in the Color enum, and (depending on whether the new ones are added in order, or to the end) might change the existing color locations.

---

I think the missing color values are:

• RebeccaPurple #663399
• DarkGrey #a9a9a9
• DarkSlateGrey #2f4f4f
• DimGrey #696969
• Grey #808080
• LightGrey #d3d3d3
• LightSlateGrey #778899
• SlateGrey #708090

[ghost]

Tagging subscribers to this area: @safern, @tannergooding
Notify danmosemsft if you want to be subscribed.

[safern]

THanks @HumanEquivalentUnit -- this would be an API proposal because we need to add these new members to [KnownColor](https://docs.microsoft.com/en-us/dotnet/api/system.drawing.knowncolor?view=net-5.0)

For new APIs we need to follow this process: https://github.com/dotnet/runtime/blob/97e553f5c3244b7df65ebfb7de3bb712081228cc/docs/project/api-review-process.md

So first we need to format the issue description to show that these are additions to KnownColor. Once the issue description is updated, we can mark it as api-ready-for-review and then it will be reviewed by the .NET API Reviewers.

[HumanEquivalentUnit]

@safern I have rewritten the issue description from the API proposal template, as best as I can. I have limited C# experience, and am not sure what the full diff would look like, or whether System.Windows.Media.Colors needs to be involved/included either.

NB. If the .Net colors are not intended to track the CSS colors, then I am not strongly advocating to add these. Only if they are supposed to be identical, then it seems like a bug that they differ at all.

[safern]

If the .Net colors are not intended to track the CSS colors

I'm not sure if it has been our intention over the past, but I think it doesn't hurt to add those colors if they help matching other frameworks with default colors.

Marked as ready for review.

[reflectronic]

If KnownColor members are being added, static members should be added to Color as well.

[safern]

If KnownColor members are being added, static members should be added to Color as well.

That is correct. Will add it to the description as well, thanks @reflectronic

[terrajobst]

Video

• For better or worse, framework spelling is en-us.
• If this would be parsing, supporting both grey and gray seems reasonable, but duplicating API surface to offer AE and BE feels wrong, so we don't feel we should be exposing those.

namespace System.Drawing
{
    public enum KnownColor
    {
        RebeccaPurple
    }
    public readonly struct Color : IEquatable<Color>
    {
        public static Color RebeccaPurple;
    }
}

[FireCubeStudios]

i want to try implementing this

[danmoseley]

@FireCubeStudios go for it!

[FireCubeStudios]

ok so i can get it done this weekend and i will send a pull request in.

[FireCubeStudios]

i added rebecca purple, but what to do with "grey", do i replace "gray" with "grey" or add both or add none? @HumanEquivalentUnit @terrajobst

[danmoseley]

@FireCubeStudios my understanding of the post above is they only want Rebecca purple: the others would simply be duplicating existing "grAy" entries with parallel "grEy" which they don't think makes sense to expose in API. Anywhere we parse such colors we should accept both, but we should represent both of them with the "grAy" spelling in the API.

[saucecontrol]

The thing about missing the 'Grey' spelling variants is that there are a lot of places where color name mappings are populated from the static properties on Color. There's an expectation that all W3C color names can be mapped through there, and that's not currently true.

Example from ColorConverter:

runtime/src/libraries/System.Drawing.Common/src/System/Drawing/ColorConverter.cs

Lines 329 to 343 in 6072e4d

	private static void FillConstants(Hashtable hash, Type enumType) {
	MethodAttributes attrs = MethodAttributes.Public | MethodAttributes.Static;
	PropertyInfo[] props = enumType.GetProperties();
	for (int i = 0; i < props.Length; i++) {
	PropertyInfo prop = props[i];
	if (prop.PropertyType == typeof(Color)) {
	MethodInfo method = prop.GetGetMethod();
	if (method != null && (method.Attributes & attrs) == attrs) {
	object[] tempIndex = null;
	hash[prop.Name] = prop.GetValue(null, tempIndex);
	}
	}
	}
	}

[danmoseley]

Ah, if there's an interop angle then possibly that should be revisited then. cc @safern @tannergooding who are area owners and will have a more interesting response than me.

[saucecontrol]

Interestingly, LightGrey is special-cased in ColorTranslator, but the other grey shades are not:

runtime/src/libraries/Common/src/System/Drawing/ColorTranslator.cs

Lines 248 to 252 in 6072e4d

	// special case. Html requires LightGrey, but .NET uses LightGray
	if (c.IsEmpty && string.Equals(htmlColor, "LightGrey", StringComparison.OrdinalIgnoreCase))
	{
	c = Color.LightGray;
	}

[HumanEquivalentUnit]

There's an expectation that all W3C color names can be mapped through there

What do you have in mind - who expects, or where is it documented that way? My assumption was "the colour lists are very close, so they probably intend to be identical", but the review video above says .Net Framework used "web" colours as a convenient source for a colour list, but never intended to be in sync with them.

[tannergooding]

cc @safern @tannergooding who are area owners and will have a more interesting response than me.

Given that these are largely the W3C color types, that we define other aliases such as aqua/cyan and fuchsia/magenta, and that we have existing special handling for at least one grey vs gray; I think it would be worthwhile to add the other aliases and just spec it out as explicitly mapping to the W3C color name/values defined for CSS: https://www.w3.org/wiki/CSS/Properties/color/keywords (the docs currently link to https://developer.mozilla.org/en-US/docs/Web/CSS/color_value in a couple places).

[akoeplinger]

RebeccaPurple was added in #42785 (thanks @FireCubeStudios!).
Do we still think we want to address the gray vs. grey colors or should we close this issue?

[safern]

Do we still think we want to address the gray vs. grey colors or should we close this issue?

I think we should add the aliases as @tannergooding mentioned.

[akoeplinger]

Does this need to go through another round of API review then? In @terrajobst's comment only the RebeccaPurple change was approved: #38244 (comment)

[terrajobst]

@tannergooding

I think it would be worthwhile to add the other aliases and just spec it out as explicitly mapping to the W3C color name/values defined for CSS:

I think that's a reasonable rationale and probably doesn't result in explosion. However, we should contain it here. I don't believe a general praxis of adding aliases for BE vs AE spelling makes any sense.

Another compromise would be to add the properties with AE but accept AE and BE when parsing.

@dotnet/fxdc, does anybody feel strongly here?

[bartonjs]

Another compromise would be to add the properties with AE but accept AE and BE when parsing.

does anybody feel strongly here?

I believe most, if not all, of the BCL is American English spelling, so I think my vote is for making the parser here support both while leaving IntelliSense consistent as American English (even though I usually spell grey with an e).

[tannergooding]

I think supporting parsing both without exposing both would be reasonable. We can then chalk the existing duplicates up as legacy/back-compat

[safern]

I'm going to close this as it fixed what was approved. If we want to add grey colors then we should open a new issue and should go through API Review