Merge pull request #5936 from unoplatform/dev/djo/combobox-twoway-fix

fix(selector): Don't push null selection to binding when unloading Selector

src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_ListViewBase.cs

@@ -27,6 +27,7 @@
 using FluentAssertions.Execution;
 using Uno.Extensions;
 using Uno.UI.RuntimeTests.Helpers;
+using System.ComponentModel;
 
 namespace Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml_Controls
 {
@@ -1235,7 +1236,53 @@ public async Task When_ItemTemplateSelector_Set_And_Fluent()
 			}
 		}
 
+		[TestMethod]
+		public async Task When_Removed_From_Tree_And_Selection_TwoWay_Bound()
+		{
+			var page = new ListViewBoundSelectionPage();
+
+			var dc = new When_Removed_From_Tree_And_Selection_TwoWay_Bound_DataContext();
+			page.DataContext = dc;
+
+			WindowHelper.WindowContent = page;
+			await WindowHelper.WaitForLoaded(page);
+			Assert.IsNull(dc.MySelection);
+
+			var SUT = page.MyListView;
+			SUT.SelectedItem = "Rice";
+			await WindowHelper.WaitForIdle();
+			Assert.AreEqual("Rice", dc.MySelection);
+
+			page.HostPanel.Children.Remove(page.IntermediateGrid);
+			await WindowHelper.WaitForIdle();
+			Assert.IsNull(SUT.DataContext);
+
+			Assert.AreEqual("Rice", dc.MySelection);
+		}
+
 		private bool ApproxEquals(double value1, double value2) => Math.Abs(value1 - value2) <= 2;
+
+		private class When_Removed_From_Tree_And_Selection_TwoWay_Bound_DataContext : INotifyPropertyChanged
+		{
+			public event PropertyChangedEventHandler PropertyChanged;
+
+			public string[] MyItems { get; } = new[] { "Red beans", "Rice" };
+
+			private string _mySelection;
+			public string MySelection
+			{
+				get => _mySelection;
+				set
+				{
+					var changing = _mySelection != value;
+					_mySelection = value;
+					if (changing)
+					{
+						PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(MySelection)));
+					}
+				}
+			}
+		}
 	}
 
 	public partial class OnItemsChangedListView : ListView

src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/ListViewPages/ListViewBoundSelectionPage.xaml

@@ -0,0 +1,23 @@
+<Page x:Class="Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml_Controls.ListViewPages.ListViewBoundSelectionPage"
+	  xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
+	  xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
+	  xmlns:local="using:Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml_Controls.ListViewPages"
+	  xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
+	  xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
+	  mc:Ignorable="d"
+	  Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">
+
+	<StackPanel x:Name="HostPanel"
+				x:FieldModifier="public">
+		<Grid x:Name="IntermediateGrid"
+			  x:FieldModifier="public">
+			<ListView x:Name="MyListView"
+					  x:FieldModifier="public"
+					  ItemsSource="{Binding MyItems}"
+					  SelectedItem="{Binding MySelection, Mode=TwoWay}" />
+		</Grid>
+		<TextBlock x:Name="SelectionTextBlock"
+				   x:FieldModifier="public"
+				   Text="{Binding MySelection}" />
+	</StackPanel>
+</Page>

src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/ListViewPages/ListViewBoundSelectionPage.xaml.cs

@@ -0,0 +1,30 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Runtime.InteropServices.WindowsRuntime;
+using Windows.Foundation;
+using Windows.Foundation.Collections;
+using Windows.UI.Xaml;
+using Windows.UI.Xaml.Controls;
+using Windows.UI.Xaml.Controls.Primitives;
+using Windows.UI.Xaml.Data;
+using Windows.UI.Xaml.Input;
+using Windows.UI.Xaml.Media;
+using Windows.UI.Xaml.Navigation;
+
+// The Blank Page item template is documented at https://go.microsoft.com/fwlink/?LinkId=234238
+
+namespace Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml_Controls.ListViewPages
+{
+	/// <summary>
+	/// An empty page that can be used on its own or navigated to within a Frame.
+	/// </summary>
+	public sealed partial class ListViewBoundSelectionPage : Page
+	{
+		public ListViewBoundSelectionPage()
+		{
+			this.InitializeComponent();
+		}
+	}
+}

src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs

@@ -499,6 +499,17 @@ public void SetBindingValue(object value, DependencyProperty property)
 		/// </summary>
 		internal void SetBindingValue(DependencyPropertyDetails propertyDetails, object value)
 		{
+			var unregisteringInheritedProperties = _unregisteringInheritedProperties || _parentUnregisteringInheritedProperties;
+			if (unregisteringInheritedProperties)
+			{
+				// This guards against the scenario where inherited DataContext is removed when the view is removed from the visual tree,
+				// in which case 2-way bindings should not be updated.
+				if (this.Log().IsEnabled(Microsoft.Extensions.Logging.LogLevel.Debug))
+				{
+					this.Log().DebugFormat("SetSourceValue() not called because inherited property is being unset.");
+				}
+				return;
+			}
 			_properties.SetSourceValue(propertyDetails, value);
 		}
 

src/Uno.UI/UI/Xaml/DependencyObjectStore.cs

@@ -102,6 +102,10 @@ public static class TraceProvider
 		private bool _registeringInheritedProperties;
 		private bool _unregisteringInheritedProperties;
 		/// <summary>
+		/// An ancestor store is unregistering inherited properties.
+		/// </summary>
+		private bool _parentUnregisteringInheritedProperties;
+		/// <summary>
 		/// Is a theme-bound value currently being set?
 		/// </summary>
 		private bool _isSettingThemeBinding;
@@ -1586,8 +1590,7 @@ private void InvokeCallbacks(
 				{
 					for (var storeIndex = 0; storeIndex < _childrenStores.Count; storeIndex++)
 					{
-						var store = _childrenStores[storeIndex];
-						store.OnParentPropertyChangedCallback(instanceRef, property, eventArgs);
+						CallChildCallback(_childrenStores[storeIndex], instanceRef, property, eventArgs);
 					}
 				}
 			}
@@ -1625,6 +1628,30 @@ private void InvokeCallbacks(
 			}
 		}
 
+		private void CallChildCallback(DependencyObjectStore childStore, ManagedWeakReference instanceRef, DependencyProperty property, DependencyPropertyChangedEventArgs eventArgs)
+		{
+			var propagateUnregistering = (_unregisteringInheritedProperties || _parentUnregisteringInheritedProperties) && property == _dataContextProperty;
+#if !HAS_EXPENSIVE_TRYFINALLY // Try/finally incurs a very large performance hit in mono-wasm - https://github.com/dotnet/runtime/issues/50783
+			try
+#endif
+			{
+				if (propagateUnregistering)
+				{
+					childStore._parentUnregisteringInheritedProperties = true;
+				}
+				childStore.OnParentPropertyChangedCallback(instanceRef, property, eventArgs);
+			}
+#if !HAS_EXPENSIVE_TRYFINALLY // Try/finally incurs a very large performance hit in mono-wasm - https://github.com/dotnet/runtime/issues/50783
+			finally
+#endif
+			{
+				if (propagateUnregistering)
+				{
+					childStore._parentUnregisteringInheritedProperties = false;
+				}
+			}
+		}
+
 		/// <summary>
 		/// Updates the parent of the <paramref name="newValue"/> to the
 		/// <paramref name="actualInstanceAlias"/> and resets the parent of <paramref name="previousValue"/>.