[UWP] ListView ItemSelected event will fire only once on selection changed (#1005)

* Add repro for 44886

* [UWP] Fire ListItemClicked when Selection changes

This will automatically set the value on the renderer and prevent the double event from firing.

* Clean up repro

* Update test case for delection scenario

* [Core] Allow ListView item deselection

* [UWP] Send events when item is deselected, too

* Test works better when you DO something.

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla44886.cs

@@ -0,0 +1,90 @@
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+using System.Collections.Generic;
+using System.Runtime.CompilerServices;
+using System.ComponentModel;
+
+#if UITEST
+using Xamarin.UITest;
+using NUnit.Framework;
+#endif
+
+namespace Xamarin.Forms.Controls.Issues
+{
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Bugzilla, 44886, "UWP Listview ItemSelected event triggered twice for each selection", PlatformAffected.UWP)]
+	public class Bugzilla44886 : TestContentPage
+	{
+		const string Item1 = "Item 1";
+		const string Instructions = "Select one of the items in the list. The text in blue should show 1, indicating that the ItemSelected event fired once. If it shows 2, this test has failed. Be sure to also test Keyboard selection and Narrator selection. On UWP, the ItemSelected event should fire when an item is highlighted and again when it is un-highlighted (by pressing spacebar).";
+		const string CountId = "countId";
+
+		Label _CountLabel = new Label { AutomationId = CountId, TextColor = Color.Blue };
+		MyViewModel _vm = new MyViewModel();
+
+		[Preserve(AllMembers = true)]
+		class MyViewModel : INotifyPropertyChanged
+		{
+			int _count;
+			public int Count
+			{
+				get { return _count; }
+				set
+				{
+					if (value != _count)
+					{
+						_count = value;
+						RaisePropertyChanged();
+					}
+				}
+			}
+
+			void RaisePropertyChanged([CallerMemberName] string propertyName = null)
+			{
+				PropertyChangedEventHandler handler = PropertyChanged;
+
+				handler?.Invoke(this, new PropertyChangedEventArgs(propertyName));
+			}
+
+			#region INotifyPropertyChanged implementation
+
+			public event PropertyChangedEventHandler PropertyChanged;
+
+			#endregion
+		}
+
+		protected override void Init()
+		{
+			BindingContext = _vm;
+
+			_CountLabel.SetBinding(Label.TextProperty, nameof(MyViewModel.Count));
+
+				var listView = new ListView
+			{
+				ItemsSource = new List<string> { Item1, "Item 2", "Item 3", "Item 4", "Item 5" }
+			};
+			listView.ItemSelected += ListView_ItemSelected;
+
+			var stack = new StackLayout { Children = { new Label { Text = Instructions }, _CountLabel, listView } };
+			Content = stack;
+		}
+
+		void ListView_ItemSelected(object sender, SelectedItemChangedEventArgs e)
+		{
+			_vm.Count++;
+		}
+
+#if UITEST
+		[Test]
+		public void Bugzilla44886Test()
+		{
+			RunningApp.WaitForElement(q => q.Marked(Item1));
+			RunningApp.Tap(q => q.Marked(Item1));
+
+			int count = int.Parse(RunningApp.Query(q => q.Marked(CountId))[0].Text);
+
+			Assert.IsTrue(count == 1);
+		}
+#endif
+	}
+}

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems

@@ -314,6 +314,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla54036.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla56896.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40161.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Bugzilla44886.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzila57749.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)ScrollViewObjectDisposed.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla58645.cs" />

Xamarin.Forms.Core/ListView.cs

@@ -399,17 +399,19 @@ namespace Xamarin.Forms
 
 			_previousRowSelected = inGroupIndex;
 			_previousGroupSelected = groupIndex;
-			if (cell == null)
+
+			// A11y: Keyboards and screen readers can deselect items, allowing -1 to be possible
+			if (cell == null && inGroupIndex != -1)
 			{
 				cell = group[inGroupIndex];
 			}
 
 			// Set SelectedItem before any events so we don't override any changes they may have made.
