[Android] Isolate fragment management for children of MasterDetailPage (#136)

* Isolate fragment management for children of MasterDetailPage in AppCompat
If a MasterDetailPage hosts NavigationPages or TabbedPages in either the
Master or Detail sections, wrap those pages in their own Fragment (and
ChildFragmentManager) to isolate their Fragment management operations and
avoid recursive entry into the executePendingTransactions method
Also fix a disposal bug in the custom MDP renderer in Control Gallery

* Remove MDP Split setting that breaks test on iPad

Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs

@@ -4,6 +4,7 @@ using Android.App;
 using System.Collections.Generic;
 using Android.Views;
 using System.Collections;
+using System.ComponentModel;
 using System.Linq;
 using Xamarin.Forms.Controls;
 using Xamarin.Forms.Platform.Android;
@@ -29,9 +30,10 @@ using System.Reflection;
 #endif
 namespace Xamarin.Forms.ControlGallery.Android
 {
-
 	public class NativeDroidMasterDetail : Xamarin.Forms.Platform.Android.AppCompat.MasterDetailPageRenderer
 	{
+		MasterDetailPage _page;
+
 		protected override void OnElementChanged(VisualElement oldElement, VisualElement newElement)
 		{
 			base.OnElementChanged(oldElement, newElement);
@@ -41,16 +43,32 @@ namespace Xamarin.Forms.ControlGallery.Android
 				return;
 			}
 
-			MasterDetailPage page = newElement as MasterDetailPage;
-			page.PropertyChanged += (object sender, System.ComponentModel.PropertyChangedEventArgs e) => pChange();
-			page.LayoutChanged += Page_LayoutChanged;
+			_page = newElement as MasterDetailPage;
+			_page.PropertyChanged += Page_PropertyChanged; 
+			_page.LayoutChanged += Page_LayoutChanged;
 		}
 
-		private void Page_LayoutChanged(object sender, EventArgs e)
+		void Page_PropertyChanged(object sender, PropertyChangedEventArgs propertyChangedEventArgs)
 		{
 			pChange();
 		}
 
+		void Page_LayoutChanged(object sender, EventArgs e)
+		{
+			pChange();
+		}
+
+		protected override void Dispose(bool disposing)
+		{
+			if (disposing && _page != null)
+			{
+				_page.LayoutChanged -= Page_LayoutChanged;
+				_page.PropertyChanged -= Page_PropertyChanged;
+			}
+
+			base.Dispose(disposing);
+		}
+
 		public void pChange()
 		{
 			if (Build.VERSION.SdkInt >= BuildVersionCodes.Lollipop)

Xamarin.Forms.ControlGallery.Android/Properties/AndroidManifest.xml

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <manifest xmlns:android="http://schemas.android.com/apk/res/android" android:versionCode="1" android:versionName="1.0" package="AndroidControlGallery.AndroidControlGallery" android:installLocation="auto">
 	<uses-sdk android:minSdkVersion="15" android:targetSdkVersion="23" />
 	<uses-permission android:name="android.permission.INTERNET" />

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

@@ -0,0 +1,225 @@
+using System;
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+#if UITEST
+using NUnit.Framework;
+#endif
+
+namespace Xamarin.Forms.Controls
+{
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Bugzilla, 40333, "[Android] IllegalStateException: Recursive entry to executePendingTransactions", PlatformAffected.Android)]
+	public class Bugzilla40333 : TestNavigationPage
+	{
+		const string StartNavPageTestId = "StartNavPageTest";
+		const string OpenMasterId = "OpenMaster";
+		const string StartTabPageTestId = "StartTabPageTest";
+		const string StillHereId = "3 Still Here";
+		const string ClickThisId = "2 Click This";
+
+		protected override void Init()
+		{
+			var navButton = new Button { Text = "Test With NavigationPage", AutomationId = StartNavPageTestId };
+			navButton.Clicked += (sender, args) => { PushAsync(new _40333MDP(false)); };
+
+			var tabButton = new Button { Text = "Test With TabbedPage", AutomationId = StartTabPageTestId };
+			tabButton.Clicked += (sender, args) => { PushAsync(new _40333MDP(true)); };
+
+			var content = new ContentPage {
+				Content = new StackLayout {
+					Children = { navButton, tabButton }
+				}
+			};
+
+			PushAsync(content);
+		}
+
+		[Preserve(AllMembers = true)]
+		public class _40333MDP : TestMasterDetailPage
+		{
+			readonly bool _showTabVersion;
+
+			public _40333MDP(bool showTabVersion)
+			{
+				_showTabVersion = showTabVersion;
+			}
+
+			protected override void Init()
+			{
+				Master = new NavigationPage(new _40333NavPusher("Root")) { Title = "MasterNav" };
+
+				if (_showTabVersion)
+				{
+					Detail = new TabbedPage() { Title = "DetailNav", Children = { new _40333DetailPage("T1") } };
+				}
+				else
+				{
+					Detail = new NavigationPage(new _40333DetailPage("Detail") { Title = "DetailPage" }) { Title = "DetailNav" };
+				}
+			}
+
+			[Preserve(AllMembers = true)]
+			public class _40333DetailPage : ContentPage
+			{
+				public _40333DetailPage(string title)
+				{
+					Title = title;
+
+					var openMaster = new Button {
+						Text = "Open Master",
+						AutomationId = OpenMasterId
+					};
+
+					openMaster.Clicked += (sender, args) => ((MasterDetailPage)Parent.Parent).IsPresented = true;
+
+					Content = new StackLayout() {
+						Children = { new Label { Text = "Detail Text" }, openMaster }
+					};
+				}
+			}
+
+			[Preserve(AllMembers = true)]
+			public class _40333NavPusher : ContentPage
+			{
+				readonly ListView _listView = new ListView();
+
+				public _40333NavPusher(string title)
+				{
+					Title = title;
+
+					_listView.ItemTemplate = new DataTemplate(() =>
+					{
+						var lbl = new Label();
+						lbl.SetBinding(Label.TextProperty, ".");
+						lbl.AutomationId = lbl.Text;
+
+						var result = new ViewCell
+						{
+							View = new StackLayout
+							{
+								Orientation = StackOrientation.Horizontal,
+								Children =
+								{
+									lbl
+								}
+							}
+						};
+
+						return result;
+					});
+
+					_listView.ItemsSource = new[] { "1", ClickThisId, StillHereId };
+					_listView.ItemTapped += OnItemTapped;
+
+					Content = new StackLayout {
+						Children = { _listView }
+					};
+				}
+
+				async void OnItemTapped(object sender, EventArgs e)
+				{
+					var masterNav = ((MasterDetailPage)this.Parent.Parent).Master.Navigation;
+
+					var newTitle = $"{Title}.{_listView.SelectedItem}";
+					await masterNav.PushAsync(new _40333NavPusher(newTitle));
+				}
+
+				protected override async void OnAppearing()
+				{
+					base.OnAppearing();
+
+					var newPage = new _40333DetailPage(Title);
+
+					var detailNav = ((MasterDetailPage)this.Parent.Parent).Detail.Navigation;
+					var currentRoot = detailNav.NavigationStack[0];
+					detailNav.InsertPageBefore(newPage, currentRoot);
+					await detailNav.PopToRootAsync();
+				}
+			}
+
+			[Preserve(AllMembers = true)]
+			public class _40333TabPusher : ContentPage
+			{
+				readonly ListView _listView = new ListView();
+
+				public _40333TabPusher(string title)
+				{
+					Title = title;
+
+					_listView.ItemTemplate = new DataTemplate(() => {
+						var lbl = new Label();
+						lbl.SetBinding(Label.TextProperty, ".");
+						lbl.AutomationId = lbl.Text;
+
+						var result = new ViewCell {
+							View = new StackLayout {
+								Orientation = StackOrientation.Horizontal,
+								Children =
+								{
+									lbl
+								}
+							}
+						};
+
+						return result;
+					});
+
+					_listView.ItemsSource = new[] { "1", ClickThisId, StillHereId };
+					_listView.ItemTapped += OnItemTapped;
+
+					Content = new StackLayout {
+						Children = { _listView }
+					};
+				}
+
+				async void OnItemTapped(object sender, EventArgs e)
+				{
+					var masterNav = ((MasterDetailPage)this.Parent.Parent).Master.Navigation;
+
+					var newTitle = $"{Title}.{_listView.SelectedItem}";
+					await masterNav.PushAsync(new _40333TabPusher(newTitle));
+				}
+
+				protected override void OnAppearing()
+				{
+					base.OnAppearing();
+
+					var newPage = new _40333DetailPage(Title);
+
+					var detailTab = (TabbedPage)((MasterDetailPage)this.Parent.Parent).Detail;
+
+					detailTab.Children.Add(newPage);
+					detailTab.CurrentPage = newPage;
+				}
+			}
+		}
+
+#if UITEST
+		[Test]
+		public void ClickingOnMenuItemInMasterDoesNotCrash_NavPageVersion()
+		{
+			RunningApp.Tap(q => q.Marked(StartNavPageTestId));
+			RunningApp.WaitForElement(q => q.Marked(OpenMasterId));
+
+			RunningApp.Tap(q => q.Marked(OpenMasterId));
+			RunningApp.WaitForElement(q => q.Marked(ClickThisId));
+
+			RunningApp.Tap(q => q.Marked(ClickThisId));
+			RunningApp.WaitForElement(q => q.Marked(StillHereId)); // If the bug isn't fixed, the app will have crashed by now
+		}
+
+		[Test]
+		public void ClickingOnMenuItemInMasterDoesNotCrash_TabPageVersion()
+		{
+			RunningApp.Tap(q => q.Marked(StartTabPageTestId));
+			RunningApp.WaitForElement(q => q.Marked(OpenMasterId));
+
+			RunningApp.Tap(q => q.Marked(OpenMasterId));
+			RunningApp.WaitForElement(q => q.Marked(ClickThisId));
+
+			RunningApp.Tap(q => q.Marked(ClickThisId));
+			RunningApp.WaitForElement(q => q.Marked(StillHereId)); // If the bug isn't fixed, the app will have crashed by now
+		}
+#endif
+	}
+}

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

@@ -101,6 +101,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40173.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla39821.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40185.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40333.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)CarouselAsync.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla34561.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla34727.cs" />

