ContextMenuMode="SecondWindow" can perform strangely for WinUI 3

ArvinZJC commented 2 years ago

Describe the bug

I really appreciate the idea of the new context menu option. Yet I found in my own project that its window size and the menu item height could be strange, and could be even different for the 1st time and the later time shown the menu. So I validated with your sample app and reproduced the issue.

As you can see in the screenshots, the context menu item shown the 2nd time is higher than the one shown the 1st time. Both screenshots indicate that the context menu window has incorrect size.

I notice that the screenshot in the README can demonstrate a perfect context menu. So this really confuses me.

Steps to reproduce the bug

Build the project with Visual Studio 2022.
Run the packaged H.NotifyIcon.Apps.WinUI app.
Right click the app icon in the taskbar corner to show the context menu (1st time).
Dismiss the context menu and repeat Step 3 (2nd time).

Expected behavior

Show as the screenshot in README.

Screenshots

NuGet package version

No response

Platform

WinUI

IDE

Visual Studio 2022

Additional context

Windows 11 Pro 22000.652

HavenDV commented 2 years ago

In fact, the calculation of the required size is poorly implemented at the moment. I have not yet figured out why this method returns different results from time to time. availableSize always is new Size(10000.0, 10000.0)

private static Size MeasureFlyout(FlyoutBase flyout, Size availableSize)
    {
        const double offset = 16.0;

        var width = 0.0;
        var height = offset;
        foreach (var flyoutItemBase in ((MenuFlyout)flyout).Items)
        {
            flyoutItemBase.Measure(availableSize);

            width = Math.Max(
                width,
                flyoutItemBase.DesiredSize.Width +
                flyoutItemBase.Padding.Left +
                flyoutItemBase.Padding.Right +
                offset);
            height +=
                flyoutItemBase.DesiredSize.Height +
                flyoutItemBase.Padding.Top +
                flyoutItemBase.Padding.Bottom;
        }

        return new Size(
            width: 2 * Math.Round(width),
            height: Math.Round(height));
    }

ArvinZJC commented 2 years ago

In fact, the calculation of the required size is poorly implemented at the moment. I have not yet figured out why this method returns different results from time to time. availableSize always is new Size(10000.0, 10000.0)

private static Size MeasureFlyout(FlyoutBase flyout, Size availableSize)
    {
        const double offset = 16.0;

        var width = 0.0;
        var height = offset;
        foreach (var flyoutItemBase in ((MenuFlyout)flyout).Items)
        {
            flyoutItemBase.Measure(availableSize);

            width = Math.Max(
                width,
                flyoutItemBase.DesiredSize.Width +
                flyoutItemBase.Padding.Left +
                flyoutItemBase.Padding.Right +
                offset);
            height +=
                flyoutItemBase.DesiredSize.Height +
                flyoutItemBase.Padding.Top +
                flyoutItemBase.Padding.Bottom;
        }

        return new Size(
            width: 2 * Math.Round(width),
            height: Math.Round(height));
    }

Yeah, I actually have read the related logic and do not have any better idea at present. But I have some other things to tell.

Do we really need to include the padding during the calculation? I'm not sure but I think it is repeated with DesiredSize. I used the following code instead and the window size makes more sense, though still not proper.

            width = Math.Max(
                width,
                flyoutItemBase.DesiredSize.Width +
                offset);
            height +=
                flyoutItemBase.DesiredSize.Height;

Each menu item height is also an issue. For the 1st time showing the context menu, each menu item has a height of 40, while for the later time it is 32 which is expected. Similar issue is described here: microsoft/microsoft-ui-xaml#6396

HavenDV commented 2 years ago

Do we really need to include the padding during the calculation? I'm not sure but I think it is repeated with DesiredSize. I used the following code instead and the window size makes more sense, though still not proper.

