microsoft / microsoft-ui-xaml

WinUI: a modern UI framework with a rich set of controls and styles to build dynamic and high-performing Windows applications.
MIT License
6.38k stars 683 forks source link

StackPanel applies Spacing to Collapsed items #916

Closed bschoepke closed 10 months ago

bschoepke commented 5 years ago

Describe the bug StackPanel applies Spacing to Collapsed child items, which means a Collapsed item causes whitespace of 2x the spacing value.

Steps to reproduce the bug

    <StackPanel Spacing="24" BorderThickness="2" BorderBrush="Navy" Padding="24">
        <StackPanel.Resources>
            <Style TargetType="Border">
                <Setter Property="BorderThickness" Value="1"/>
                <Setter Property="BorderBrush" Value="{ThemeResource SystemControlBackgroundBaseHighBrush}"/>
            </Style>
        </StackPanel.Resources>
        <Border>
            <TextBlock Text="..."/>
        </Border>

        <Border>
            <TextBlock Text="Hello"/>
        </Border>

        <!-- Expected: 24px whitespace between Hello and World. Observed: 48px. -->
        <Border Visibility="Collapsed">
            <TextBlock Text="..." />
        </Border>

        <Border>
            <TextBlock Text="World"/>
        </Border>

        <Border>
            <TextBlock Text="..."/>
        </Border>
    </StackPanel>

Expected behavior Collapsed elements should not be considered in Spacing calculations.

Screenshots image

Version Info C# UWP app targeting 1903 SDK

NuGet package version: n/a

Windows 10 version Saw the problem?
Insider Build (18913) Yes
May 2019 Update (18362) ?
October 2018 Update (17763) ?
April 2018 Update (17134) ?
Fall Creators Update (16299) ?
Creators Update (15063) ?
Anniversary Update (14393) ?
Device form factor Saw the problem?
Desktop Yes
Mobile ?
Xbox ?
Surface Hub ?
IoT ?
robloo commented 4 years ago

I just ran across this one myself. Definitely would be a big help to fix this for cases where items are dynamically shown/hidden in a StackPanel.

MartinZikmund commented 3 years ago

I would definitely love to see this behavior adjusted. Maybe it could be controlled by a separate property like SkipCollapsedSpacing. Similarly it would be useful for Grid where it could be used to skip spacing for empty columns/rows.

robloo commented 3 years ago

This is more of a bug. I wouldn't advocate adding a new property to control this. Concerns about changing existing apps would be trivial. The change is non-breaking and would only slightly affect the look of some (an extremely small number) of apps.

MartinZikmund commented 3 years ago

@robloo I am afraid people who come across this have actually employed some "hacks" to work around this (like negative margins), so it would be a breaking change. But it is probably an acceptable one. For Grid on the other hand, it could be a new (and useful) behavior.

robloo commented 3 years ago

Non-breaking to me means the app will still compile, build and run without errors or crash. Of course visually these are differences but there are also differences when styling is updated from time to time. I agree that it is acceptable reguardless.

duraz0rz commented 3 years ago

Just ran across this when trying out the Spacing property. We have a TextBlock inside a StackPanel that we dynamically show/hide, and it would be nice to use this property vs setting Margin on every child element in the StackPanel. Any updates?

ZodmanPerth commented 3 years ago

I'm adding my +1 to this.

tomtom-m commented 2 years ago

Since there doesn't seem to be an official fix for the problem, I created a new implementation which inherits from StackPanel. My version uses margins to create spacing between items in the StackPanel, but only for those items which are visible.

Full implemenation: StackPanelWithSpacing

private void SetSpacingForChildren(int spacing)
        {
            for (int i = 0; i < this.Children.Count; i++)
            {
                if (this.Children[i] is FrameworkElement element
                    && element.Visibility == Visibility.Visible)
                {
                    var halfSpacing = spacing / 2;
                    var topSpacing = i == 0 ? 0 : halfSpacing;
                    element.Margin = new Thickness(element.Margin.Left, element.Margin.Top + topSpacing, element.Margin.Right, element.Margin.Bottom + halfSpacing);
                }
            }
        }
brandon3343 commented 2 years ago

two years has passed, issue should be closed if not planned.

MartinRichards23 commented 2 years ago

I'd like to see this change, I've never been able to use the Spacing property due to this, and I would use it often otherwise.

LucaCris commented 2 years ago

This is a "one minute fix" (replicating @tomtom-m code). Please...

Odinodin commented 1 year ago

I wasted some time on this today, is there any chance this bug will be fixed after 3.5 years?

trungnt2910 commented 1 year ago

This is the nth WinUI bug that I've encountered today, all of which has been opened for a few years and labeled with needs-winui-3.

With WinUI3 out for a while now, when will this issue (and many others with needs-winui-3) be finally fixed?

robloo commented 1 year ago

