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.2k stars 375 forks source link

[BUG] Popup Does Not Support Multi-Window #1275

Closed babenkovitaliy closed 12 months ago

babenkovitaliy commented 1 year ago

Is there an existing issue for this?

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

Current Behavior

ShowPopup() and ShowPopupAsync() results in the popup generating in a wrong window on a MacCatalyst application. This requires that MacCatalyst has multi window enabled.

I originally stumbled upon this issue when I opened up another window and tried to display a popup. It resulted in the popup displaying on a primary screen. Attempting to reproduce works, but sporadically - I could not figure out exactly what was the root cause. I tried opening up windows, focusing, resizing - anything to recreate the issue with 100% success rate (instead of 50%).

After running the tests, I figured out that I can recreate this problem with 100% success rate by opening up multiple windows (not just two). I will provide the steps to reproduce.

Expected Behavior

ShowPopup and ShowPopupAsync() should display the popup on the window that requested it.

Steps To Reproduce

  1. Clone the project from the GitHub repo.
  2. Open the solution in Visual Studio for Mac and click on the "Play" button to run the application in debug mode.
  3. You will now see a Mac app running. The main window will have a button that says "Click me to open a new window". Click on it two times to open up two windows. I'll call those "secondary windows".
  4. Move those two secondary windows side by side (to make it more easier to see the issue).
  5. Click on either one of the buttons on the left window ("Popup" or "PopupAsync" button).
  6. If the popup showed up on the right window when it should have showed up on the [same] left window, then you have successfully recreated the bug. If you were unable to replicate this, then try to click on either button on the right window instead. At this point, you should be able to replicate this.

Link to public reproduction project repository

https://github.com/babenkovitaliy/MAUIToolkitPopupBug

Environment

- .NET MAUI CommunityToolkit: 5.2.0
- OS: MacOS Ventura 13.4.1
- .NET MAUI: 7.0.86/7.0.100
- Hardware: MacBook Air M2, 2022

Anything else?

No response

babenkovitaliy commented 1 year ago

image

babenkovitaliy commented 1 year ago

I decided to clone the repo, create separate test UI project that supports multiple windows. Then I added a whole bunch of breakpoints to see at what point does the library think that the popup should appear in a certain [incorrect] window. Under CommunityToolkit.Maui.Core.Views.MauiPopup class, there is a method called SetPresentationController(). This is what the code looks like:

    void SetPresentationController()
    {
        var popOverDelegate = new PopoverDelegate();
        popOverDelegate.PopoverDismissedEvent += HandlePopoverDelegateDismissed;

        UIPopoverPresentationController presentationController = (UIPopoverPresentationController)(PresentationController ?? throw new InvalidOperationException($"{nameof(PresentationController)} cannot be null."));
        presentationController.SourceView = ViewController?.View ?? throw new InvalidOperationException($"{nameof(ViewController.View)} cannot be null.");

        presentationController.Delegate = popOverDelegate;
    }

I added a breakpoint at the last line of code before the ending curly brace and hovered over a variety of variables of properties and hovered over the ViewController property. Getting data from the View child property was a bit confusing to understand, so I took a look at CurrentView child property and in the 'Id' property, I saw that it was pointing to the wrong view. It was not pointing to the view where the button was clicked, but pointing to the other window where the popup was incorrectly showing up.

My next step is to figure out what sets the ViewController property and why does it get set incorrectly.

babenkovitaliy commented 1 year ago

Didn't take too long to backtrace to the culprit:

    public void SetElement(IPopup element)
    {
        if (element.Parent?.Handler is not PageHandler)
        {
            throw new InvalidOperationException($"The {nameof(element.Parent)} must be of type {typeof(PageHandler)}.");
        }

        VirtualView = element;
        ModalPresentationStyle = UIModalPresentationStyle.Popover;

        _ = View ?? throw new InvalidOperationException($"{nameof(View)} cannot be null.");
        _ = VirtualView ?? throw new InvalidOperationException($"{nameof(VirtualView)} cannot be null.");

        var rootViewController = WindowStateManager.Default.GetCurrentUIViewController() ?? throw new InvalidOperationException($"{nameof(PageHandler.ViewController)} cannot be null.");
        ViewController ??= rootViewController;
        SetDimmingBackgroundEffect();
    }

WindowStateManager.Default.GetCurrentUIViewController() is returning an incorrect value. Maybe it works fine for single window applications, but it may not work for multi-window applications. I'm gonna poke around and try to look for a solution.

cat0363 commented 1 year ago

I don't know how it works on MacCatalyst versions before 14.0 because I can't test it on versions before 14.0, but the following worked. However, this works as intended if the target window is always active. If there are Window A and Window B, and you press a button to display a Popup in Window B while Window A is active and Window B is inactive, the Popup will be displayed in Window A. The following is implemented with reference to the part to obtain the active window of .NET MAUI.

public void SetElement(IPopup element)
{
    if (element.Parent?.Handler is not PageHandler)
    {
        throw new InvalidOperationException($"The {nameof(element.Parent)} must be of type {typeof(PageHandler)} or PhoneFlyoutPageRenderer.");
    }

    VirtualView = element;
    ModalPresentationStyle = UIModalPresentationStyle.Popover;

    _ = View ?? throw new InvalidOperationException($"{nameof(View)} cannot be null.");
    _ = VirtualView ?? throw new InvalidOperationException($"{nameof(VirtualView)} cannot be null.");

    var rootViewController = GetRootViewController() ?? throw new InvalidOperationException($"{nameof(PageHandler.ViewController)} cannot be null.");
    ViewController ??= rootViewController;
    SetDimmingBackgroundEffect();
}

