Setup Shell to only use the LogicalChildren for reporting its LogicalChildren (#13462)

* Visual Tree Fixes

* Fix logical Chidlren

* - fix setting of view and test

* Call Add/RemoveLogicalChildren for Flyout Items

* - fix logical children

* - fix iOS

Xamarin.Forms.Core.UnitTests/ShellTests.cs

@@ -768,10 +768,10 @@ namespace Xamarin.Forms.Core.UnitTests
 			shell.FlyoutHeader = null;
 			shell.FlyoutHeader = layout;
 
-			Assert.True(shell.ChildrenNotDrawnByThisElement.Contains(layout));
+			Assert.True(shell.LogicalChildren.Contains(layout));
 			shell.FlyoutHeader = null;
 
-			Assert.False(shell.ChildrenNotDrawnByThisElement.Contains(layout));
+			Assert.False(shell.LogicalChildren.Contains(layout));
 		}
 
 

Xamarin.Forms.Core/Page.cs

@@ -379,7 +379,7 @@ namespace Xamarin.Forms
 				var element = LogicalChildren[i];
 				if (element is VisualElement c)
 				{
-					if (c.Bounds != startingLayout[i])
+					if (startingLayout.Count <= i || c.Bounds != startingLayout[i])
 					{
 						LayoutChanged?.Invoke(this, EventArgs.Empty);
 						return;

Xamarin.Forms.Core/Shell/Shell.cs

@@ -9,6 +9,7 @@ using System.Linq;
 using System.Runtime.CompilerServices;
 using System.Threading.Tasks;
 using Xamarin.Forms.Internals;
+using Xamarin.Forms.Xaml.Diagnostics;
 
 namespace Xamarin.Forms
 {
@@ -493,6 +494,40 @@ namespace Xamarin.Forms
 			return _navigationManager.GoToAsync(state, animate, false);
 		}
 
+		public void AddLogicalChild(Element element)
+		{
+			if (element == null)
+			{
+				return;
+			}
+
+			if (_logicalChildren.Contains(element))
+				return;
+
+			_logicalChildren.Add(element);
+			element.Parent = this;
+			OnChildAdded(element);
+			VisualDiagnostics.OnChildAdded(this, element);
+		}
+
+		public void RemoveLogicalChild(Element element)
+		{
+			if (element == null)
+			{
+				return;
+			}
+
+			element.Parent = null;
+
+			if (!_logicalChildren.Contains(element))
+				return;
+
+			var oldLogicalIndex = _logicalChildren.IndexOf(element);
+			_logicalChildren.Remove(element);
+			OnChildRemoved(element, oldLogicalIndex);
+			VisualDiagnostics.OnChildRemoved(this, element, oldLogicalIndex);
+		}
+
 		public static readonly BindableProperty CurrentItemProperty =
 			BindableProperty.Create(nameof(CurrentItem), typeof(ShellItem), typeof(Shell), null, BindingMode.TwoWay,
 				propertyChanging: OnCurrentItemChanging,
@@ -547,6 +582,11 @@ namespace Xamarin.Forms
 		ShellNavigationManager _navigationManager;
 		ShellFlyoutItemsManager _flyoutManager;
 
+		ObservableCollection<Element> _logicalChildren = new ObservableCollection<Element>();
+
+		internal override ReadOnlyCollection<Element> LogicalChildrenInternal => 
+			new ReadOnlyCollection<Element>(_logicalChildren);
+
 		public Shell()
 		{
 			_navigationManager = new ShellNavigationManager(this);
@@ -566,6 +606,7 @@ namespace Xamarin.Forms
 			Navigation = new NavigationImpl(this);
 			Route = Routing.GenerateImplicitRoute("shell");
 			Initialize();
+			InternalChildren.CollectionChanged += OnInternalChildrenCollectionChanged;
 		}
 
 		void Initialize()
@@ -784,38 +825,17 @@ namespace Xamarin.Forms
 		View FlyoutHeaderView
 		{
 			get => _flyoutHeaderView;
-			set
-			{
-				if (_flyoutHeaderView == value)
-					return;
-
-				if (_flyoutHeaderView != null)
-					OnChildRemoved(_flyoutHeaderView, -1);
-				_flyoutHeaderView = value;
-				if (_flyoutHeaderView != null)
-					OnChildAdded(_flyoutHeaderView);
-			}
 		}
 
 		View FlyoutFooterView
 		{
 			get => _flyoutFooterView;
-			set
-			{
-				if (_flyoutFooterView == value)
-					return;
-
-				if (_flyoutFooterView != null)
-					OnChildRemoved(_flyoutFooterView, -1);
-				_flyoutFooterView = value;
-				if (_flyoutFooterView != null)
-					OnChildAdded(_flyoutFooterView);
-			}
 		}
 
 		protected override void OnBindingContextChanged()
 		{
 			base.OnBindingContextChanged();
+
 			if (FlyoutHeaderView != null)
 				SetInheritedBindingContext(FlyoutHeaderView, BindingContext);
 
@@ -871,18 +891,6 @@ namespace Xamarin.Forms
 
 		bool ValidDefaultShellItem(Element child) => !(child is MenuShellItem);
 
-		internal override IEnumerable<Element> ChildrenNotDrawnByThisElement
-		{
-			get
-			{
-				if (FlyoutHeaderView != null)
-					yield return FlyoutHeaderView;
-
-				if (FlyoutFooterView != null)
-					yield return FlyoutFooterView;
-			}
-		}
-
 		protected virtual void OnNavigated(ShellNavigatedEventArgs args)
 		{
 		}
@@ -1094,6 +1102,18 @@ namespace Xamarin.Forms
 			return null;
 		}
 
+
+		void OnInternalChildrenCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
+		{
+			if (e.NewItems != null)
+				foreach (Element element in e.NewItems)
+					AddLogicalChild(element);
+
+			if (e.OldItems != null)
+				foreach (Element element in e.OldItems)
+					RemoveLogicalChild(element);
+		}
+
 		void NotifyFlyoutBehaviorObservers()
 		{
 			if (CurrentItem == null || GetVisiblePage() == null)
@@ -1106,56 +1126,44 @@ namespace Xamarin.Forms
 
 		void OnFlyoutHeaderChanged(object oldVal, object newVal)
 		{
-			if (FlyoutHeaderTemplate == null)
-			{
-				if (newVal is View newFlyoutHeader)
-					FlyoutHeaderView = newFlyoutHeader;
-				else
-					FlyoutHeaderView = null;
-			}
+			ShellTemplatedViewManager.OnViewDataChanged(
+				FlyoutHeaderTemplate,
+				ref _flyoutHeaderView,
+				newVal,
+				RemoveLogicalChild,
+				AddLogicalChild);
 		}
 
 		void OnFlyoutHeaderTemplateChanged(DataTemplate oldValue, DataTemplate newValue)
 		{
-			if (newValue == null)
-			{
-				if (FlyoutHeader is View flyoutHeaderView)
-					FlyoutHeaderView = flyoutHeaderView;
-				else
-					FlyoutHeaderView = null;
-			}
-			else
-			{
-				var newHeaderView = (View)newValue.CreateContent(FlyoutHeader, this);
-				FlyoutHeaderView = newHeaderView;
-			}
+			ShellTemplatedViewManager.OnViewTemplateChanged(
+				newValue,
+				ref _flyoutHeaderView,
+				FlyoutHeader,
+				RemoveLogicalChild,
+				AddLogicalChild,
+				this);
 		}
 
 		void OnFlyoutFooterChanged(object oldVal, object newVal)
 		{
-			if (FlyoutFooterTemplate == null)
-			{
-				if (newVal is View newFlyoutFooter)
-					FlyoutFooterView = newFlyoutFooter;
-				else
-					FlyoutFooterView = null;
-			}
+			ShellTemplatedViewManager.OnViewDataChanged(
+				FlyoutFooterTemplate,
+				ref _flyoutFooterView,
+				newVal,
+				RemoveLogicalChild,
+				AddLogicalChild);
 		}
 
 		void OnFlyoutFooterTemplateChanged(DataTemplate oldValue, DataTemplate newValue)
 		{
-			if (newValue == null)
-			{
-				if (FlyoutFooter is View flyoutFooterView)
-					FlyoutFooterView = flyoutFooterView;
-				else
-					FlyoutFooterView = null;
-			}
-			else
-			{
-				var newFooterView = (View)newValue.CreateContent(FlyoutFooter, this);
-				FlyoutFooterView = newFooterView;
-			}
+			ShellTemplatedViewManager.OnViewTemplateChanged(
+				newValue,
+				ref _flyoutFooterView,
+				FlyoutFooter,
+				RemoveLogicalChild,
+				AddLogicalChild,
+				this);
 		}
 
 		internal Element GetVisiblePage()
@@ -1194,8 +1202,15 @@ namespace Xamarin.Forms
 				PropertyPropagationExtensions.PropagatePropertyChanged(propertyName, this, new[] { FlyoutHeaderView });
 			if (FlyoutFooterView != null)
 				PropertyPropagationExtensions.PropagatePropertyChanged(propertyName, this, new[] { FlyoutFooterView });
+			if (FlyoutContentView != null)
+				PropertyPropagationExtensions.PropagatePropertyChanged(propertyName, this, new[] { FlyoutContentView });
 		}
 
+		protected override void LayoutChildren(double x, double y, double width, double height)
+		{
+			// Page by default tries to layout all logical children
+			// we don't want this behavior with shell
+		}
 
 		#region Shell Flyout Content
 
@@ -1223,44 +1238,27 @@ namespace Xamarin.Forms
 		View FlyoutContentView
 		{
 			get => _flyoutContentView;
-			set
-			{
-				if (_flyoutContentView == value)
-					return;
-
-				if (_flyoutContentView != null)
-					OnChildRemoved(_flyoutContentView, -1);
-				_flyoutContentView = value;
-				if (_flyoutContentView != null)
-					OnChildAdded(_flyoutContentView);
-			}
 		}
 
 		void OnFlyoutContentChanged(object oldVal, object newVal)
 		{
-			if (FlyoutContentTemplate == null)
-			{
-				if (newVal is View newFlyoutContent)
-					FlyoutContentView = newFlyoutContent;
-				else
-					FlyoutContentView = null;
-			}
+			ShellTemplatedViewManager.OnViewDataChanged(
+				FlyoutContentTemplate,
+				ref _flyoutContentView,
+				newVal,
+				RemoveLogicalChild,
+				AddLogicalChild);
 		}
 
 		void OnFlyoutContentTemplateChanged(DataTemplate oldValue, DataTemplate newValue)
 		{
-			if (newValue == null)
-			{
-				if (FlyoutContent is View flyoutContentView)
-					FlyoutContentView = flyoutContentView;
-				else
-					FlyoutContentView = null;
-			}
-			else
-			{
-				var newContentView = (View)newValue.CreateContent(FlyoutContent, this);
-				FlyoutContentView = newContentView;
-			}
+			ShellTemplatedViewManager.OnViewTemplateChanged(
+				newValue,
+				ref _flyoutContentView,
+				FlyoutContent,
+				RemoveLogicalChild,
+				AddLogicalChild,
+				this);
 		}
 
 		static void OnFlyoutContentChanging(BindableObject bindable, object oldValue, object newValue)

Xamarin.Forms.Core/Shell/ShellTemplatedViewManager.cs

@@ -0,0 +1,63 @@
+using System;
+using System.Collections.Generic;
+using System.Text;
+using Xamarin.Forms.Internals;
+
+namespace Xamarin.Forms
+{
+	public static class ShellTemplatedViewManager
+	{
+		public static void SetView(
+			ref View localView, 
+			View newView, 
+			Action<Element> OnChildRemoved, 
+			Action<Element> OnChildAdded)
+		{
+			if (localView == newView)
+				return;
+
+			if (localView != null)
+				OnChildRemoved(localView);
+			localView = newView;
+			if (localView != null)
+				OnChildAdded(localView);
+		}
+
+
+		public static void OnViewDataChanged(
+			DataTemplate currentViewTemplate,
+			ref View localViewRef,
+			object newViewData,
+			Action<Element> OnChildRemoved,
+			Action<Element> OnChildAdded)
+		{
+			if (currentViewTemplate == null)
+			{
+				SetView(ref localViewRef,
+					newViewData as View,
+					OnChildRemoved,
+					OnChildAdded);
+			}
+		}
+
+		public static void OnViewTemplateChanged(
+			DataTemplate newViewTemplate,
+			ref View localViewRef,
+			object currentViewData,
+			Action<Element> OnChildRemoved,
+			Action<Element> OnChildAdded,
+			Shell shell)
+		{
+			View newContentView = currentViewData as View;
+			if (newViewTemplate != null)
+			{ 
+				newContentView = (View)newViewTemplate.CreateContent(newViewTemplate, shell);
+			}
+
+			SetView(ref localViewRef,
+				newContentView,
+				OnChildRemoved,
+				OnChildAdded);
+		}
+	}
+}

Xamarin.Forms.Platform.Android/Renderers/ShellFlyoutRecyclerAdapter.cs

@@ -64,6 +64,16 @@ namespace Xamarin.Forms.Platform.Android
 			return id;
 		}
 
+		public override void OnViewRecycled(Java.Lang.Object holder)
+		{
+			if(holder is ElementViewHolder evh)
+			{
+				evh.Element = null;
+			}
+
+			base.OnViewRecycled(holder);
+		}
+
 		public override void OnBindViewHolder(RecyclerView.ViewHolder holder, int position)
 		{
 			var item = _listItems[position];
@@ -279,6 +289,7 @@ namespace Xamarin.Forms.Platform.Android
 					if (_element == value)
 						return;
 
+					_shell.RemoveLogicalChild(View);
 					if (_element != null && _element is BaseShellItem)
 					{
 						_element.ClearValue(Platform.RendererProperty);
@@ -287,12 +298,12 @@ namespace Xamarin.Forms.Platform.Android
 
 					_element = value;
 
-					// Set Parent after binding context so parent binding context doesn't propagate to view
-					View.BindingContext = value;
-					View.Parent = _shell;
+					// Set binding context before calling AddLogicalChild so parent binding context doesn't propagate to view
+					View.BindingContext = value;			
 
 					if (_element != null)
 					{
+						_shell.AddLogicalChild(View);
 						FastRenderers.AutomationPropertiesProvider.AccessibilitySettingsChanged(_itemView, value);
 						_element.SetValue(Platform.RendererProperty, _itemView);
 						_element.PropertyChanged += OnElementPropertyChanged;

Xamarin.Forms.Platform.UAP/Shell/ShellFlyoutItemRenderer.cs

@@ -21,7 +21,7 @@ namespace Xamarin.Forms.Platform.UWP
 		object _previousDataContext;
 		double _previousWidth;
 		FrameworkElement FrameworkElement { get; set; }
-
+		Shell _shell;
 		public ShellFlyoutItemRenderer()
 		{
 			this.DataContextChanged += OnDataContextChanged;
@@ -46,6 +46,7 @@ namespace Xamarin.Forms.Platform.UWP
 				if (_content.BindingContext is INotifyPropertyChanged inpc)
 					inpc.PropertyChanged -= ShellElementPropertyChanged;
 
+				_shell?.RemoveLogicalChild(_content);
 				_content.Cleanup();
 				_content.MeasureInvalidated -= OnMeasureInvalidated;
 				_content.BindingContext = null;
@@ -55,8 +56,8 @@ namespace Xamarin.Forms.Platform.UWP
 
 			var bo = (BindableObject)args.NewValue;
 			var element = bo as Element;
-			var shell = element?.FindParent<Shell>();
-			DataTemplate dataTemplate = (shell as IShellController)?.GetFlyoutItemDataTemplate(bo);
+			_shell = element?.FindParent<Shell>();
+			DataTemplate dataTemplate = (_shell as IShellController)?.GetFlyoutItemDataTemplate(bo);
 
 			if (bo != null)
 				bo.PropertyChanged += ShellElementPropertyChanged;
@@ -65,7 +66,8 @@ namespace Xamarin.Forms.Platform.UWP
 			{
 				_content = (View)dataTemplate.CreateContent();
 				_content.BindingContext = bo;
-				_content.Parent = shell;
+				_shell.AddLogicalChild(_content);
+				
 				_content.MeasureInvalidated += OnMeasureInvalidated;
 				IVisualElementRenderer renderer = Platform.CreateRenderer(_content);
 				Platform.SetRenderer(_content, renderer);

Xamarin.Forms.Platform.iOS/Renderers/ShellSearchResultsRenderer.cs

@@ -95,8 +95,8 @@ namespace Xamarin.Forms.Platform.iOS
 			if (cell == null)
 			{
 				var view = (View)template.CreateContent(context, _context.Shell);
-				view.Parent = _context.Shell;
 				view.BindingContext = context;
+				view.Parent = _context.Shell;
 				cell = new UIContainerCell(cellId, view);
 			}
 			else

Xamarin.Forms.Platform.iOS/Renderers/ShellTableViewSource.cs

@@ -33,7 +33,7 @@ namespace Xamarin.Forms.Platform.iOS
 					if (_cells != null)
 					{
 						foreach (var cell in _cells.Values)
-							cell.Disconnect();
+							cell.Disconnect(_context.Shell);
 					}
 
 					_cells = new Dictionary<Element, UIContainerCell>();
@@ -57,7 +57,7 @@ namespace Xamarin.Forms.Platform.iOS
 				if (_cells != null)
 				{
 					foreach (var cell in _cells.Values)
-						cell.Disconnect();
+						cell.Disconnect(_context.Shell);
 				}
 				_cells = new Dictionary<Element, UIContainerCell>();
 			}
@@ -120,18 +120,13 @@ namespace Xamarin.Forms.Platform.iOS
 			if (!_cells.TryGetValue(context, out cell))
 			{
 				var view = (View)template.CreateContent(context, _context.Shell);
-				cell = new UIContainerCell(cellId, view);
-
-				// Set Parent after binding context so parent binding context doesn't propagate to view
-				cell.BindingContext = context;
-				view.Parent = _context.Shell;
+				cell = new UIContainerCell(cellId, view, _context.Shell, context);
 			}
 			else
 			{
 				var view = _cells[context].View;
 				cell.Disconnect();
-				cell = new UIContainerCell(cellId, view);
-				cell.BindingContext = context;
+				cell = new UIContainerCell(cellId, view, _context.Shell, context);
 			}
 
 			cell.SetAccessibilityProperties(context);

Xamarin.Forms.Platform.iOS/Renderers/UIContainerCell.cs

@@ -8,22 +8,32 @@ namespace Xamarin.Forms.Platform.iOS
 	{
 		IVisualElementRenderer _renderer;
 		object _bindingContext;
+
 		internal Action<UIContainerCell> ViewMeasureInvalidated { get; set; }
 		internal NSIndexPath IndexPath { get; set; }
 		internal UITableView TableView { get; set; }
 
-		public UIContainerCell(string cellId, View view) : base(UITableViewCellStyle.Default, cellId)
+		internal UIContainerCell(string cellId, View view, Shell shell, object context) : base(UITableViewCellStyle.Default, cellId)
 		{
 			View = view;
 			View.MeasureInvalidated += MeasureInvalidated;
 			SelectionStyle = UITableViewCellSelectionStyle.None;
-
+			
 			_renderer = Platform.CreateRenderer(view);
 			Platform.SetRenderer(view, _renderer);
 
 			ContentView.AddSubview(_renderer.NativeView);
 			_renderer.NativeView.ClipsToBounds = true;
 			ContentView.ClipsToBounds = true;
+
+			BindingContext = context;
+			if (shell != null)
+				shell.AddLogicalChild(View);
+		}
+
+
+		public UIContainerCell(string cellId, View view) : this(cellId, view, null, null)
+		{
 		}
 
 		void MeasureInvalidated(object sender, System.EventArgs e)
@@ -39,7 +49,7 @@ namespace Xamarin.Forms.Platform.iOS
 			TableView.ReloadRows(new[] { IndexPath }, UITableViewRowAnimation.Automatic);
 		}
 
-		internal void Disconnect()
+		internal void Disconnect(Shell shell = null)
 		{
 			ViewMeasureInvalidated = null;
 			View.MeasureInvalidated -= MeasureInvalidated;
@@ -48,6 +58,9 @@ namespace Xamarin.Forms.Platform.iOS
 
 			_bindingContext = null;
 			Platform.SetRenderer(View, null);
+			if (shell != null)
+				shell.RemoveLogicalChild(shell);
+
 			View = null;
 			TableView = null;
 		}
@@ -72,7 +85,6 @@ namespace Xamarin.Forms.Platform.iOS
 					baseShell2.PropertyChanged += OnElementPropertyChanged;
 					UpdateVisualState();
 				}
-
 			}
 		}