Fix finding element index (#226)

* Fix finding element index
* Update ControlGallery to have repro case

samples/ControlGallery/ControlGallery/AppShell.razor

@@ -1,7 +1,7 @@
 @using ControlGallery.Views
 
-<Shell FlyoutBehavior="FlyoutBehavior.Disabled">
-    <TabBar>
+<Shell>
+    <FlyoutItem Title="Main">
         <Tab Title="Gallery">
             <ShellContent>
                 <PlaygroundList />
@@ -14,32 +14,53 @@
             <ContentPage Title="Adder">
                 <StackLayout>
                     <Button OnClick="AddTab">Add Tab</Button>
-                    <Button IsEnabled="(count > 1)" OnClick="RemoveTab">Remove tab</Button>
+                    <Button IsEnabled="(tabCount > 1)" OnClick="RemoveTab">Remove tab</Button>
+                    <Button OnClick="AddMenuItem">Add MenuItem</Button>
+                    <Button IsEnabled="(menuItemCount > 0)" OnClick="RemoveMenuItem">Remove MenuItem</Button>
                 </StackLayout>
             </ContentPage>
         </Tab>
 
-        @for (int i = 0; i < count; i++)
+        @for (int i = 0; i < tabCount; i++)
         {
             var currentIndex = i;
             <ShellTab Title="@($"Tab #{currentIndex + 1}")" />
         }
-    </TabBar>
+    </FlyoutItem>
+
+    @for (int i = 0; i < menuItemCount; i++)
+    {
+        <MenuItem Text="@($"MenuItem #{i + 1}")" />
+    }
 </Shell>
 
 @code {
-    int count = 1;
+    int tabCount = 1;
+    int menuItemCount = 2;
 
     void AddTab()
     {
-        count++;
+        tabCount++;
     }
 
     void RemoveTab()
     {
-        if (count > 1)
+        if (tabCount > 1)
+        {
+            tabCount--;
+        }
+    }
+
+    void AddMenuItem()
+    {
+        menuItemCount++;
+    }
+
+    void RemoveMenuItem()
+    {
+        if (menuItemCount > 0)
         {
-            count--;
+            menuItemCount--;
         }
     }
 }

src/BlinForms.Framework/BlinFormsElementManager.cs

@@ -29,9 +29,9 @@ protected override void AddChildElement(IWindowsFormsControlHandler parentHandle
             }
         }
 
-        protected override int GetPhysicalSiblingIndex(IWindowsFormsControlHandler handler)
+        protected override int GetChildElementIndex(IWindowsFormsControlHandler parentHandler, IWindowsFormsControlHandler childHandler)
         {
-            return handler.Control.Parent.Controls.GetChildIndex(handler.Control);
+            return parentHandler.Control.Controls.GetChildIndex(childHandler.Control);
         }
 
         protected override bool IsParented(IWindowsFormsControlHandler handler)

src/Microsoft.MobileBlazorBindings.Core/ElementManager.cs

@@ -12,7 +12,7 @@ namespace Microsoft.MobileBlazorBindings.Core
     public abstract class ElementManager
     {
         public abstract void AddChildElement(IElementHandler parentHandler, IElementHandler childHandler, int physicalSiblingIndex);
-        public abstract int GetPhysicalSiblingIndex(IElementHandler handler);
+        public abstract int GetChildElementIndex(IElementHandler parentHandler, IElementHandler childHandler);
         public abstract bool IsParented(IElementHandler handler);
         public abstract bool IsParentOfChild(IElementHandler parentHandler, IElementHandler childHandler);
         public abstract void RemoveChildElement(IElementHandler parentHandler, IElementHandler childHandler);

src/Microsoft.MobileBlazorBindings.Core/ElementManagerOfElementType.cs

@@ -25,9 +25,9 @@ public sealed override void AddChildElement(IElementHandler parentHandler, IElem
             AddChildElement(ConvertToType(parentHandler, nameof(parentHandler)), ConvertToType(childHandler, nameof(childHandler)), physicalSiblingIndex);
         }
 
-        public sealed override int GetPhysicalSiblingIndex(IElementHandler handler)
+        public sealed override int GetChildElementIndex(IElementHandler parentHandler, IElementHandler childHandler)
         {
-            return GetPhysicalSiblingIndex(ConvertToType(handler, nameof(handler)));
+            return GetChildElementIndex(ConvertToType(parentHandler, nameof(parentHandler)), ConvertToType(childHandler, nameof(childHandler)));
         }
 
         public sealed override bool IsParented(IElementHandler handler)
@@ -45,10 +45,10 @@ public sealed override void RemoveChildElement(IElementHandler parentHandler, IE
             RemoveChildElement(ConvertToType(parentHandler, nameof(parentHandler)), ConvertToType(childHandler, nameof(childHandler)));
         }
 
-        protected abstract void AddChildElement(TElementType elementType1, TElementType elementType2, int physicalSiblingIndex);
-        protected abstract int GetPhysicalSiblingIndex(TElementType elementType);
-        protected abstract bool IsParented(TElementType elementType);
-        protected abstract bool IsParentOfChild(TElementType elementType1, TElementType elementType2);
-        protected abstract void RemoveChildElement(TElementType elementType1, TElementType elementType2);
+        protected abstract void AddChildElement(TElementType parentHandler, TElementType childHandler, int physicalSiblingIndex);
+        protected abstract int GetChildElementIndex(TElementType parentHandler, TElementType childHandler);
+        protected abstract bool IsParented(TElementType handler);
+        protected abstract bool IsParentOfChild(TElementType parentHandler, TElementType childHandler);
+        protected abstract void RemoveChildElement(TElementType parentHandler, TElementType childHandler);
     }
 }