Xamarin.Forms.Controls/App.cs

@@ -24,7 +24,7 @@ namespace Xamarin.Forms.Controls
 		{
 			_testCloudService = DependencyService.Get<ITestCloudService>();
 			InitInsights();
-			//MainPage = new MainPageLifeCycleTests();
+
 			MainPage = new MasterDetailPage
 			{
 				Master = new ContentPage { Title = "Master", BackgroundColor = Color.Red },

Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs

@@ -11,6 +11,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 	{
 		readonly WeakReference _pageReference;
 
+		Action<PageContainer> _onCreateCallback;
 		bool? _isVisible;
 		PageContainer _pageContainer;
 		IVisualElementRenderer _visualElementRenderer;
@@ -51,6 +52,11 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 			return new FragmentContainer(page) { Arguments = new Bundle() };
 		}
 
+		public void SetOnCreateCallback(Action<PageContainer> callback)
+		{
+			_onCreateCallback = callback;
+		}
+
 		public override AView OnCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState)
 		{
 			if (Page != null)
@@ -59,6 +65,9 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 				Android.Platform.SetRenderer(Page, _visualElementRenderer);
 
 				_pageContainer = new PageContainer(Forms.Context, _visualElementRenderer, true);
+
+				_onCreateCallback?.Invoke(_pageContainer);
+
 				return _pageContainer;
 			}
 

Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs

@@ -0,0 +1,92 @@
+using Android.App;
+using Android.Content;
+using Fragment = Android.Support.V4.App.Fragment;
+using FragmentManager = Android.Support.V4.App.FragmentManager;
+using FragmentTransaction = Android.Support.V4.App.FragmentTransaction;
+
+namespace Xamarin.Forms.Platform.Android.AppCompat
+{
+	internal class MasterDetailContainer : Xamarin.Forms.Platform.Android.MasterDetailContainer, IManageFragments
+	{
+		PageContainer _pageContainer;
+		FragmentManager _fragmentManager;
+		readonly bool _isMaster;
+		readonly MasterDetailPage _parent;
+
+		public MasterDetailContainer(MasterDetailPage parent, bool isMaster, Context context) : base(parent, isMaster, context)
+		{
+			Id = FormsAppCompatActivity.GetUniqueId();
+			_parent = parent;
+			_isMaster = isMaster;
+		}
+
+		FragmentManager FragmentManager => _fragmentManager ?? (_fragmentManager = ((FormsAppCompatActivity)Context).SupportFragmentManager);
+
+		protected override void OnLayout(bool changed, int l, int t, int r, int b)
+		{
+			base.OnLayout(changed, l, t, r, b);
+
+			// If we're using a PageContainer (i.e., we've wrapped our contents in a Fragment),
+			// Make sure that it gets laid out
+			if (_pageContainer != null)
+			{
+				if (_isMaster)
+				{
+					var width = (int)Context.ToPixels(_parent.MasterBounds.Width);
+                    // When the base class computes the size of the Master container, it starts at the top of the 
+                    // screen and adds padding (_parent.MasterBounds.Top) to leave room for the status bar
+                    // When this container is laid out, it's already starting from the adjusted y value of the parent,
+                    // so we subtract _parent.MasterBounds.Top from our starting point (to get 0) and add it to the 
+                    // bottom (so the master page stretches to the bottom of the screen)
+					var height = (int)Context.ToPixels(_parent.MasterBounds.Height + _parent.MasterBounds.Top); 
+					_pageContainer.Layout(0, 0, width, height);
+				}
+				else
+				{
+					_pageContainer.Layout(l, t, r, b);
+				}
+
+				_pageContainer.Child.UpdateLayout();
+			}
+		}
+
+		protected override void AddChildView(VisualElement childView)
+		{
+			_pageContainer = null;
+
+			Page page = childView as NavigationPage ?? (Page)(childView as TabbedPage);
+
+			if (page == null)
+			{
+				// Not a NavigationPage or TabbedPage? Just do the normal thing
+				base.AddChildView(childView);
+			}
+			else
+			{
+				// The renderers for NavigationPage and TabbedPage both host fragments, so they need to be wrapped in a 
+				// FragmentContainer in order to get isolated fragment management
+
+				Fragment fragment = FragmentContainer.CreateInstance(page);
+				
+				var fc = fragment as FragmentContainer;
+				fc?.SetOnCreateCallback(pc =>
+				{
+					_pageContainer = pc;
+					SetDefaultBackgroundColor(pc.Child);
+				});
+
+				FragmentTransaction transaction = FragmentManager.BeginTransaction();
+				transaction.DisallowAddToBackStack();
+				transaction.Add(Id, fragment);
+				transaction.SetTransition((int)FragmentTransit.FragmentOpen);
+				transaction.Commit();
+			}
+		}
+
+		public void SetFragmentManager(FragmentManager fragmentManager)
+		{
+			if (_fragmentManager == null)
+				_fragmentManager = fragmentManager;
+		}
+	}
+}

Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs

@@ -3,10 +3,11 @@ using System.ComponentModel;
 using System.Threading.Tasks;
 using Android.Support.V4.Widget;
 using Android.Views;
+using Android.Support.V4.App;
 
 namespace Xamarin.Forms.Platform.Android.AppCompat
 {
-	public class MasterDetailPageRenderer : DrawerLayout, IVisualElementRenderer, DrawerLayout.IDrawerListener
+	public class MasterDetailPageRenderer : DrawerLayout, IVisualElementRenderer, DrawerLayout.IDrawerListener, IManageFragments
 	{
 		#region Statics
 
@@ -17,12 +18,12 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 		int _currentLockMode = -1;
 		MasterDetailContainer _detailLayout;
-
+		MasterDetailContainer _masterLayout;
 		bool _disposed;
 		bool _isPresentingFromCore;
-		MasterDetailContainer _masterLayout;
 		bool _presented;
 		VisualElementTracker _tracker;
+		FragmentManager _fragmentManager;
 
 		public MasterDetailPageRenderer() : base(Forms.Context)
 		{
@@ -68,6 +69,12 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 			UpdateIsPresented();
 		}
 
+		void IManageFragments.SetFragmentManager(FragmentManager fragmentManager)
+		{
+			if (_fragmentManager == null)
+				_fragmentManager = fragmentManager;
+		}
+
 		VisualElement IVisualElementRenderer.Element => Element;
 
 		event EventHandler<VisualElementChangedEventArgs> IVisualElementRenderer.ElementChanged
@@ -114,6 +121,12 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 						LayoutParameters = new LayoutParams(ViewGroup.LayoutParams.WrapContent, ViewGroup.LayoutParams.WrapContent) { Gravity = (int)GravityFlags.Start }
 					};
 
+					if (_fragmentManager != null)
+					{
+						_detailLayout.SetFragmentManager(_fragmentManager);
+						_masterLayout.SetFragmentManager(_fragmentManager);
+					}
+
 					AddView(_detailLayout);
 					AddView(_masterLayout);
 
@@ -329,7 +342,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 
 		void UpdateMaster()
 		{
-			MasterDetailContainer masterContainer = _masterLayout;
+			Android.MasterDetailContainer masterContainer = _masterLayout;
 			if (masterContainer == null)
 				return;
 

Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs

@@ -85,7 +85,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 			}
 		}
 
-		public void SetFragmentManager(FragmentManager childFragmentManager)
+		void IManageFragments.SetFragmentManager(FragmentManager childFragmentManager)
 		{
 			if (_fragmentManager == null)
 				_fragmentManager = childFragmentManager;

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs

@@ -28,7 +28,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 			AutoPackage = false;
 		}
 
-		public FragmentManager FragmentManager => _fragmentManager ?? (_fragmentManager = ((FormsAppCompatActivity)Context).SupportFragmentManager);
+		FragmentManager FragmentManager => _fragmentManager ?? (_fragmentManager = ((FormsAppCompatActivity)Context).SupportFragmentManager);
 
 		internal bool UseAnimations
 		{
@@ -43,7 +43,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat
 			}
 		}
 
