fix(visualstates): fix rolling back properties when the "next state" …

…doesn't modify a property, but modifies one of its subproperties

src/Uno.UI/DataBinding/BindingPath.BindingItem.cs

@@ -12,10 +12,8 @@ namespace Uno.UI.DataBinding
 {
 	internal partial class BindingPath
 	{
-		private sealed class BindingItem : IBindingItem, IDisposable
+		private sealed class BindingItem : IBindingItem, IDisposable, IEquatable<BindingItem>
 		{
-			private delegate void PropertyChangedHandler(object? previousValue, object? newValue, bool shouldRaiseValueChanged);
-
 			private ManagedWeakReference? _dataContextWeakStorage;
 			private Flags _flags;
 
@@ -540,6 +538,35 @@ public void NewValue()
 				public void NewValue(DependencyObject dependencyObject, DependencyPropertyChangedEventArgs args)
 					=> NewValue();
 			}
+
+			/// <remarks>
+			/// This is defined so that 2 BindingItems are equal if they refer to the same property on the same object
+			/// even if they're a part of 2 different "chains".
+			/// </remarks>
+			public bool Equals(BindingItem? that)
+			{
+				return that != null
+					&& this.PropertyType == that.PropertyType
+					&& !DependencyObjectStore.AreDifferent(this.DataContext, that.DataContext)
+					&& ComparePropertyName(this.PropertyName, that.PropertyName);
+
+				// This is a naive comparison that most definitely doesn't match WinUI, but it should be good enough
+				// for almost all cases.
+				static bool ComparePropertyName(string name1, string name2)
+				{
+					if (name1.StartsWith('['))
+					{
+						// for indexers, we look for an identical match.
+						return name1 == name2;
+					}
+					else
+					{
+						// e.g. "(Microsoft.UI.Xaml.Controls.Border.Background)" and "Background" should match.
+						return name1.Replace(")", "").Replace("(", "").Split(':', '.')[^1] ==
+							name2.Replace(")", "").Replace("(", "").Split(':', '.')[^1];
+					}
+				}
+			}
 		}
 	}
 }

src/Uno.UI/DataBinding/BindingPath.cs

