Skip to content

Commit

Permalink
fix(selector): Don't push null selection to binding when unloading Se…
Browse files Browse the repository at this point in the history
…lector

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.
  • Loading branch information
davidjohnoliver committed May 5, 2021
1 parent 5ec3af6 commit ff0cd13
Show file tree
Hide file tree
Showing 5 changed files with 132 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
@@ -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();
}
}
}
7 changes: 7 additions & 0 deletions src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
27 changes: 25 additions & 2 deletions src/Uno.UI/UI/Xaml/DependencyObjectStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
}
}
}
Expand Down

0 comments on commit ff0cd13

Please sign in to comment.