microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.36k stars 678 forks source link

CommandBarFlyout has gap between primary and secondary commands when secondary are shown above. #5833

Closed eugenegff closed 8 months ago

eugenegff commented 3 years ago

Describe the bug CommandBarFlyout has gap between primary and secondary commands when secondary are shown above primary due to the constrained space. Gap is shown only first time, bug could be repeated with both WUX and MUX 2.6.2 versions of CommandBarFlyout, everything was tested on OS 19043

Steps to reproduce the bug

  1. VS2019, new blank C++/CX app, target 19041, min target 17763
  2. Add button aligned to the bottom of page with attached CommandBarFlyout

    <Page.Resources>
        <CommandBarFlyout x:Name="CommandBarFlyout1">
            <AppBarButton Label="Share" Icon="Share" ToolTipService.ToolTip="Share" />
            <AppBarButton Label="Save" Icon="Save" ToolTipService.ToolTip="Save" />
            <AppBarButton Label="Delete" Icon="Delete" ToolTipService.ToolTip="Delete" />
            <CommandBarFlyout.SecondaryCommands>
                <AppBarButton x:Name="ResizeButton1" Label="Resize"/>
                <AppBarButton x:Name="MoveButton1" Label="Move"/>
            </CommandBarFlyout.SecondaryCommands>
        </CommandBarFlyout>
    </Page.Resources>
    
    <Button VerticalAlignment="Bottom" Content="Show Flyout" Flyout="{StaticResource CommandBarFlyout1}"/>
  3. Press button - flyout is shown with gap between primary and secondary commands
  4. Press button again - flyout is shown without gap between primary and secondary commands

Expected behavior CommandBarFlyout should show secondary commands above primary ones without any gap between

Screenshots Windows.UI.Xaml version image

Microsoft.UI.Xaml version from XamlControlsGallery image

Microsoft.UI.Xaml version 2.6.2 in ControlsResourcesVersion="Version1" mode image

Version Info CommandBarFlyout from both Microsoft.UI.Xaml 2.6.2 and Windows.UI.Xaml has this bug Host OS 19043.1165 UWP App, WinUI 2.6.2

Windows app type: UWP Win32
Yes
Windows version Saw the problem?
Insider Build (xxxxx)
May 2021 Update (19043) Yes
October 2020 Update (19042)
May 2020 Update (19041)
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop Yes
Xbox
Surface Hub
IoT
eugenegff commented 3 years ago

I suspect that problem is within CommandBar code used inside CommandBarFlyout. From my investigation container for secondary commands measures too large, and then shrinks on first closing. But without CommandBar source code it is hard to move further

zhuxb711 commented 3 years ago

They just don't want to fix this issue since I mention it on Aug, 2020 #3209

eugenegff commented 3 years ago

New knowledge - the height of the gap is proportional to the count of the secondary commands aka menu items. Seems that CommandBar has code that attempts to predict corresponding height, but does it incorrectly till the first layout. For my menu with 9 secondary items in file CommandBarFlyoutCommandBar.cpp, line 538

                m_secondaryItemsRoot.get().Measure({ std::numeric_limits<float>::infinity(), std::numeric_limits<float>::infinity() });
                const auto overflowPopupSize = m_secondaryItemsRoot.get().DesiredSize();

initially I got overflowPopupSize.Height == 370 and popup was shown with large gap as on third attached screenshot, and then on second context menu opening overflowPopupSize was 298, i.e. less by 72pt, 8pt per secondary menu item, and there were no gap. P.S. screenshots were made on monitor with 150% scale, 8pt == 12px

eugenegff commented 3 years ago

It was AppBarButton in CommandBarFlyout.SecondaryCommands measured as 40pt before first showing, becaming 32pt on second and following showing attempt. Unfortunately wrong size was already used to calculate overflow popup size. Could be seen and worked around with quick and dirty code like this

    [Windows::Foundation::Metadata::WebHostHidden]
    public ref class CommandBarFlyoutSecondaryItem sealed
        : public Windows::UI::Xaml::Controls::AppBarButton
    {
    public:
        CommandBarFlyoutSecondaryItem() {}
    protected:
        virtual Windows::Foundation::Size MeasureOverride(Windows::Foundation::Size availableSize) override
        {
            auto sz = __super::MeasureOverride(availableSize);
            sz.Height = 32.0f; // workaround for wrong measured height
            return sz;
        }
    };

