From 1e46757fd55b3c57b85a78df4648edca5c53e3e0 Mon Sep 17 00:00:00 2001
From: Jeremy Kuhne <jeremy.kuhne@microsoft.com>
Date: Mon, 29 Jul 2024 10:09:57 -0700
Subject: [PATCH] Respond to feedback

---
 .../src/System/Drawing/KnownColorTable.cs     | 35 +++++++++++--------
 .../src/System/Drawing/SystemColors.cs        |  5 +++
 2 files changed, 26 insertions(+), 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
index 67cda2762d45d0..06922761f4b3eb 100644
--- 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
index a65f4149e283fd..daf6e33770ce91 100644
--- 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>