Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WinUI: TabbedPage pages provided by a DataTemplate crash when swapping to a different tab. #14572

Open
Keflon opened this issue Apr 13, 2023 · 11 comments
Labels
area/layout 🔲 StackLayout, GridLayout, ScrollView, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter area/TabbedPage TabbedPage platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Milestone

Comments

@Keflon
Copy link

Keflon commented Apr 13, 2023

Description

When a TabbedPage is used as the root application page and the child pages are provided using ItemsSource, the app crashes when a tab other than the current tab is pressed.
image

Steps to Reproduce

  1. Create a new MAUI app.
  2. Replace App.xaml.cs with this:
public partial class App : Application
{
    public App()
    {
        InitializeComponent();

        var rootPage = new TabbedPage();
        rootPage.ItemsSource = new List<object> { new Vm1(), new Vm2(), new Vm3() };
        rootPage.ItemTemplate = new TabbedItemTemplateSelector();
        MainPage = rootPage;
    }
    public class Vm1 { }
    public class Vm2 { }
    public class Vm3 { }
    class TabbedItemTemplateSelector : DataTemplateSelector
    {
        protected override DataTemplate OnSelectTemplate(object item, BindableObject container)
        {
            if (item is Vm1)
                return new DataTemplate(() => new ContentPage() { Title = "Type 1" });
            if (item is Vm2)
                return new DataTemplate(() => new ContentPage() { Title = "Type 2" });
            if (item is Vm3)
                return new DataTemplate(() => new ContentPage() { Title = "Type 3" });

            return null;
        }
    }
}
  1. Run in on Windows and tap for a crash.

Link to public reproduction project repository

https://github.com/Keflon/TabbedPageTemplateBug

Version with bug

7.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

Latest

Did you find any workaround?

No. If anyone finds one please share here.

Relevant log output

Exception thrown: 'System.ArgumentException' in WinRT.Runtime.dll
WinRT information: The parameter is incorrect.
@Keflon Keflon added the t/bug Something isn't working label Apr 13, 2023
@Keflon
Copy link
Author

Keflon commented Apr 13, 2023

Clues:
It crashes whether all items are of the same type or not.
It crashes whether a DataTemplate or a DataTemplateSelector is used.
The SelectedItem can be successfully changed to e.g. Vm2 prior to presentation, though the app still crashes when that selection changes.

@Keflon
Copy link
Author

Keflon commented Apr 13, 2023