static UIViewController? GetRootViewController()
{
    var window = GetActiveWindow();
    var viewController = window?.RootViewController;
    if (viewController != null)
    {
        while (viewController.PresentedViewController != null)
        {
            viewController = viewController.PresentedViewController;
        }
    }

    return viewController;
}

static UIWindow? GetActiveWindow()
{
    UIWindow? window = null;

    if (OperatingSystem.IsIOSVersionAtLeast(14) || OperatingSystem.IsMacCatalystVersionAtLeast(14))
    {
        var activeWindowScenes = new List<UIWindowScene>();
        foreach (var scene in UIApplication.SharedApplication.ConnectedScenes)
        {
            if (scene is UIWindowScene windowScene &&
                windowScene.TraitCollection.ActiveAppearance == UIUserInterfaceActiveAppearance.Active)
            {
                activeWindowScenes.Add(windowScene);
            }
        }
        if (activeWindowScenes.Count > 0)
        {
            if (activeWindowScenes.Count > 1)
            {
                foreach (var ws in activeWindowScenes)
                {
                    if (ws.WindowingBehaviors is not null && !ws.WindowingBehaviors.Closable)
                    {
                        if (ws?.KeyWindow?.RootViewController != null &&
                            ws?.KeyWindow?.WindowLevel == UIWindowLevel.Normal)
                        {
                            window = ws.KeyWindow;
                            break;
                        }
                    }
                }
            }
            else
            {
                window = activeWindowScenes[0].KeyWindow;
            }
        }
    }
    else
    {
        window = UIApplication.SharedApplication.KeyWindow;
    }
    return window;
}
babenkovitaliy commented 1 year ago

However, this works as intended if the target window is always active. If there are Window A and Window B, and you press a button to display a Popup in Window B while Window A is active and Window B is inactive, the Popup will be displayed in Window A.

Here are a few questions that I'd want to answer:

Now think of this as a product manager of your company:

What is a real life example where the first bullet point would make sense from a perspective of a user? Or perspective from a developer who want to have a consistent experience displaying a popup to a user? I don't see any. Now, bullet point #2 makes much more sense.

cat0363 commented 1 year ago

The code I presented is at least not what you're expecting. I would like the window on the side where the button is pressed to be active, but my code did not do that. I'd like the window on which the button was pressed to be considered the active window, but the code I've provided doesn't do that. I have not been able to find out the reason why the window does not become active when the button of an inactive window is suddenly pressed.

At least it's better than the previous behavior, but it's not perfect.

I think the ideal would be the latter behavior you describe, but I can't say because of the design concept involved.

babenkovitaliy commented 1 year ago

Ah sorry. I misread your message. Your code definitely looks like it may help solve this issue. You should try to submit a PR for it.

cat0363 commented 1 year ago

@babenkovitaliy , Thank you for your reply. Before creating a PR, it is necessary to verify the operation of each version of MacCatalyst, but I can only verify some versions. Is there anyone who can verify this with each version of MacCatalyst? We need to make sure that the API you are calling is ok with each version of MacCatalyst.

babenkovitaliy commented 1 year ago

@cat0363 I gotta give you massive props for that example code above. Particularly the UIApplication.SharedApplication.ConnectedScenes part. I spent a good few hours on this and was finally able to create a fix that won't use the 'Active' window functionality. Instead, it will actually locate the UIViewController regardless if it is an "Active" window or not. This is what it roughly looks like (this is a nested/local method inside the 'SetElement' method).

        UIViewController? GetUIViewControllerFromPopupParentPage()
        {
            var scenes = UIApplication.SharedApplication.ConnectedScenes.OfType<UIWindowScene>().ToList();

            foreach (var scene in scenes)
            {
                foreach (var window in scene.Windows)
                {
                    var isContainerViewController = window.RootViewController?.GetType() == typeof(Microsoft.Maui.Platform.ContainerViewController)
                        || window.RootViewController?.GetType().BaseType == typeof(Microsoft.Maui.Platform.ContainerViewController);

                    if (!isContainerViewController)
                    {
                        continue;
                    }

                    var rvc = window.RootViewController as Microsoft.Maui.Platform.ContainerViewController;
                    var isViewMatching = rvc?.CurrentView == element.Parent;

                    if (isViewMatching)
                    {
                        return window.RootViewController;
                    }
                }
            }

Then, within the same SetElement method, I will add one line and modify one line (first line is added, second line is modified):

        var parentPageViewController = GetUIViewControllerFromPopupParentPage();
        rootViewController = parentPageViewController ?? WindowStateManager.Default.GetCurrentUIViewController() ?? throw new InvalidOperationException($"{nameof(PageHandler.ViewController)} cannot be null.");

I have tested it and it works flawlessly. It should allow Shell apps and multi-window apps to work fine. However, I need to clean up the code and name the variables more better. It is 1:30 AM (night), so I will proceed to work on this sometime tomorrow.

cat0363 commented 1 year ago

@babenkovitaliy , I'm glad you seem to have found a better way. The important point is whether it works with each version, or whether it works with both MacCatalyst and iOS. I'm looking forward to the PR you create.

babenkovitaliy commented 1 year ago

@cat0363 if you are curious, I do have a PR created. It is referenced in this thread. ^^^

cat0363 commented 1 year ago

@cat0363 if you are curious, I do have a PR created. It is referenced in this thread. ^^^

@babenkovitaliy , Thank you for letting me know. I will apply your PR and personally verify it. I hope the review goes smoothly.