src/Microsoft.MobileBlazorBindings.Core/NativeComponentAdapter.cs

@@ -335,7 +335,7 @@ private int GetIndexForElement()
 
                     // If a native element was found somewhere within this sibling, the index for the new element
                     // will be 1 greater than its native index.
-                    return Renderer.ElementManager.GetPhysicalSiblingIndex(matchedEarlierSibling._targetElement) + 1;
+                    return Renderer.ElementManager.GetChildElementIndex(_closestPhysicalParent, matchedEarlierSibling._targetElement) + 1;
                 }
 
                 // If this level has a native element and all its relevant children have been scanned, then there's

src/Microsoft.MobileBlazorBindings/Elements/Handlers/BaseAttachedPropertiesHandler.cs

@@ -25,7 +25,6 @@ public void SetParent(object parentElement)
         // should be no-ops.
         public XF.Element ElementControl => null;
         public object TargetElement => null;
-        public int GetPhysicalSiblingIndex() => 0;
         public bool IsParented() => false;
         public bool IsParentedTo(XF.Element parent) => false;
 

src/Microsoft.MobileBlazorBindings/Elements/Handlers/ContentPageHandler.cs

@@ -13,6 +13,11 @@ public virtual void AddChild(XF.Element child, int physicalSiblingIndex)
             ContentPageControl.Content = childAsView;
         }
 