@@ -23,7 +23,7 @@
 namespace Uno.UI.DataBinding
 {
 	[DebuggerDisplay("Path={_path} DataContext={_dataContextWeakStorage?.Target}")]
-	internal partial class BindingPath : IDisposable, IEquatable<BindingPath>
+	internal partial class BindingPath : IDisposable
 	{
 		private static List<IPropertyChangedRegistrationHandler> _propertyChangedHandlers = new List<IPropertyChangedRegistrationHandler>(2);
 		private readonly string _path;
@@ -317,11 +317,26 @@ internal void OnValueChanged(object? o)
 			Dispose(false);
 		}
 
-		public bool Equals(BindingPath? that)
+		public bool PrefixOfOrEqualTo(BindingPath? that)
 		{
-			return that != null
-				&& this.Path == that.Path
-				&& !DependencyObjectStore.AreDifferent(this.DataContext, that.DataContext);
+			if (that == null || DependencyObjectStore.AreDifferent(this.DataContext, that.DataContext))
+			{
+				return false;
+			}
+
+			var currentLeft = this._chain;
+			var currentRight = that._chain;
+			while (currentLeft != null)
+			{
+				if (!currentLeft.Equals(currentRight))
+				{
+					return false;
+				}
+				currentLeft = currentLeft.Next;
+				currentRight = currentRight.Next;
+			}
+
+			return true;
 		}
 
 		// We define Equals to be based on Path, so `RuntimeHelpers.GetHashCode(path)` doesn't work,

src/Uno.UI/UI/Xaml/VisualStateGroup.cs

@@ -9,6 +9,7 @@
 using Microsoft.UI.Xaml.Markup;
 using Uno.UI;
 using Windows.Foundation.Collections;
+using System.Buffers;
 using Uno.UI.DataBinding;
 using static Microsoft.UI.Xaml.Media.Animation.Timeline.TimelineState;
 
@@ -23,14 +24,6 @@ namespace Microsoft.UI.Xaml
 	[ContentProperty(Name = "States")]
 	public sealed partial class VisualStateGroup : DependencyObject
 	{
-		/// <summary>
-		/// Reusable HashSet to check properties set by the current state and not set by the next state
-		/// in <see cref="GoToState"/>. Since changing visual states should happen on the UI thread, there
-		/// are no concurrency problems. Consider this not a part of the state. Use it instead of
-		/// new HashSet() and clear before each use.
-		/// </summary>
-		private static HashSet<BindingPath> _propertiesNotToClear;
-
 		/// <summary>
 		/// The xaml scope in force at the time the VisualStateGroup was created.
 		/// </summary>
@@ -387,13 +380,15 @@ void ApplyTargetStateSetters()
 		// WinUI rolls back properties that are set by the current state that won't be set by the
 		// setters of the next state and won't be set by the nextAnimation.
 		// It doesn't matter how it's set by the current state (setters or transition or animation) or
-		// how it will possibly be set by the new state. Simply put, if the ne doesn't touch
+		// how it will possibly be set by the new state. Simply put, if the next state (or transition) doesn't touch
 		// the property in any way, roll it back. Similarly, if the animation of the new state
 		// doesn't set a property but it's set in the transition, it should be cleared after the
 		// transition.
 		// In other words, one can think of it as actually manipulating 3 states
 		// "Active" State (Setter & runningAnimation) --> Transition (if exists) --> New State (Setter & target.animation)
 		// Moving from one to the next rolls back the properties of the one before it.
+		// Note that "touching the property" also includes modifying a subproperty. Meaning, if the previous state sets
+		// the property Prop and the "next state" sets "Prop.SubProp", then we don't roll back Prop.
 		/*****************************************************************/
 		private static void ClearPropertiesNotSetInNextState(
 			IFrameworkElement element,
@@ -407,21 +402,22 @@ private static void ClearPropertiesNotSetInNextState(
 				return;
 			}
 
-			(_propertiesNotToClear ??= new HashSet<BindingPath>(new PathComparer())).Clear();
+			var propertiesNotToClear = ArrayPool<BindingPath>.Shared.Rent((nextSetters?.Count ?? 0) + (nextAnimation?.Children.Items.Count ?? 0));
+			var propsLength = 0;
 
 			if (nextSetters is { })
 			{
 				foreach (var setter in nextSetters.OfType<Setter>())
 				{
-					_propertiesNotToClear.Add(setter.TryGetOrCreateBindingPath(element));
+					propertiesNotToClear[propsLength++] = setter.TryGetOrCreateBindingPath(element);
 				}
 			}
 
 			if (nextAnimation is { })
 			{
 				foreach (var item in nextAnimation.Children.Items)
 				{
-					_propertiesNotToClear.Add(item.PropertyInfo);
+					propertiesNotToClear[propsLength++] = item.PropertyInfo;
 				}
 			}
 
@@ -430,7 +426,7 @@ private static void ClearPropertiesNotSetInNextState(
 				foreach (var setter in prevSetters.OfType<Setter>())
 				{
 					// Setters with a null path are always cleared.
-					if (setter.TryGetOrCreateBindingPath(element) is not { } path || !_propertiesNotToClear.Contains(path))
+					if (setter.TryGetOrCreateBindingPath(element) is not { } prevPath || !propertiesNotToClear.Any(path => prevPath.PrefixOfOrEqualTo(path)))
 					{
 						setter.ClearValue();
 					}
@@ -442,7 +438,7 @@ private static void ClearPropertiesNotSetInNextState(
 				foreach (var item in prevAnimation.Children.Items)
 				{
 					// Animations with a null path are always cleared.
-					if (item.PropertyInfo is not { } path || !_propertiesNotToClear.Contains(path))
+					if (item.PropertyInfo is not { } prevPath || !propertiesNotToClear.Any(path => prevPath.PrefixOfOrEqualTo(path)))
 					{
 						item.PropertyInfo.ClearValue();
 					}
@@ -595,13 +591,5 @@ private VisualState GetActiveTrigger()
 
 		public override string ToString()
 			=> Name ?? $"<unnamed group {GetHashCode()}>";
-
-		private class PathComparer : EqualityComparer<BindingPath>
-		{
-			public override bool Equals(BindingPath path1, BindingPath path2)
-				=> (path1 != null && path1.Equals(path2)) || (path1 == null && path2 == null);
-
-			public override int GetHashCode(BindingPath path) => path.GetHashCode();
-		}
 	}
 }