Fixed callback subscription edge cases

Author: Avid29

components/ColorAnalyzer/src/Contrast/ContrastHelper.Properties.cs

@@ -8,9 +8,16 @@ namespace CommunityToolkit.WinUI.Helpers;
 
 public partial class ContrastHelper
 {
+    // TODO: Handle gradient brushes?
     /// <summary>
     /// An attached property that defines the color to compare against.
     /// </summary>
+    /// <remarks>
+    /// This property can be attached to any <see cref="TextBlock"/> or <see cref="Control"/>
+    /// to update their <see cref="TextBlock.Foreground"/> or <see cref="Control.Foreground"/>.
+    /// If the original Foreground is not a <see cref="SolidColorBrush"/>, it will always apply contrast.
+    /// It can also be attached to any <see cref="SolidColorBrush"/> to update the <see cref="SolidColorBrush.Color"/>.
+    /// </remarks>
     public static readonly DependencyProperty OpponentProperty =
         DependencyProperty.RegisterAttached(
             "Opponent",
@@ -41,7 +48,15 @@ public partial class ContrastHelper
             typeof(ContrastHelper),
             new PropertyMetadata(Colors.Transparent));
 
-    // Tracks the callback on the original brush being updated.
+    // Tracks the SolidColorBrush we're monitoring for changes
+    private static readonly DependencyProperty CallbackObjectProperty =
+        DependencyProperty.RegisterAttached(
+            "CallbackObject",
+            typeof(DependencyObject),
+            typeof(ContrastHelper),
+            new PropertyMetadata(null));
+
+    // Tracks the callback token from the SolidColorBrush we are monitoring
     private static readonly DependencyProperty CallbackProperty =
         DependencyProperty.RegisterAttached(
             "Callback",
@@ -77,6 +92,10 @@ public partial class ContrastHelper
 
     private static void SetOriginal(DependencyObject obj, Color color) => obj.SetValue(OriginalColorProperty, color);
 
+    private static DependencyObject GetCallbackObject(DependencyObject obj) => (DependencyObject)obj.GetValue(CallbackObjectProperty);
+
+    private static void SetCallbackObject(DependencyObject obj, DependencyObject dp) => obj.SetValue(CallbackObjectProperty, dp);
+
     private static long GetCallback(DependencyObject obj) => (long)obj.GetValue(CallbackProperty);
 
     private static void SetCallback(DependencyObject obj, long value) => obj.SetValue(CallbackProperty, value);

components/ColorAnalyzer/src/Contrast/ContrastHelper.cs

@@ -11,21 +11,29 @@ namespace CommunityToolkit.WinUI.Helpers;
 /// </summary>
 public partial class ContrastHelper
 {
+    // When the helper is updating the color, this flag is set to avoid feedback loops
+    // It has no threading issues since all updates are on the UI thread
     private static bool _selfUpdate = false;
 
     private static void OnOpponentChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
     {
-        // Subscribe to brush updates 
-        if (GetCallback(d) is 0)
+        // Subscribe to brush updates if not already
+        if (GetCallbackObject(d) is null)
         {
             SubscribeToUpdates(d);
         }
 
+        // Update the actual color to ensure contrast
         ApplyContrastCheck(d);
     }
 
     private static void OnMinRatioChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
     {
+        // No opponent has been set, nothing to do
+        if (GetCallback(d) is 0)
+            return;
+        
+        // Update the actual color to ensure contrast
         ApplyContrastCheck(d);
     }
 
@@ -57,49 +65,97 @@ private static void ApplyContrastCheck(DependencyObject d)
 
     private static void SubscribeToUpdates(DependencyObject d)
     {
-        var brush = FindBrush(d, out var dp);
-        if (brush is null)
-            return;
+        // Get the original color from the brush and the property to monitor.
+        // Use Transparent as a sentinel value if the brush is not a SolidColorBrush
+        var solidColorBrush = FindBrush(d, out var dp);
+        var color = solidColorBrush?.Color ?? Colors.Transparent;
+
+        // Record the original color
+        SetOriginal(d, color);
 
-        // Apply initial update
-        SetOriginal(d, brush.Color);
+        // Rhetortical Question: Why don't we return if the solidColorBrush is null?
+        // Just because the brush is not a SolidColorBrush doesn't mean we can't monitor the
+        // Foreground property. We just can't monitor the brush's Color property
 
+        // If the original is not a SolidColorBrush, we need to monitor the Foreground property
         if (d is not SolidColorBrush)
         {
-            // Subscribe to updates from the source Foreground and Brush
-            d.RegisterPropertyChangedCallback(dp, (sender, prop) =>
+            // Subscribe to updates from the source Foreground
+            _ = d.RegisterPropertyChangedCallback(dp, (sender, prop) =>
             {
                 OnOriginalChangedFromSource(d, sender, prop);
             });
         }
 
+        // Subscribe to updates from the source SolidColorBrush
+        // If solidColorBrush is null, this is a no-op
+        SubscribeToBrushUpdates(d, solidColorBrush);
+    }
+
+    private static void SubscribeToBrushUpdates(DependencyObject d, SolidColorBrush? brush)
+    {
+        // No brush, nothing to do
+        if (brush is null)
+            return;
+
+        // Unsubscribe from previous brush if any
+        var oldBrush = GetCallbackObject(d);
+        var oldCallback = GetCallback(d);
+        oldBrush?.UnregisterPropertyChangedCallback(SolidColorBrush.ColorProperty, oldCallback);
+
+        // Subscribe to updates from the source SolidColorBrush
         var callback = brush.RegisterPropertyChangedCallback(SolidColorBrush.ColorProperty, (sender, prop) =>
         {
             OnOriginalChangedFromSource(d, sender, prop);
         });
 
+        // Track the callback so we don't double subscribe and can unsubscribe if needed
+        SetCallbackObject(d, brush);
         SetCallback(d, callback);
     }
 
     private static void OnOriginalChangedFromSource(DependencyObject obj, DependencyObject sender, DependencyProperty prop)
     {
-        // The contrast helper is updating the color.
+        // The contrast helper is updating the color
         // Ignore the assignment.
         if (_selfUpdate)
             return;
 
-        // Get brush
+        // Get the original color from the brush.
+        // We use the sender, not the obj, because the sender is the object that changed.
+        // Use Transparent as a sentinel value if the brush is not a SolidColorBrush
         var brush = FindBrush(sender, out _);
-        if (brush is null)
-            return;
+        var color = brush?.Color ?? Colors.Transparent;
 
         // Update original color
-        SetOriginal(obj, brush.Color);
+        SetOriginal(obj, color);
+
+        // The sender is the Foreground property, not the brush itself.
+        // This means the brush changed and our callback on the brush is dead.
+        // We need to subscribe to the new brush if it's a SolidColorBrush.
+        if (sender is not SolidColorBrush)
+        {
+            // Subscribe to the new brush
+            // Notice we're finding the brush on the object, not the sender this time.
+            // We may not find a SolidColorBrush, and that's ok.
+            var solidColorBrush = FindBrush(obj, out _);
+            SubscribeToBrushUpdates(obj, solidColorBrush);
+        }
 
         // Apply contrast correction
         ApplyContrastCheck(obj);
     }
 
+    /// <summary>
+    /// Finds the <see cref="SolidColorBrush"/> and its associated <see cref="DependencyProperty"/>
+    /// from <paramref name="d"/>..
+    /// </summary>
+    /// <param name="d">The attached <see cref="DependencyObject"/>.</param>
+    /// <param name="dp">
+    /// The <see cref="DependencyProperty"/> associated with the <see cref="SolidColorBrush"/>
+    /// belonging to <paramref name="d"/>.
+    /// </param>
+    /// <returns>The <see cref="SolidColorBrush"/> for <paramref name="d"/>.</returns>
     private static SolidColorBrush? FindBrush(DependencyObject d, out DependencyProperty? dp)
     {
         (SolidColorBrush? brush, dp) = d switch
@@ -131,6 +187,7 @@ private static void AssignColor(DependencyObject d, Color color)
                 break;
         }
 
+        // Unlock the original color updates
         _selfUpdate = false;
     }
 
@@ -151,8 +208,24 @@ private static double CalculateWCAGContrastRatio(Color color1, Color color2)
         return (lighter + 0.05f) / (darker + 0.05f);
     }
 
-    private static double CalculatePerceivedLuminance(Color color) =>
-        // Using the formula for perceived luminance
-        // Source WCAG guidelines: https://www.w3.org/TR/AERT/#color-contrast
-        (0.299f * color.R + 0.587f * color.G + 0.114f * color.B) / 255f;
+    private static double CalculatePerceivedLuminance(Color color)
+    {
+        // Color theory is a massive iceberg. Here's a peek at the tippy top:
+
+        // There's two (main) standards for calculating luminance from RGB values.
+        // ITU Rec. 709: Y = 0.2126 R + 0.7152 G + 0.0722 B
+        // ITU Rec. 601: Y = 0.299 R + 0.587 G + 0.114 B
+
+        // They're based on the standard ability of the human eye to perceive brightness,
+        // from different colors, as well as the average monitor's ability to produce them.
+        // Both standards produce similar results, but Rec. 709 is more accurate for modern displays.
+
+        // NOTE: If we for whatrever reason we ever need to optimize this code,
+        // we can make approximations using integer math instead of floating point math.
+        // The precise values are not critical, as long as the relative luminance is accurate.
+        // Like so: return (2 * color.R + 7 * color.G + color.B);
+
+        // TLDR: We use ITU Rec. 709 standard formula for perceived luminance.
+        return (0.2126f * color.R + 0.7152f * color.G + 0.0722 * color.B) / 255;
+    }
 }