I don't know. Keep in mind the offset constant I'm using. Ideally, it shouldn't be. And if you remove the padding and offset, it turns out really bad. The size is calculated somehow differently at the time of the first appearance of the window, and, with a small chance, in other cases. I'm assuming this has to do with the asynchronous nature of computation.

Each menu item height is also an issue. For the 1st time showing the context menu, each menu item has a height of 40, while for the later time it is 32 which is expected. Similar issue is described here:

Yes, this looks like a problem. But through debug values 33.3 and 41.3 or something similar.

pluto-dev commented 2 years ago

EarTrumpet works with flyouts as well. This might be helpful

brandon3343 commented 2 years ago

This is the solution

double width1 = 0;
double height1 = 0;
int menuItemCount = 0;

foreach (var item in MyMenu.Items)
{
    if(item is MenuFlyoutItem)
    {
        menuItemCount++;
    }
    item.Measure(new Size(10000, 10000));
    height1 += item.DesiredSize.Height;
    width1 = item.DesiredSize.Width;
}
// the window's height
int height = getRealSize(height1 + menuItemCount * 2);
// the window's width
int width = getRealSize(width1) * 2;

private int getRealSize(double value)
{
    var dpi = Windows.Win32.PInvoke.GetDpiForWindow((Windows.Win32.Foundation.HWND)App.Hwnd);
    float scalingFactor = (float)dpi / 96;
    return (int)(value * scalingFactor);
}

