CommunityToolkit / Maui

The .NET MAUI Community Toolkit is a community-created library that contains .NET MAUI Extensions, Advanced UI/UX Controls, and Behaviors to help make your life as a .NET MAUI developer easier
https://learn.microsoft.com/dotnet/communitytoolkit/maui
MIT License
2.28k stars 403 forks source link

[BUG] iOS screen mirroring with popup let the app crash on opening the popup #1158

Open imuller opened 1 year ago

imuller commented 1 year ago

Is there an existing issue for this?

Did you read the "Reporting a bug" section on Contributing file?

Current Behavior

When opening a Popup when screen mirroring is active. The app crashes with a System.InvalidOperationException.

System.InvalidOperationException: ViewController cannot be null. at CommunityToolkit.Maui.Core.Views.MauiPopup.SetElement(IPopup element) in /_/src/CommunityToolkit.Maui.Core/Views/Popup/MauiPopup.macios.cs:line 80 at CommunityToolkit.Maui.Core.Handlers.PopupHandler.ConnectHandler(MauiPopup platformView) in /_/src/CommunityToolkit.Maui.Core/Handlers/Popup/PopupHandler.macios.cs:line 91 at Microsoft.Maui.Handlers.ElementHandler2[[CommunityToolkit.Maui.Core.IPopup, CommunityToolkit.Maui.Core, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null],[CommunityToolkit.Maui.Core.Views.MauiPopup, CommunityToolkit.Maui.Core, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].OnConnectHandler(Object platformView) at Microsoft.Maui.Handlers.ElementHandler.ConnectHandler(Object platformView) at Microsoft.Maui.Handlers.ElementHandler.SetVirtualView(IElement view) at Microsoft.Maui.Controls.Element.SetHandler(IElementHandler newHandler) at Microsoft.Maui.Controls.Element.setHandler(IElementHandler value) at Microsoft.Maui.Platform.ElementExtensions.ToHandler(IElement view, IMauiContext context) at CommunityToolkit.Maui.Views.PopupExtensions.CreatePopup(Page page, Popup popup) in //src/CommunityToolkit.Maui/Views/Popup/PopupExtensions.shared.cs:line 89 at CommunityToolkit.Maui.Views.PopupExtensions.CreateAndShowPopupAsync[ToolkitPopupPage](Page page, ToolkitPopupPage popup) in //src/CommunityToolkit.Maui/Views/Popup/PopupExtensions.shared.cs:line 113 at CommunityToolkit.Maui.Views.PopupExtensions.ShowPopupAsync[ToolkitPopupPage](Page page, ToolkitPopupPage popup) in //src/CommunityToolkit.Maui/Views/Popup/PopupExtensions.shared.cs:line 57 at MauiApp1.MainPage.OnOpenToolkitPopupPage(Object sender, EventArgs e) in /Users/user/RiderProjects/MauiApp1/MauiApp1/MainPage.xaml.cs:line 24 at System.Threading.Tasks.Task.<>c.b__128_0(Object ) at Foundation.NSAsyncSynchronizationContextDispatcher.Apply() at UIKit.UIApplication.UIApplicationMain(Int32 , String[] , IntPtr , IntPtr ) at UIKit.UIApplication.Main(String[] , Type , Type ) at MauiApp1.Program.Main(String[] args) in /Users/user/RiderProjects/MauiApp1/MauiApp1/Platforms/iOS/Program.cs:line 13`

Expected Behavior

The popup should behave the same as on the device. This is very annoying when demo MAUI apps when the app crashes.

Steps To Reproduce

  1. Create a Popup in you're application
  2. Add a button that opens the popup
  3. Start screen mirroring on the device (i've tested it on an iPad with iOS 16.4)
  4. Open the app
  5. Click the button that opens the popup
  6. The app crashes...

Link to public reproduction project repository

https://github.com/imuller/app-popups-problem

Environment

- .NET MAUI CommunityToolkit: 5.1.0
- OS: iOS 16.4 running on and iPad Pro
- .NET MAUI: 7.0.81

Anything else?

No response

ghost commented 1 year ago

Hi @imuller. We have added the "needs reproduction" label to this issue, which indicates that we cannot take further action. This issue will be closed automatically in 5 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

imuller commented 1 year ago

I added a project: https://github.com/imuller/app-popups-problem

pbhatt-nucor commented 12 months ago

Any updates on this thread? I have faced the same issue with iPhone 16 (OS version 17.1.1) & screen mirroring. I'm using .Net MAUI 7.0

ViewController cannot be null. CommunityToolkit.Maui.Core.Views.MauiPopup.SetElement(IPopup element) CommunityToolkit.Maui.Core.Handlers.PopupHandler.ConnectHandler(MauiPopup platformView) Microsoft.Maui.Handlers.ElementHandler`2[[CommunityToolkit.Maui.Core.IPopup, CommunityToolkit.Maui.Core, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null],[CommunityToolkit.Maui.Core.Views.MauiPopup, CommunityToolkit.Maui.Core, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].OnConnectHandler(Object platformView) Microsoft.Maui.Handlers.ElementHandler.SetVirtualView(IElement view) Microsoft.Maui.Controls.Element.SetHandler(IElementHandler newHandler) Microsoft.Maui.Controls.Element.set_Handler(IElementHandler value)

softlion commented 9 months ago

That's because the code in Platform.GetCurrentUIViewController for iOS is too naive. It's not an issue with communitytoolkit, but with maui.

Most of the time, this is caused by a popup/dialog/alert view trying to display over an existing popup/dialog/alert view (or a not yet disappeared top level window because disappearing is animated).

So updating your own code to make sure that does not happen is the best bet.

The good new is, you can override the buggy implementation of IWindowStateManager.

The current maui implementation is here:

public UIViewController? GetCurrentUIViewController()
{
    var viewController = getCurrentController?.Invoke();

    if (viewController != null)
        return viewController;

    var window = GetKeyWindow();

    if (window != null && window.WindowLevel == UIWindowLevel.Normal)
        viewController = window.RootViewController;

    if (viewController == null)
    {
        window = GetWindows()?
            .OrderByDescending(w => w.WindowLevel)
            .FirstOrDefault(w => w.RootViewController != null && w.WindowLevel == UIWindowLevel.Normal);

        viewController = window?.RootViewController;
    }

    while (viewController?.PresentedViewController != null)
        viewController = viewController.PresentedViewController;

    return viewController;
}

The bad news is, WindowStateManager.SetDefault() is internal. And also the fixed implementation has still to be provided.

If a contributor wants to check, he/she can use the windows store app "AirPlay Screen Mirroring Receiver" as an airplay receiver for screen mirrorring, to test that implementation.

zeldafreak commented 8 months ago

I think I was able to work around this issue thanks to @softlion and this PR over here: https://github.com/rotorgames/Rg.Plugins.Popup/pull/754

While we can't provide our own implementation of IWindowStateManager that properly selects the correct UIWindow and UIViewController, it seems you can use WindowStateManager.Default.Init and pass in a delegate for GetCurrentUIViewController that will be favored over the default implementation.

My personal approach was to sort the UIWindowScene's by their UIWindowSceneSessionRole:

using var scenes = UIApplication.SharedApplication.ConnectedScenes;
var windowScene = scenes.ToArray<UIWindowScene>()
                        .MinBy(scene => (int)scene.Session.Role); // favor "Application" over other roles.
return windowScene?.Windows.FirstOrDefault();

This works because Application has a value of 0, so all the others, like ExternalDisplay (which is the role of the AirPlay scene) will always come after it.

I have no idea if I will run into some other kind of issue in the future by doing this-- I'm not an iOS developer. But if I'm lucky I won't be humiliated by intermittent crashes when demonstrating progress on my project like I was this morning.

Thank you so much, @softlion and @EmilienDup !

softlion commented 8 months ago

@zeldafreak A better way would be to get the MainPage from maui, and get its window instead. Why try to guess something when we already have it. no ?

zeldafreak commented 8 months ago

@softlion I admit I don't fully understand the ramifications of changing how it works now, so I erred on the side of making as few changes as possible. Leaving all the existing logic in place and merely adjusting how it handles the case where there are multiple scenes seemed like the safest solution at the time. But to your point, this is supposed to get the "root" view controller, and so going directly to the MainPage's Window does seem appropriate.

Are you suggesting:

private static UIWindow? GetKeyWindow()
{
    // Use the MainPage's Window as the key Window if it exists.
    if (Microsoft.Maui.Controls.Application.Current?.MainPage?.Window?.Handler?.PlatformView is UIWindow window)
    {
        return window;
    }

    // if we have scene support, use that
    if (OperatingSystem.IsIOSVersionAtLeast(13) || OperatingSystem.IsMacCatalystVersionAtLeast(13))
    {
        try
        {
            using var scenes = UIApplication.SharedApplication.ConnectedScenes;
            var windowScene = scenes.ToArray<UIWindowScene>()
                                    .MinBy(scene => (int)scene.Session.Role); // favor "Application" over other roles.
            return windowScene?.Windows.FirstOrDefault();
        }
        catch (InvalidCastException)
        {
            // HACK: Workaround for https://github.com/xamarin/xamarin-macios/issues/13704
            //       This only throws if the collection is empty.
            return null;
        }
    }

    // use the windows property (up to 13.0)
    return UIApplication.SharedApplication.KeyWindow;
}

... which will still fall back to looking at all the scenes for an appropriate window if it can't find one, but tries first to get MAUI's application window...

devinduplessis commented 7 months ago

@zeldafreak I know this has been a while but this issue is plagueing my app as well. How could I ovveride the default implementation. By injectecting my own WindowsStateManager with the changes or some other way?

zeldafreak commented 7 months ago

@devinduplessis I'll put my code below for your sake and any others who suffer the same issue. But first I want to express frustration that it took less than 2 weeks for DevExpress to have a hotfix for the same issue. By using a decompiler, I know they added a single expression, a ~20 character change, and now all of their controls are immune to this problem. Meanwhile, this issue has been ignored for a year by the MAUI team. (EDIT: To clarify, this is not a Toolkit bug, it's a MAUI bug.)

Here's the entirety of my /Platforms/iOS/AppDelegate.cs file. I copy/pasted almost all of this from MAUI's WindowStateManagerImplementation and then made as few changes as possible. But if you return null, it'll fall back on the default behavior, so where this code is identical to WindowStateManagerImplementation, I could've returned null rather than duplicate MAUI/Xamarin's behavior. I apologize for how unnecessarily long this is, but I've been using it for over a month now and it's never crashed on me, so I'm giving it to you as-is.

[Register("AppDelegate")]
public class AppDelegate : MauiUIApplicationDelegate
{
    protected override MauiApp CreateMauiApp()
    {
        var app = MauiProgram.CreateMauiApp();

        // Give WindowStateManager a default implementation for retrieving the UIViewController.
        // If specified, it will try this function first. The default behavior will be used if it returns null.
        WindowStateManager.Default.Init(GetCurrentUIViewController);

        return app;
    }

    private static UIViewController? GetCurrentUIViewController()
    {
        var window = GetKeyWindow();

        UIViewController? viewController = null;
        if (window != null && window.WindowLevel == UIWindowLevel.Normal)
            viewController = window.RootViewController;

        if (viewController == null)
        {
            window = GetWindows()?
                     .OrderByDescending(w => w.WindowLevel)
                     .FirstOrDefault(w => w.RootViewController != null && w.WindowLevel == UIWindowLevel.Normal);

            viewController = window?.RootViewController;
        }

        while (viewController?.PresentedViewController != null)
            viewController = viewController.PresentedViewController;

        return viewController;
    }

    private static UIWindow? GetKeyWindow()
    {
        // Prefer the MainPage window, per @softlion: https://github.com/CommunityToolkit/Maui/issues/1158#issuecomment-2002395273
        if (Microsoft.Maui.Controls.Application.Current?.MainPage?.Window?.Handler?.PlatformView is UIWindow window)
        {
            return window;
        }

        // if we have scene support, use that
        if (OperatingSystem.IsIOSVersionAtLeast(13) || OperatingSystem.IsMacCatalystVersionAtLeast(13))
        {
            try
            {
                using var scenes = UIApplication.SharedApplication.ConnectedScenes;
                var windowScene = scenes.ToArray<UIWindowScene>()
                                        .MinBy(scene => (int)scene.Session.Role); // favor "Application" over other roles.
                return windowScene?.Windows.FirstOrDefault();
            }
            catch (InvalidCastException)
            {
                // HACK: Workaround for https://github.com/xamarin/xamarin-macios/issues/13704
                //       This only throws if the collection is empty.
                return null;
            }
        }

        // use the windows property (up to 13.0)
        return UIApplication.SharedApplication.KeyWindow;
    }

    private static UIWindow[]? GetWindows()
    {
        // if we have scene support, use that
        if (OperatingSystem.IsIOSVersionAtLeast(13) || OperatingSystem.IsMacCatalystVersionAtLeast(13))
        {
            try
            {
                using var scenes = UIApplication.SharedApplication.ConnectedScenes;
                var windowScene = scenes.ToArray<UIWindowScene>()
                                         .MinBy(scene => (int)scene.Session.Role); // favor "Application" over other roles.
                return windowScene?.Windows;
            }
            catch (InvalidCastException)
            {
                // HACK: Workaround for https://github.com/xamarin/xamarin-macios/issues/13704
                //       This only throws if the collection is empty.
                return null;
            }
        }

        // use the windows property (up to 15.0)
        return UIApplication.SharedApplication.Windows;
    }
}

All credit for this workaround should go to @softlion and @EmilienDup. I may never have gotten this working (nor been able to advise DevExpress on how to fix their related bug) without their guidance/example. (but also, this isn't their code, so any bugs in it are entirely my fault 🙂)

brminnick commented 7 months ago

But first I want to express frustration that it took less than 2 weeks for DevExpress to have a hotfix for the same issue. By using a decompiler, I know they added a single expression, a ~20 character change, and now all of their controls are immune to this problem. Meanwhile, this issue has been ignored for a year by the MAUI/Toolkit teams.

@zeldafreak I hope you understand that every maintainer on the Community Toolkit team is a volunteer. None of us get paid for the hundreds of hours we have contributed to this project. We devote our spare time on our nights and weekends to help fellow .NET MAUI developers.

We are community members just like you. Everyone in the community, including you, is equally capable of submitting PRs, fixing bugs and creating new features.

We aren't ignoring Issues. We aren't ignoring Pull Requests. Our time is limited to the hours we're willing to dedicate away from our families to help create this library to benefit developers like you.

I encourage you to submit a Pull Request with your fix.

devinduplessis commented 7 months ago

@zeldafreak Thank you so much for this. My team and I are demoing an application saturday. We never ran into the issue during our testing until presenting to a colleague by mirroring the ipad screen onto a tv. We really appreciate your quick response. Once again, thank you, @softlion, and @EmilienDup

zeldafreak commented 7 months ago

@brminnick It sounds like I was under the mistaken impression that Microsoft supported this project. I didn't think they'd completely defer important features (like popups!) or bugfixes (like this one!) to a community working for free. I apologize, and was in the wrong to mention the Toolkit team in my post. I will remove it.

zeldafreak commented 7 months ago

Also, @brminnick, is it actually possible to fix this with a Community Toolkit PR? It'd have to involve using a workaround like my snippet above. I assumed this would really need to be a MAUI change, which has more time-consuming requirements for both reporting issues and making PRs to fix them. But I can think of a few ways to allow users to opt-in to this workaround more easily, which isn't the ideal solution but it might help some people out. The main issue I see with this is that if .NET MAUI ever does get around to addressing this, it could break whatever workaround I put in place.

bijington commented 7 months ago

I'm intrigued, if it's a MAUI issue how have DevExpress solved the issue? They provide a library just like the toolkit. Does anyone know someone at DevExpress that might be willing to give any insight?

zeldafreak commented 7 months ago

I used a decompiler to figure out how to fix their issue, I've put a description in the ticket I gave to DevExpress for more details.

But the short version is, they don't use WindowStateManager at all, they have their own utility class to get the view controller

zeldafreak commented 7 months ago

For reference in case that ticket is inconvenient, DevExpress has a CustomViewExtensions class that closely resembles WindowStateManager (this is from dotPeek so it's ugly, but clearly it shares ancestry with WindowStateManager):

  internal static class CustomViewExtensions
  {
    internal static UIWindow FirstKeyWindowOrDefault()
    {
      UIWindowScene uiWindowScene = (UIWindowScene) ((IEnumerable<UIScene>) UIApplication.SharedApplication.ConnectedScenes.ToArray()).Where<UIScene>((Func<UIScene, bool>) (it => it.ActivationState == 0L)).FirstOrDefault<UIScene>((Func<UIScene, bool>) (it => it is UIWindowScene));
      return uiWindowScene == null ? (UIWindow) null : ((IEnumerable<UIWindow>) uiWindowScene.Windows).FirstOrDefault<UIWindow>((Func<UIWindow, bool>) (it => it.IsKeyWindow));
    }

    internal static UIViewController FindPresentedViewController()
    {
      UIViewController presentedViewController = CustomViewExtensions.FirstKeyWindowOrDefault()?.RootViewController;
      while (presentedViewController?.PresentedViewController != null)
        presentedViewController = presentedViewController.PresentedViewController;
      return presentedViewController;
    }
  }

Their fix (again, based on decompiled output) was to filter based on the scene's role, so only Application scenes get returned. It didn't require a change to MAUI because this helper class is owned by them, it's not WindowStateManager.

bijington commented 7 months ago

That's interesting! This may well be the fix for us too. @zeldafreak would you feel comfortable trying to open a PR to test out whether this fixes it within the toolkit?

softlion commented 5 months ago

Do you still want a pr ?

bijington commented 5 months ago

Yes please