and somewhere inside Themes\generic.xaml <Style TargetType="local:CommandBarFlyoutItem" BasedOn="{StaticResource CommandBarFlyoutAppBarButtonStyle}"/>

StephenLPeters commented 3 years ago

@eugenegff I beleive this issue has been fixed in windows 11, are you able to verify you don't see the behavior on windows 11?

eugenegff commented 3 years ago

latest Windows 11, 10.0.22000.184, problem still exist

OTOH, it became less reproducible.

With the repro code in first post it is still reproducible with WUX CommandBarFlyout, but not with MUX 2.6.2 in either ControlsResourcesVersion mode.

In our app bug is still reproducible with MUX 2.6.2 CommandBarFlyout in ControlsResourcesVersion=Version1 mode.

So the bug is definitely still with us.

image

image

StephenLPeters commented 3 years ago

@eugenegff what scale factor is your display at when you get these repro's?

eugenegff commented 3 years ago

@eugenegff what scale factor is your display at when you get these repro's?

4K monitor, 150% scale. I repeated tests on 100% scale - it does not matter, everything including gap became proportionally smaller.

Oh, I saw. I fixed my comments where I said "px" but mean "pt", mea culpa.

eugenegff commented 3 years ago

It was AppBarButton in CommandBarFlyout.SecondaryCommands measured as 40pxpt before first showing, becaming 32pxpt on second and following showing attempt. Unfortunately wrong size was already used to calculate overflow popup size. Could be seen and worked around with quick and dirty code like this

  [Windows::Foundation::Metadata::WebHostHidden]
  public ref class CommandBarFlyoutSecondaryItem sealed
      : public Windows::UI::Xaml::Controls::AppBarButton
  {
  public:
      CommandBarFlyoutSecondaryItem() {}
  protected:
      virtual Windows::Foundation::Size MeasureOverride(Windows::Foundation::Size availableSize) override
      {
          auto sz = __super::MeasureOverride(availableSize);
          sz.Height = 32.0f; // workaround for wrong measured height
          return sz;
      }
  };

and somewhere inside Themes\generic.xaml <Style TargetType="local:CommandBarFlyoutItem" BasedOn="{StaticResource CommandBarFlyoutAppBarButtonStyle}"/>

It seems that at the time of calculating popup size AppBarButtons used as the secondary commands are not yet switched to their "overflow" visual state, and returns height of 40pt rather than actual 32pt that would be returned slightly later. Of course without AppBarButton source code I can only wonder about the actual reason of this 40pt => 32pt transition in measured height of AppBarButton. But at least I saw this transition in measured sizes by breaking inside CommandBarFlyoutCommandBar and then by overriding AppBarButton::Measure in derived class to investigate it further.

CommandBarFlyoutCommandBar.cpp:561

                m_secondaryItemsRoot.get().Measure({ std::numeric_limits<float>::infinity(), std::numeric_limits<float>::infinity() });
                const auto overflowPopupSize = m_secondaryItemsRoot.get().DesiredSize();
zadjii-msft commented 1 year ago

Not to necro an old thread, but we saw this with the Terminal too - until we switched to the Microsoft.UI.Xaml.Controls:CommandBarFlyout version. Then it just worked like a charm. We were able to do that as a drop-in replacement, just change the namespace.

Figured that future readers may find that helpful.

ranjeshj commented 8 months ago

@zadjii-msft good find. There is only one CommandBarFlyout in WinAppSDK, but there are 2 in WinUI2/OS Xaml. The WinUI2 CommandBarFlyout which is Microsoft.UI.Xaml.Controls.CommandBarFlyout is the updated version that is recommended. This is the same for few other controls like the Progress controls.

eugenegff commented 8 months ago

@ranjeshj This bug and https://github.com/microsoft/microsoft-ui-xaml/issues/5813 are actually the same bug.