+        public int GetChildIndex(XF.Element child)
+        {
+            return ContentPageControl.Content == child ? 0 : -1;
+        }
+
         public virtual void RemoveChild(XF.Element child)
         {
             if (ContentPageControl.Content == child)

src/Microsoft.MobileBlazorBindings/Elements/Handlers/ElementHandler.cs

@@ -54,95 +54,6 @@ public virtual bool ApplyAdditionalAttribute(ulong attributeEventHandlerId, stri
             return false;
         }
 
-        public virtual int GetPhysicalSiblingIndex()
-        {
-            // TODO: What is the set of types that support child elements? Do they all need to be special-cased here? (Maybe...)
-            var nativeComponent = ElementControl;
-
-            if (nativeComponent is null)
-            {
-                // If there is no native object representing this element, bail out
-                // TODO: Is this OK? It's probably OK for the same reason the GridCellHandler case above is OK. If this item has
-                // no physical representation in the live UI tree, there's nothing to track.
-                return 0;
-            }
-
-            if (nativeComponent.Parent is null)
-            {
-                // If this is the root element, the child's index is always 0
-                return 0;
-            }
-
-            switch (nativeComponent.Parent)
-            {
-                case XF.Layout<XF.View> parentAsLayout:
-                    {
-                        var childAsView = nativeComponent as XF.View;
-                        return parentAsLayout.Children.IndexOf(childAsView);
-                    }
-                case XF.ContentView _:
-                    {
-                        // A ContentView can have only 1 child, so the child's index is always 0.
-                        return 0;
-                    }
-                case XF.ContentPage _:
-                    {
-                        // A ContentPage can have only 1 child, so the child's index is always 0.
-                        return 0;
-                    }
-                case XF.TabbedPage parentAsTabbedPage:
-                    {
-                        var childAsPage = nativeComponent as XF.Page;
-                        return parentAsTabbedPage.Children.IndexOf(childAsPage);
-                    }
-                case XF.ScrollView _:
-                    {
-                        // A ScrollView can have only 1 child, so the child's index is always 0.
-                        return 0;
-                    }
-                case XF.Label parentAsLabel:
-                    {
-                        // There are two cases to consider:
-                        // 1. A Xamarin.Forms Label can have only 1 child (a FormattedString), so the child's index is always 0.
-                        // 2. But to simplify things, in MobileBlazorBindings a Label can contain a Span directly, so if the child
-                        //    is a Span, we have to compute its sibling index.
-                        if (nativeComponent is XF.Span childAsSpan)
-                        {
-                            return parentAsLabel.FormattedText?.Spans.IndexOf(childAsSpan) ?? 0;
-                        }
-
-                        return 0;
-                    }
-                case XF.FormattedString parentAsFormattedString:
-                    {
-                        var childAsSpan = nativeComponent as XF.Span;
-                        return parentAsFormattedString.Spans.IndexOf(childAsSpan);
-                    }
-                case XF.ShellSection parentAsShellSection:
-                    {
-                        var childAsShellContent = nativeComponent as XF.ShellContent;
-                        return parentAsShellSection.Items.IndexOf(childAsShellContent);
-                    }
-                case XF.ShellItem parentAsShellItem:
-                    {
-                        var childAsShellSection = nativeComponent as XF.ShellSection;
-                        return parentAsShellItem.Items.IndexOf(childAsShellSection);
-                    }
-                case XF.Shell parentAsShell:
-                    {
-                        var childAsShellItem = nativeComponent as XF.ShellItem;
-                        return parentAsShell.Items.IndexOf(childAsShellItem);
-                    }
-                case XF.Application _:
-                    {
-                        // An Application can have only 1 child (its MainPage), so the child's index is always 0.
-                        return 0;
-                    }
-                default:
-                    throw new InvalidOperationException($"Don't know how to handle parent element type {nativeComponent.Parent.GetType().FullName} in order to get index of sibling {nativeComponent.GetType().FullName}");
-            }
-        }
-
         public virtual bool IsParented()
         {
             return ElementControl.Parent != null;

src/Microsoft.MobileBlazorBindings/Elements/Handlers/GestureRecognizerHandler.cs

@@ -40,13 +40,6 @@ public virtual void ApplyAttribute(ulong attributeEventHandlerId, string attribu
             }
         }
 
-        public int GetPhysicalSiblingIndex()
-        {
-            // Because this is a 'fake' element, all matters related to physical trees
-            // should be no-ops.
-            return 0;
-        }
-
         public bool IsParented()
         {
             // Because this is a 'fake' element, all matters related to physical trees

src/Microsoft.MobileBlazorBindings/Elements/Handlers/GridCellHandler.cs

@@ -61,7 +61,7 @@ public void RemoveChild(XF.Element child)
             throw new NotImplementedException();
         }
 
-        public int GetPhysicalSiblingIndex()
+        public int GetChildIndex(XF.Element child)
         {
             // Because this is a 'fake' element, all matters related to physical trees
             // should be no-ops.

src/Microsoft.MobileBlazorBindings/Elements/Handlers/LabelHandler.cs

@@ -24,6 +24,21 @@ public virtual void AddChild(XF.Element child, int physicalSiblingIndex)
             }
         }
 
+        public int GetChildIndex(XF.Element child)
+        {
+            // There are two cases to consider:
+            // 1. A Xamarin.Forms Label can have only 1 child (a FormattedString), so the child's index is always 0.
+            // 2. But to simplify things, in MobileBlazorBindings a Label can contain a Span directly, so if the child
+            //    is a Span, we have to compute its sibling index.
+
+            return child switch
+            {
+                XF.Span span => LabelControl.FormattedText?.Spans.IndexOf(span) ?? -1,
+                XF.FormattedString formattedString when LabelControl.FormattedText == formattedString => 0,
+                _ => -1
+            };
+        }
+
         public virtual void RemoveChild(XF.Element child)
         {
             var childAsSpan = child as XF.Span;

src/Microsoft.MobileBlazorBindings/Elements/Handlers/LayoutHandler.cs

@@ -25,6 +25,14 @@ public virtual void AddChild(XF.Element child, int physicalSiblingIndex)
             }
         }
 