@trungnt2910 Begin transition to other UI frameworks. Microsoft is killing this off for anyone but the OS team and C++ apps. I would even go as far as to say it will never be open sourced (contrary to what they will publicly state) so the community will never be able to fix issues like these. If you are using C# Avalonia will be dominant in a few years, it is already dominant in most scenarios.

Microsoft is repeating the same failures here with WinUI3 we've seen many times before. https://github.com/microsoft/microsoft-ui-xaml/issues/3639

trungnt2910 commented 1 year ago

I already tried Avalonia, probably the best framework for desktop-only development, but not most scenarios.

Still I'm developing a cross-platform application (Desktop, Mobile, Web), and I'm using the Uno Platform for this. On Windows, I rely on WinUI3.

bogdan-patraucean commented 1 year ago

This is a basic feature that should work by default. We deserve an update on this. I encountered the same issue and it's just annoying to see it doesn't work as expected.

bogdan-patraucean commented 1 year ago

@tomtom-m I've allowed myself to improve your solution as it doesn't take 2 aspects into consideration:

var lastVisibleIndex = Children.IndexOf(Children.LastOrDefault(c => c.Visibility is Visibility.Visible));

for (int i = 0; i < Children.Count - 1; i++)
{
    if (Children[i] is FrameworkElement child && child.Visibility is Visibility.Visible && i != lastVisibleIndex)
    {
        child.Margin = new Thickness(child.Margin.Left, child.Margin.Top, Orientation is Orientation.Horizontal ? gap : child.Margin.Right, Orientation is Orientation.Vertical ? gap : child.Margin.Bottom);
    }
}
LucaCris commented 1 year ago

Correction, @bogdan-patraucean :

Orientation is Orientation.Horizontal ? child.Margin.Right + gap : child.Margin.Right

and the same for the other side...

bogdan-patraucean commented 1 year ago

@LucaCris the idea is to place the spacing only when the orientation is set to that particular one, otherwise the default Margin can be applied, not viceversa. Just test it, it works.

LucaCris commented 1 year ago

@LucaCris the idea is to place the spacing only when the orientation is set to that particular one, otherwise the default Margin can be applied, not viceversa. Just test it, it works.

The implicit item margin must be present every time. Plus the gap when needed...

fabianoriccardi commented 1 year ago

Any updates? Almost 4 years since this issue was opened... I'm testing WinUI3, but this kind of unexpected behavior are annoying and discouraging for the adoption of this library.

LucaCris commented 1 year ago

Any updates? Almost 4 years since this issue was opened... I'm testing WinUI3, but this kind of unexpected behavior are annoying and discouraging for the adoption of this library.

Do not exagerate... The library is very good. There are some minor problems only.

robloo commented 1 year ago

That wasn't an exaggeration by any means. It is an example of pain point that has never been fixed (but should have been easy to resolve). It's why, among many other things, many companies (including mine) dropped UWP/WinUI. Even MAUI is adding a WPF backend because WinUI 3 future is in question.

bogdan-patraucean commented 1 year ago

Any updates? Almost 4 years since this issue was opened... I'm testing WinUI3, but this kind of unexpected behavior are annoying and discouraging for the adoption of this library.

Do not exagerate... The library is very good. There are some minor problems only.

There are some really serious problems. Indeed, maybe this isn't one of them but they add up. You can't event set a Min or Max width and height for a Window. I myself have a lot of issues opened that haven't been fixed yet. The team working on this is small, and their effort is sometimes redirected in other parts than fixing the library, like File Explorer for example.

trungnt2910 commented 1 year ago

@robloo

Even MAUI is adding a WPF backend because WinUI 3 future is in question.

A bit off-topic, but where did you get this information?

fabianoriccardi commented 1 year ago

A bit off-topic, but where did you get this information?

I'm curious too, it is not the first time I read similar comment un WinUI team.

You can't event set a Min or Max width and height for a Window.

Moreover, you cannot either set the width/height of the Window, you have to get AppWindow to do it. Nothing near to an issue, but it seems I'm not using a "native" framework.

It is an example of pain point that has never been fixed.

Exactly, there are many bugs also on TreeView (I have performed a query and there are 73 open issues, I have personally hit few of them), and this is a quite basic component, TreeView exists since WinForms and even before.

Now, I'm not really blaming the developers, I know that if man-power and budget is not enough the result cannot be satisfying, but I would understand if investing in this framework will pay in the future.

Currently I'm not giving up, just questioning :)

UPDATE: even today, during daily activities, I have met 2 well known annoyances (or issues) about DialogContent (March 2019) and about NumberBox (August 2022).

robloo commented 1 year ago

A bit off-topic, but where did you get this information?

At this point let's just say rumors from the BUILD conference.

JJBrychell commented 10 months ago

Fix is in to not add spacing for collased items. It is scheduled for 1.5.

bogdan-patraucean commented 10 months ago

@JJBrychell awesome news, I can finally ditch my custom Stackpanel.

legistek commented 6 months ago

Grid has this issue too.

codendone commented 6 months ago

See #9491 for Grid.