-		public void SetFragmentManager(FragmentManager childFragmentManager)
+		void IManageFragments.SetFragmentManager(FragmentManager childFragmentManager)
 		{
 			if (_fragmentManager == null)
 				_fragmentManager = childFragmentManager;

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

@@ -1,8 +1,10 @@
 using System;
+using Android.App;
 using Android.Content;
 using Android.Content.Res;
 using Android.Runtime;
 using Android.Views;
+using Android.Widget;
 
 namespace Xamarin.Forms.Platform.Android
 {
@@ -40,19 +42,24 @@ namespace Xamarin.Forms.Platform.Android
 
 				if (_childView == null)
 					return;
+				
+				AddChildView(_childView);
+			}
+		}
 
-				IVisualElementRenderer renderer = Platform.GetRenderer(_childView);
-				if (renderer == null)
-					Platform.SetRenderer(_childView, renderer = Platform.CreateRenderer(_childView));
+		protected virtual void AddChildView(VisualElement childView)
+		{
+			IVisualElementRenderer renderer = Platform.GetRenderer(childView);
+			if (renderer == null)
+				Platform.SetRenderer(childView, renderer = Platform.CreateRenderer(childView));
 
-				if (renderer.ViewGroup.Parent != this)
-				{
-					if (renderer.ViewGroup.Parent != null)
-						renderer.ViewGroup.RemoveFromParent();
-					SetDefaultBackgroundColor(renderer);
-					AddView(renderer.ViewGroup);
-					renderer.UpdateLayout();
-				}
+			if (renderer.ViewGroup.Parent != this)
+			{
+				if (renderer.ViewGroup.Parent != null)
+					renderer.ViewGroup.RemoveFromParent();
+				SetDefaultBackgroundColor(renderer);
+				AddView(renderer.ViewGroup);
+				renderer.UpdateLayout();
 			}
 		}
 