-			SetValueCore(SelectedItemProperty, cell.BindingContext, SetValueFlags.ClearOneWayBindings | SetValueFlags.ClearDynamicResource | (changed ? SetValueFlags.RaiseOnEqual : 0));
+			SetValueCore(SelectedItemProperty, cell?.BindingContext, SetValueFlags.ClearOneWayBindings | SetValueFlags.ClearDynamicResource | (changed ? SetValueFlags.RaiseOnEqual : 0));
 
-			cell.OnTapped();
+			cell?.OnTapped();
 
-			ItemTapped?.Invoke(this, new ItemTappedEventArgs(ItemsSource.Cast<object>().ElementAt(groupIndex), cell.BindingContext));
+			ItemTapped?.Invoke(this, new ItemTappedEventArgs(ItemsSource.Cast<object>().ElementAt(groupIndex), cell?.BindingContext));
 		}
 
 		[EditorBrowsable(EditorBrowsableState.Never)]

Xamarin.Forms.Platform.WinRT/ListViewRenderer.cs

@@ -62,7 +62,8 @@ namespace Xamarin.Forms.Platform.WinRT
 
 				if (List == null)
 				{
-					List = new WListView {
+					List = new WListView
+					{
 						IsSynchronizedWithCurrentItem = false,
 						ItemTemplate = (Windows.UI.Xaml.DataTemplate)WApp.Current.Resources["CellTemplate"],
 						HeaderTemplate = (Windows.UI.Xaml.DataTemplate)WApp.Current.Resources["View"],
@@ -465,10 +466,10 @@ namespace Xamarin.Forms.Platform.WinRT
 		void OnListItemClicked(int index)
 		{
 #if !WINDOWS_UWP
-	// If we're on the phone , we need to cache the selected item in case the handler 
-	// we're about to call changes any item indexes;
-	// in some cases, those index changes will throw an exception we can't catch if 
-	// the listview has an item selected
+			// If we're on the phone , we need to cache the selected item in case the handler 
+			// we're about to call changes any item indexes;
+			// in some cases, those index changes will throw an exception we can't catch if 
+			// the listview has an item selected
 			object selectedItem = null;
 			if (Device.Idiom == TargetIdiom.Phone)
 			{
@@ -508,7 +509,15 @@ namespace Xamarin.Forms.Platform.WinRT
 			RestorePreviousSelectedVisual();
 
 			if (e.AddedItems.Count == 0)
+			{
+				// Deselecting an item is a valid SelectedItem change.
+				if (Element.SelectedItem != List.SelectedItem)
+				{
+					OnListItemClicked(List.SelectedIndex);
+				}
+
 				return;
+			}
 
 			object cell = e.AddedItems[0];
 			if (cell == null)
@@ -525,9 +534,12 @@ namespace Xamarin.Forms.Platform.WinRT
 			}
 #endif
 
-			// A11y: Tapped event will not be routed when Narrator is active
-			// Also handles keyboard selection
-			SelectElementItem();
+			// A11y: Tapped event will not be routed when Narrator is active, so we need to handle it here.
+			// Also handles keyboard selection. 
+			// Default UWP behavior is that items are selected when you navigate to them via the arrow keys
+			// and deselected with the space bar, so this will remain the same.
+			if (Element.SelectedItem != List.SelectedItem)
+				OnListItemClicked(List.SelectedIndex);
 		}
 
 		FrameworkElement FindElement(object cell)
@@ -541,15 +553,6 @@ namespace Xamarin.Forms.Platform.WinRT
 			return null;
 		}
 
-		void SelectElementItem()
-		{
-			if (List.SelectedItem != null && Element.SelectedItem != List.SelectedItem)
-			{
-				((IElementController)Element).SetValueFromRenderer(ListView.SelectedItemProperty, List?.SelectedItem);
-				OnElementItemSelected(null, new SelectedItemChangedEventArgs(Element?.SelectedItem));
-			}
-		}
-
 #if WINDOWS_UWP
 		void RestorePreviousSelectedVisual()
 		{