@HavenDV can work in winui 3, no scroll bar, and use MenuFlyout Closing event to prevent close, because MenuItem size change to 32 when open again. ( MenuItem = 40 when you have't click the window or move mouse over window, after that, MenuItem change to 32. )

HavenDV commented 2 years ago

Thank you, I will test. You can replace Windows.Win32.PInvoke.GetDpiForWindow((Windows.Win32.Foundation.HWND)App.Hwnd) / 96.0 with MyMenu.XamlRoot.RasterizationScale

brandon3343 commented 2 years ago

You can replace Windows.Win32.PInvoke.GetDpiForWindow((Windows.Win32.Foundation.HWND)App.Hwnd) / 96.0 with MyMenu.XamlRoot.RasterizationScale

I didn't know such a convenient way, awesome!

HavenDV commented 2 years ago

@HavenDV can work in winui 3, no scroll bar, and use MenuFlyout Closing event to prevent close, because MenuItem size change to 32 when open again. ( MenuItem = 40 when you have't click the window or move mouse over window, after that, MenuItem change to 32. )

Ideally, I want to solve the sizing problem without preventing the MenuFlyout from closing in order to save animations. But it seems to be quite a difficult task, here is the log of the data I get:

Item0 - DesiredSize: 153.33333,39.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 52,39.333332. Padding: 11,8,11,9
Width: 175.3333282470703, Height: 117.99999737739563
Item0 - DesiredSize: 238.66667,41.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,41.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 121.99999737739563
Item0 - DesiredSize: 238.66667,41.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,41.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 121.99999737739563
Item0 - DesiredSize: 238.66667,41.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,41.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 121.99999737739563
Item0 - DesiredSize: 238.66667,33.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,33.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 105.99999737739563
Item0 - DesiredSize: 238.66667,33.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,33.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 105.99999737739563
Item0 - DesiredSize: 238.66667,33.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,33.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 105.99999737739563
Item0 - DesiredSize: 238.66667,33.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,33.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 105.99999737739563
Item0 - DesiredSize: 238.66667,33.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,33.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 105.99999737739563
Item0 - DesiredSize: 238.66667,33.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,33.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 105.99999737739563
Item0 - DesiredSize: 238.66667,33.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,33.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 105.99999737739563
Item0 - DesiredSize: 238.66667,33.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,33.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 105.99999737739563
Item0 - DesiredSize: 238.66667,33.333332. Padding: 11,8,11,9
Item1 - DesiredSize: 0,3.3333333. Padding: -4,1,-4,1
Item2 - DesiredSize: 137.33333,33.333332. Padding: 11,8,11,9
Width: 260.6666717529297, Height: 105.99999737739563

At the same time, the transparent background is not supported in stable versions of WindowsAppSDK 1.1, but it is supported in WindowsAppSDK 1.1 preview and unpackaged applications, and animations look good there. Apparently, I should implement your method for simplified context menus without animations, but showing a stable result. And for animations, turn on the full mode, where I can ensure that the menu sizes are correctly predicted. However, the code to implement all this looks very terrible.

brandon3343 commented 2 years ago

However, the code to implement all this looks very terrible.

maybe we can waiting for Microsoft fixed the bug https://github.com/microsoft/microsoft-ui-xaml/issues/7374, and use the legacy win32 context menu temporary this moment, but it's not support dark theme, so it looks weirdly a little bit in dark mode. I'm not good at c#、c++ and win32 api, I just use these to dev some tool app, so I'm sorry I can't help you. 4E32DCFE317BC5A9FB9789799D5C04CF

HavenDV commented 2 years ago

I have released a new version with updated window size calculations (thanks to @l619534951) but wrong sizes still pop up in some cases. I would appreciate any help in testing and repeatable sequences of actions when incorrect sizes appear.

IllyaTheHath commented 2 years ago

Seems the flyout size is not totally fixed, sometimes the size is just right, but sometimes it still wrong. I think it may related to DPI scale setting. 2k resolution with 200% scale, it's a bit lager: 2k resolution with 150% scale, just the size: 2k resolution with 100% scale, too small and shows the scroll bar:

And it still not shown at first right click, I need to click twice to get the menu popup.

I also tried add some extra size to the window and then hide the background window, but it seems that Microsoft has restricted the menuflyout in winui3 to only be displayed inside the window, not beyond the area of the window. Hide the window will hide the menu too.:cry: (So that's the reason why we need to calc the window size. Why doesn't Microsoft make the flyout can beyond the window borders like the winui2) Transparent window may work using preview WindowsAppSDK.

HavenDV commented 2 years ago

Thanks a lot for testing, I'll work it out and let you know when it's ready.

HavenDV commented 2 years ago

@IllyaTheHath I'm having trouble recreating screen size issues. I have 4k resolution with 150% scale, I tried changing the resolution and scale and it worked correctly in all cases, even with changing the scale while the application was running. Therefore, I have additional questions.

What version of Windows are you using?
Is the application packaged or unpackaged?

Does the application have an active manifest of this kind?


<?xml version="1.0" encoding="utf-8"?>
<assembly manifestVersion="1.0" xmlns="urn:schemas-microsoft-com:asm.v1">
<assemblyIdentity version="1.0.0.0" name="App1.app"/>

<application xmlns="urn:schemas-microsoft-com:asm.v3">
<windowsSettings>
  <!-- The combination of below two tags have the following effect:
       1) Per-Monitor for >= Windows 10 Anniversary Update
       2) System < Windows 10 Anniversary Update
  -->
  <dpiAware xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings">true/PM</dpiAware>
  <dpiAwareness xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">PerMonitorV2, PerMonitor</dpiAwareness>
</windowsSettings>
</application>
</assembly>

Make sure your .csproj has an explicit manifest setting:
```xml
<PropertyGroup>
     <ApplicationManifest>app.manifest</ApplicationManifest>
</PropertyGroup>

Also I will be grateful if you try to replicate the problem using the application for testing - https://github.com/HavenDV/H.NotifyIcon/tree/master/src/apps/H.NotifyIcon.Apps.WinUI

IllyaTheHath commented 2 years ago

@HavenDV Windows 10 21H2 with packaged application. I tested with the sample app, It show the same behavior with my app. But I think it may related to my debug environment, I'm using virtual machine for remote debugging, and it behaviors strange. When I switch to my local machine for debugging it works fine. I don't know what caused this and if it's normal.

HavenDV / H.NotifyIcon