Respond to feedback

dotnet · Jul 29, 2024 · 1e46757 · 1e46757
1 parent abdac5a
commit 1e46757
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 14 deletions.
diff --git a/src/libraries/System.Drawing.Primitives/src/System/Drawing/KnownColorTable.cs b/src/libraries/System.Drawing.Primitives/src/System/Drawing/KnownColorTable.cs
@@ -427,6 +427,11 @@ internal static class KnownColorTable
             KnownColorKindWeb,      // RebeccaPurple
         ];
 
+        // These values were based on manual investigation of dark mode themes in the
+        // Win32 Common Controls and WinUI. There aren't direct mappings published by
+        // Windows, these may change slightly when this feature is finalized to make
+        // sure we have the best experience in hybrid dark mode scenarios (mixing
+        // WPF, WinForms, and WinUI).
         private static ReadOnlySpan<uint> AlternateSystemColors =>
         [
             0,          // To align with KnownColor.ActiveBorder = 1
@@ -494,18 +499,24 @@ public static uint KnownColorToArgb(KnownColor color)
                  : ColorValueTable[(int)color];
         }
 
+        private static uint GetAlternateSystemColorArgb(KnownColor color)
+        {
+            // Shift the original (split) index to fit the alternate color map.
+            int index = color <= KnownColor.WindowText
+                ? (int)color
+                : (int)color - (int)KnownColor.ButtonFace + (int)KnownColor.WindowText + 1;
+
+            return AlternateSystemColors[index];
+        }
+
 #if FEATURE_WINDOWS_SYSTEM_COLORS
         public static uint GetSystemColorArgb(KnownColor color)
         {
             Debug.Assert(Color.IsKnownColorSystem(color));
 
-            if (!SystemColors.s_useAlternativeColorSet || HighContrastEnabled())
-            {
-                return ColorTranslator.COLORREFToARGB(Interop.User32.GetSysColor((byte)ColorValueTable[(int)color]));
-            }
-
-            int index = color <= KnownColor.WindowText ? (int)color : (int)color - (int)KnownColor.ButtonFace + (int)KnownColor.WindowText + 1;
-            return AlternateSystemColors[index];
+            return !SystemColors.s_useAlternativeColorSet || HighContrastEnabled()
+                ? ColorTranslator.COLORREFToARGB(Interop.User32.GetSysColor((byte)ColorValueTable[(int)color]))
+                : GetAlternateSystemColorArgb(color);
         }
 
         private static unsafe bool HighContrastEnabled()
@@ -529,13 +540,9 @@ public static uint GetSystemColorArgb(KnownColor color)
         {
             Debug.Assert(Color.IsKnownColorSystem(color));
 
-            if (!SystemColors.s_useAlternativeColorSet)
-            {
-                return ColorValueTable[(int)color];
-            }
-
-            int index = color <= KnownColor.WindowText ? (int)color : (int)color - (int)KnownColor.ButtonFace + (int)KnownColor.WindowText + 1;
-            return AlternateSystemColors[index];
+            return (!SystemColors.s_useAlternativeColorSet)
+                ? ColorValueTable[(int)color]
+                : GetAlternateSystemColorArgb(color);
         }
 #endif
     }

diff --git a/src/libraries/System.Drawing.Primitives/src/System/Drawing/SystemColors.cs b/src/libraries/System.Drawing.Primitives/src/System/Drawing/SystemColors.cs
@@ -60,6 +60,11 @@ public static class SystemColors
         /// </summary>
         /// <remarks>
         /// <para>
+        /// <see cref="KnownColor"/> <see cref="Color"/> values are always looked up every
+        /// time you use them and do not retain any other context. As such, existing
+        /// <see cref="Color"/> values will change when this property is set.
+        /// </para>
+        /// <para>
         /// On Windows, system <see cref="KnownColor"/> values will always return the current
         /// Windows color when the OS has a high contrast theme enabled.
         /// </para>