+        public int GetChildIndex(XF.Element child)
+        {
+            var layoutControlOfView = (XF.Layout<XF.View>)LayoutControl;
+            var childAsView = child as XF.View;
+
+            return layoutControlOfView.Children.IndexOf(childAsView);
+        }
+
         public virtual void RemoveChild(XF.Element child)
         {
             var layoutControlOfView = (XF.Layout<XF.View>)LayoutControl;

src/Microsoft.MobileBlazorBindings/Elements/Handlers/MasterDetailPageHandler.cs

@@ -67,5 +67,16 @@ public virtual void RemoveChild(XF.Element child)
                 throw new InvalidOperationException($"Unknown child type {child.GetType().FullName} being removed from parent element type {GetType().FullName}.");
             }
         }
+
+        public int GetChildIndex(XF.Element child)
+        {
+            // Not sure whether elements "order" matters here
+            return child switch
+            {
+                _ when child == MasterDetailPageControl.Master => 0,
+                _ when child == MasterDetailPageControl.Detail => 1,
+                _ => -1
+            };
+        }
     }
 }

src/Microsoft.MobileBlazorBindings/Elements/Handlers/ModalContainerHandler.cs

@@ -106,7 +106,7 @@ public void SetParent(XF.Element parent)
             throw new NotSupportedException();
         }
 
-        public int GetPhysicalSiblingIndex()
+        public int GetChildIndex(XF.Element child)
         {
             // Because this is a 'fake' element, all matters related to physical trees
             // should be no-ops.

src/Microsoft.MobileBlazorBindings/Elements/Handlers/ShellContentHandler.cs

@@ -22,6 +22,11 @@ public virtual void RemoveChild(XF.Element child)
             }
         }
 
+        public int GetChildIndex(XF.Element child)
+        {
+            return child == ShellContentControl.Content ? 0 : -1;
+        }
+
         public override void SetParent(XF.Element parent)
         {
             if (ElementControl.Parent == null)

src/Microsoft.MobileBlazorBindings/Elements/Handlers/ShellFlyoutHeaderHandler.cs

@@ -44,7 +44,7 @@ public void RemoveChild(XF.Element child)
             throw new NotImplementedException();
         }
 
-        public int GetPhysicalSiblingIndex()
+        public int GetChildIndex(XF.Element child)
         {
             // Because this is a 'fake' element, all matters related to physical trees
             // should be no-ops.

src/Microsoft.MobileBlazorBindings/Elements/Handlers/ShellHandler.cs

@@ -84,17 +84,29 @@ public virtual void RemoveChild(XF.Element child)
                 throw new ArgumentNullException(nameof(child));
             }
 
-            var itemToRemove = child switch
+            var itemToRemove = GetItemForElement(child)
+                ?? throw new NotSupportedException($"Handler of type '{GetType().FullName}' representing element type '{TargetElement?.GetType().FullName ?? "<null>"}' doesn't support removing a child (child type is '{child.GetType().FullName}').");
+
+            ShellControl.Items.Remove(itemToRemove);
+        }
+
+        public int GetChildIndex(XF.Element child)
+        {
+            var shellItem = GetItemForElement(child);
+            return ShellControl.Items.IndexOf(shellItem);
+        }
+
+        private XF.ShellItem GetItemForElement(XF.Element child)
+        {
+            return child switch
             {
                 XF.TemplatedPage childAsTemplatedPage => GetItemForTemplatedPage(childAsTemplatedPage),
                 XF.ShellContent childAsShellContent => GetItemForContent(childAsShellContent),
                 XF.ShellSection childAsShellSection => GetItemForSection(childAsShellSection),
                 XF.MenuItem childAsMenuItem => GetItemForMenuItem(childAsMenuItem),
                 XF.ShellItem childAsShellItem => childAsShellItem,
-                _ => throw new NotSupportedException($"Handler of type '{GetType().FullName}' representing element type '{TargetElement?.GetType().FullName ?? "<null>"}' doesn't support removing a child (child type is '{child.GetType().FullName}').")
+                _ => null
             };
-
-            ShellControl.Items.Remove(itemToRemove);
         }
 
         private XF.ShellItem GetItemForTemplatedPage(XF.TemplatedPage childAsTemplatedPage)