I made a workaround. To help manage expectations please understand it's hot off the press, but I've hammered the ItemsSource via a timer and it all seems fine on WinUI, iOS and Droid.
Use it when you want to use ItemsSource and ItemTemplate. Stick with TabbedPage if you're manipulating the Children collection directly.
It also fixes the bad rendering of the tabs on Windows. (Something I didn't notice for the original report)
If ItemsSource is set to something supporting ICollectionChanged it will track changes.

using System.Collections;
using System.Collections.Specialized;
using System.Runtime.CompilerServices;

namespace FunctionZero.Maui.MvvmZero.Workaround
{
    /// <summary>
    /// An alternative for TabbedPage for people that run into this bug: https://github.com/dotnet/maui/issues/14572
    /// </summary>
    public class AdaptedTabbedPage : TabbedPage
    {
        protected override void OnPropertyChanged([CallerMemberName] string propertyName = null)
        {
            base.OnPropertyChanged(propertyName);

            // If the template changes, use it to rebuild the pages.
            if (propertyName == nameof(ItemTemplate))
                RebuildPages();
        }
        private void RebuildPages()
        {
            Children.Clear();

            if (ItemsSource != null)
                foreach (var item in ItemsSource)
                {
                    var page = GetPageForItem(item);
                    if (page != null)
                        Children.Add(page);
                }
        }

        /// <summary>
        /// For a given item (i.e. a ViewModel) use the ItemTemplate to get the Page,
        /// and set the BindingContext.
        /// </summary>
        /// <param name="item">the item to give to the ItemTemplate</param>
        /// <returns>A page associated with the item.</returns>
        private Page GetPageForItem(object item)
        {
            Page retval = null;
            if (ItemTemplate != null)
            {
                if (this.ItemTemplate is DataTemplateSelector selector)
                    retval = (Page)selector.SelectTemplate(item, this).CreateContent();
                else
                    retval = (Page)this.ItemTemplate?.CreateContent();

                retval.BindingContext = item;
            }
            return retval;
        }

        /// <summary>
        /// ATTENTION: Hiding base implementation!
        /// </summary>
        public static new readonly BindableProperty ItemsSourceProperty = BindableProperty.Create(nameof(ItemsSource), typeof(IEnumerable), typeof(AdaptedTabbedPage), null, BindingMode.OneWay, null, ItemsSourcePropertyChanged);

        /// <summary>
        /// ATTENTION: Hiding base implementation!
        /// </summary>
        public new IEnumerable ItemsSource
        {
            get { return (IEnumerable)GetValue(ItemsSourceProperty); }
            set { SetValue(ItemsSourceProperty, value); }
        }

        /// <summary>
        /// If the ItemsSource changes ...
        /// 1. Remove any existing Pages.
        /// 2. Detach from the previous ItemsSource if it was INotifyCollectionChanged.
        /// 2. Regenerate new Pages for any new items.
        /// 3. Attach to the previous ItemsSource if it was INotifyCollectionChanged.
        /// </summary>
        private static void ItemsSourcePropertyChanged(BindableObject bindable, object oldvalue, object newvalue)
        {
            var self = (AdaptedTabbedPage)bindable;

            self.Children.Clear();

            if (oldvalue is INotifyCollectionChanged oldCollection)
                oldCollection.CollectionChanged -= self.ItemsSourceCollectionChanged;

            if (newvalue is IEnumerable newCollection)
            {
                self.RebuildPages();
                if (newvalue is INotifyCollectionChanged newObservable)
                    newObservable.CollectionChanged += self.ItemsSourceCollectionChanged;
            }
        }

        private void ItemsSourceCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
        {
            switch (e.Action)
            {
                case NotifyCollectionChangedAction.Add:
                    var insertIndex = e.NewStartingIndex;
                    foreach (var newItem in e.NewItems)
                    {
                        var page = GetPageForItem(newItem);
                        this.Children.Insert(insertIndex++, page);
                    }
                    break;
                case NotifyCollectionChangedAction.Remove:
                    for (int c = 0; c < e.OldItems.Count; c++)
                    {
                        this.Children.RemoveAt(e.OldStartingIndex);
                    }
                    break;
                case NotifyCollectionChangedAction.Replace:
                    throw new NotImplementedException("Handle if necessary!");

                case NotifyCollectionChangedAction.Move:
                    throw new NotImplementedException("Handle if necessary!");

                case NotifyCollectionChangedAction.Reset:
                    this.Children.Clear();
                    break;
            }
        }
    }
}

CAUTION
The implementation replaces ItemsSource by hiding the base implementation.
This means if you set it up in code-behind, you must ensure you have a reference of type AdaptedTabbedPage when you set ItemsSource.
If your reference is of type TabbedPage or MultiPage<Page> you'll be setting the base ItemsSource and the crash will remain.

@jsuarezruiz
Copy link
Contributor

Stacktrace:

   at WinRT.ExceptionHelpers.<ThrowExceptionForHR>g__Throw|20_0(Int32 hr)
   at ABI.Microsoft.UI.Xaml.IFrameworkElementOverridesMethods.MeasureOverride(IObjectReference _obj, Size availableSize)
   at Microsoft.UI.Xaml.FrameworkElement.MeasureOverride(Size availableSize)
   at Microsoft.UI.Xaml.FrameworkElement.Microsoft.UI.Xaml.IFrameworkElementOverrides.MeasureOverride(Size availableSize)
   at ABI.Microsoft.UI.Xaml.IFrameworkElementOverrides.Do_Abi_MeasureOverride_0(IntPtr thisPtr, Size availableSize, Size* result)

@Keflon
Copy link
Author

Keflon commented Apr 14, 2023

It seems referencing this bug in a commit message in one of my public libraries has closed this bug. That wasn’t my intention so I’ve reopened it.

@jsuarezruiz jsuarezruiz added the area/layout 🔲 StackLayout, GridLayout, ScrollView, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Apr 17, 2023
@jsuarezruiz jsuarezruiz added this to the Backlog milestone Apr 17, 2023
@zerihal
Copy link

zerihal commented Apr 19, 2023

Could not get past this issue with using datatemplate and above workaround is not suitable for my purposes.

Alternate workaround, which did seem to work OK on all platforms was to get rid of the datatemplate all together and just dynamically add the contents of of what was previously ItemsSource to the TabbedPage children in the constructor (retaining the same view model). Not an ideal solution I know, but works for now.

@Keflon
Copy link
Author

Keflon commented Apr 20, 2023

I've enhanced the above workaround to support setting SelectedItem from code, and published a NuGet package here.
Source code here.
I hope this is useful to someone.
Setting SelectedItem from the viewModel is fine, but setting it here isn't. Maybe that's a clue? (see the comment in the else if)

public class AdaptedTabbedPage : TabbedPage
{
    public AdaptedTabbedPage()
    {
        this.PropertyChanged += AdaptedTabbedPage_PropertyChanged;
    }

    private void AdaptedTabbedPage_PropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        if (e.PropertyName == nameof(SelectedItem))
        {
            if (UseExperimentalSelectedItem)
                if (Children != null)
                    foreach (var page in Children)
                        if (page.BindingContext == SelectedItem)
                        {
                            // Works fine.
                            CurrentPage = page;
                            break;
                        }
        }
        else if (e.PropertyName == nameof(CurrentPage))
        {
            // This causes a crash on WinUI. Android and iOS are fine.
            //if (UseExperimentalSelectedItem)
            //    SelectedItem = CurrentPage?.BindingContext;
        }
    }

@XamlTest XamlTest added s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels Jun 28, 2023
@XamlTest
Copy link
Collaborator

Verified this on Visual Studio Enterprise 17.7.0 Preview 2.0. Repro on Windows 11 with below Project:
TabbedPageTemplateBug.zip

image

@mcanyucel
Copy link

mcanyucel commented Oct 17, 2023

This crash is still happening:

<?xml version="1.0" encoding="utf-8" ?>
<TabbedPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
             xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
            xmlns:local="clr-namespace:SMMS.Views.Start.InfoPages"
            x:DataType="x:String"
             x:Class="SMMS.Views.Start.InformationPage">

    <TabbedPage.ItemTemplate>
        <DataTemplate>
            <ContentPage Title="{Binding}"  >
                <StackLayout>
                    <Label Text="{Binding}" />
                </StackLayout>
            </ContentPage>
        </DataTemplate>
    </TabbedPage.ItemTemplate>
    
    

</TabbedPage>
public partial class InformationPage : TabbedPage
{
	public InformationPage()
	{
		InitializeComponent();
        ItemsSource = new List<String> { "A", "B", "C" };

    }
}

Exception Message: The parameter is incorrect.

@MitchBomcanhao
Copy link

MitchBomcanhao commented Nov 15, 2023

yet another blocker for our xamarin forms app conversion. seems like most of our UI on Windows is broken and doesn't display or just crashes due to issues like this one.

@Keflon
Copy link
Author

Keflon commented Nov 23, 2023

Hi @MitchBomcanhao
There are blockers, but this issue needn't be. The AdaptedTabbedPage found in this controls library is a workaround that is working fine and can be easily swapped out once this bug is fixed.

package here.
Source code here.

If you don't want a dependency on the library you can just lift AdaptedTabbedPage.cs

@bitzeal-johan
Copy link

How come nobody is assigned to this bug? It still happens and I've wasted days on trying to find a problem in my code until I realzed it's (another) issue with MAUI. Please stop shipping cool new features until you've fixed the bugs in those already released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/layout 🔲 StackLayout, GridLayout, ScrollView, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter area/TabbedPage TabbedPage platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants