From ff0cd13c34c8a18cfa0284705c0c0137ff491b77 Mon Sep 17 00:00:00 2001 From: David Oliver <david.oliver@nventive.com> Date: Wed, 5 May 2021 13:50:48 -0400 Subject: [PATCH 1/2] fix(selector): Don't push null selection to binding when unloading Selector When a Selector (eg ListView or ComboBox) is removed from the visual tree, if its DataContext is inherited and ItemsSource and SelectedItem are both bound, the UWP behavior is that the ItemsSource will be set to null, which will set SelectedItem to null, but the null selection won't be pushed through the two-way binding. This change approximates the same behavior under Uno's binding system by detecting that scenario and not updating two-way bindings when an inherited DataContext is being cleared. --- .../Given_ListViewBase.cs | 47 +++++++++++++++++++ .../ListViewBoundSelectionPage.xaml | 23 +++++++++ .../ListViewBoundSelectionPage.xaml.cs | 30 ++++++++++++ .../UI/Xaml/DependencyObjectStore.Binder.cs | 7 +++ src/Uno.UI/UI/Xaml/DependencyObjectStore.cs | 27 ++++++++++- 5 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/ListViewPages/ListViewBoundSelectionPage.xaml create mode 100644 src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/ListViewPages/ListViewBoundSelectionPage.xaml.cs diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_ListViewBase.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_ListViewBase.cs index 6a93bd1af228..42a7e10dee41 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_ListViewBase.cs +++ b/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 diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/ListViewPages/ListViewBoundSelectionPage.xaml b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/ListViewPages/ListViewBoundSelectionPage.xaml new file mode 100644 index 000000000000..3ce973891d7e --- /dev/null +++ b/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> diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/ListViewPages/ListViewBoundSelectionPage.xaml.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/ListViewPages/ListViewBoundSelectionPage.xaml.cs new file mode 100644 index 000000000000..2fd349b9682e --- /dev/null +++ b/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(); + } + } +} diff --git a/src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs b/src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs index 3431a7bb42b4..b391b972a3ff 100644 --- a/src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs +++ b/src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs @@ -499,6 +499,13 @@ 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. + return; + } _properties.SetSourceValue(propertyDetails, value); } diff --git a/src/Uno.UI/UI/Xaml/DependencyObjectStore.cs b/src/Uno.UI/UI/Xaml/DependencyObjectStore.cs index 48ed7c40225f..943ffad37f9f 100644 --- a/src/Uno.UI/UI/Xaml/DependencyObjectStore.cs +++ b/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; @@ -1582,8 +1586,27 @@ private void InvokeCallbacks( { for (var storeIndex = 0; storeIndex < _childrenStores.Count; storeIndex++) { - var store = _childrenStores[storeIndex]; - store.OnParentPropertyChangedCallback(instanceRef, property, eventArgs); + var childStore = _childrenStores[storeIndex]; + 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; + } + } } } } From 9d48d492e0f4496184cc4bbe8c9ad0994c17a16d Mon Sep 17 00:00:00 2001 From: David Oliver <david.oliver@nventive.com> Date: Fri, 7 May 2021 06:07:04 -0400 Subject: [PATCH 2/2] chore: React to code review --- .../UI/Xaml/DependencyObjectStore.Binder.cs | 4 ++ src/Uno.UI/UI/Xaml/DependencyObjectStore.cs | 46 ++++++++++--------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs b/src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs index b391b972a3ff..fb49ff37daa6 100644 --- a/src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs +++ b/src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs @@ -504,6 +504,10 @@ internal void SetBindingValue(DependencyPropertyDetails propertyDetails, object { // 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); diff --git a/src/Uno.UI/UI/Xaml/DependencyObjectStore.cs b/src/Uno.UI/UI/Xaml/DependencyObjectStore.cs index 943ffad37f9f..56ab5f042e9f 100644 --- a/src/Uno.UI/UI/Xaml/DependencyObjectStore.cs +++ b/src/Uno.UI/UI/Xaml/DependencyObjectStore.cs @@ -1586,27 +1586,7 @@ private void InvokeCallbacks( { for (var storeIndex = 0; storeIndex < _childrenStores.Count; storeIndex++) { - var childStore = _childrenStores[storeIndex]; - 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; - } - } + CallChildCallback(_childrenStores[storeIndex], instanceRef, property, eventArgs); } } } @@ -1644,6 +1624,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"/>.