@@ -97,7 +104,7 @@ namespace Xamarin.Forms.Platform.Android
 				MasterDetailPageController.DetailBounds = bounds;
 
 			IVisualElementRenderer renderer = Platform.GetRenderer(_childView);
-			renderer.UpdateLayout();
+			renderer?.UpdateLayout();
 		}
 
 		void DisposeChildRenderers()
@@ -132,7 +139,7 @@ namespace Xamarin.Forms.Platform.Android
 			return new Rectangle(xPos, padding, width, height - padding);
 		}
 
-		void SetDefaultBackgroundColor(IVisualElementRenderer renderer)
+		protected void SetDefaultBackgroundColor(IVisualElementRenderer renderer)
 		{
 			if (ChildView.BackgroundColor == Color.Default)
 			{

Xamarin.Forms.Platform.Android/Xamarin.Forms.Platform.Android.csproj

@@ -104,6 +104,7 @@
     <Compile Include="AppCompat\FragmentContainer.cs" />
     <Compile Include="AppCompat\FrameRenderer.cs" />
     <Compile Include="AppCompat\IManageFragments.cs" />
+    <Compile Include="AppCompat\MasterDetailContainer.cs" />
     <Compile Include="AppCompat\Platform.cs" />
     <Compile Include="AppCompat\Resource.cs" />
     <Compile Include="CellAdapter.cs" />