Fix WrapPanel stretching behavior for last item

[AndrewKeepCoding]

Added a conditional check to prevent the last item in the WrapPanel from stretching when the parent measure's width is infinite.

Fixes

Fixes #703 and #297

PR Type

What kind of change does this PR introduce?

• Bugfix

What is the current behavior?

THIS WORKS:

<Grid ColumnDefinitions="*">
    <controls:WrapPanel StretchChild="Last">
        <Button HorizontalAlignment="Stretch" Content="Item 1" />
        <Button HorizontalAlignment="Stretch" Content="Item 2" />
    </controls:WrapPanel>
</Grid>

THIS THROWS:

This code tries to stretch its last item to an infinity width causing System.Runtime.InteropServices.COMException.

<Grid ColumnDefinitions="Auto">
    <controls:WrapPanel StretchChild="Last">
        <Button HorizontalAlignment="Stretch" Content="Item 1" />
        <Button HorizontalAlignment="Stretch" Content="Item 2" />
    </controls:WrapPanel>
</Grid>

What is the new behavior?

Doesn't stretch its last item when given an infinity width.

PR Checklist

Please check if your PR fulfills the following requirements:

• [x] Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
• [x] Based off latest main branch of toolkit
• [x] Tested code with current supported SDKs
• [ ] New component
  • [ ] Documentation has been added
  • [ ] Sample in sample app has been added
  • [ ] Analyzers are passing for documentation and samples
  • [ ] Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• [ ] Tests for the changes have been added (if applicable)
• [ ] Header has been added to all new source files
• [x] Contains NO breaking changes

Other information

[michael-hawker]

Thanks Andrew, I think a screenshot would have helped me understand the context too.

If I understand correctly, in the case where this is happening, all the items are going to be laid out on one row, as it'll never wrap due to the infinite available width, right? Therefore, yes it makes sense that the last item should just be whatever size it wants vs. trying to fill that void.

Is there a case where that wouldn't be true?

Even if in the measure pass it's infinite and doesn't stretch, if the arrange pass is fixed, then it should self-correct and still stretch when laying out, right?

It could be good to add a test here too just to catch this case so we can ensure we don't regress if we modify this logic in the future.

Thanks for taking a look into this! 🦙❤️

[AndrewKeepCoding]

Hi @michael-hawker. Thanks for the feedback. 🙂

I think a screenshot would have helped me understand the context too. Sorry about this. I just added a couple of screenshots. 😅

If I understand correctly, in the case where this is happening, all the items are going to be laid out on one row, as it'll never wrap due to the infinite available width, right? Therefore, yes it makes sense that the last item should just be whatever size it wants vs. trying to fill that void. Is there a case where that wouldn't be true?

This PR just skips the last item stretching if the given space, width or height depending on the WrapPanel's `Orientation, is infinity. It'll still wrap if there's not enough space.

Even if in the measure pass it's infinite and doesn't stretch, if the arrange pass is fixed, then it should self-correct and still stretch when laying out, right?

Yes. The UpdateRows() method is also called on arrange.

It could be good to add a test here too just to catch this case so we can ensure we don't regress if we modify this logic in the future.

I'm working on this.

[AndrewKeepCoding]

@michael-hawker I added a simple test case, and it passes successfully with the fix. Without the fix, the test doesn't technically "fail". It breaks the test host itself. This is the error I'm getting:

The active test run was aborted. Reason: Unable to communicate with test host process.

I already tried handling this at App.xaml but didn't help.

public App()
{
    this.InitializeComponent();
    UnhandledException += (sender, e) =>
    {
        // Confimed that the exception (System.Runtime.InteropServices.COMException)
        // is handled.
        e.Handled = true; 
    };
}

I don't think that this behavior is related to the WrapPanel.

[AndrewKeepCoding]

Hi @Jay-o-Way! Thanks for the clarification 🙂 By “infinite width,” I’m specifically referring to the availableSize.Width argument passed to the WrapPanel’s MeasureOverride() method. In some layouts, such as when the panel is placed inside a Grid with Auto column width, this value can be double.Infinity. When that happens, the last item tries to stretch across an infinite space, which leads to a System.Runtime.InteropServices.COMException. This PR introduces a guard to skip that stretching behavior when infinite space is detected. Sorry for the confusion.

[Jay-o-Way]

@AndrewKeepCoding yep, you know a lot more about this than I do. However, I do want to say that - from my perspective - I'm not sure if this is the best solution. I mean, given these two properties:

<Grid ColumnDefinitions="*">
    <controls:WrapPanel StretchChild="Last">

combined with your first screenshot, the result IS exactly what I would expect it to be: the grid is stretched, and the last item stretches pretty inside the available space. The result you introduce with this PR is that the last element does not stretch, and that would seem like a new bug (for future developers who don't know about this history).

Obviously, the runtime exception is something to solve, but I don't think this is the best result.

[AndrewKeepCoding]

The result you introduce with this PR is that the last element does not stretch, and that would seem like a new bug (for future developers who don't know about this history).

I agree. But I couldn't find a good way to stretch the last item on this case. Interestingly, even the Slider control (which stretches by default) only takes its minimal space when placed in an Auto column.

STRETCHES:

<Grid ColumnDefinitions="*">
    <Slider />
</Grid>

DOES NOT STRETCH:

<Grid ColumnDefinitions="Auto">
    <Slider />
</Grid>

[Jay-o-Way]

even the Slider control (which stretches by default) only takes its minimal space when placed in an Auto column

As long a any control does not have a minimum size, that is to be expected.

[AndrewKeepCoding]

As long a any control does not have a minimum size, that is to be expected.

Since both WrapPanel and TokenizedTextBox, like the Slider, default to MinWidth = 0 this behavior is not that weird, right? 😅

[AndrewKeepCoding]

@michael-hawker Sorry about the XAML Styler. It's fixed now. I also added a remark to the StretchChild property explaining the behavior for parents with infinity width.

[AndrewKeepCoding]

Would you mind shoring up the tests a bit? We have the new pattern which should make it a bit easier, pointed to the examples from the template.

@michael-hawker Sure thing! I'm currently working on an issue on the WinUI Gallery that involves a wide range of changes. Once I wrap that up, I'll shift focus to this. 🙂

[AndrewKeepCoding]

Hi @michael-hawker. I just pushed the additional tests. Please let me know if they cover the scenarios you had in mind.

[michael-hawker]

Not sure if this is the same or a different scenario, so would be good to discuss/confirm if it's the same or a different bug. But if it's the same issue then it's a counter-argument to the behavior maybe setup here. As I was writing up the spec for the WinUI transfer and looking at primitive examples and was a bit baffled at what was happening with StretchChild and my intuitive expectations...

Take this code:

    <controls:WrapPanel Width="132" StretchChild="Last">
        <Rectangle Fill="Red" Width="45" Height="44"/>
        <Rectangle Fill="Blue" Width="44" Height="44"/>
        <Rectangle Fill="Green" Width="44" Height="44"/>
        <Rectangle Fill="Orange" Height="44"/>
    </controls:WrapPanel>

This properly stretches the last item across the bottom:

However, if we make enough room for the green box on the first row (by making the red 44 width [only change])...

    <controls:WrapPanel Width="132" StretchChild="Last">
        <Rectangle Fill="Red" Width="44" Height="44"/>
        <Rectangle Fill="Blue" Width="44" Height="44"/>
        <Rectangle Fill="Green" Width="44" Height="44"/>
        <Rectangle Fill="Orange" Height="44"/>
    </controls:WrapPanel>

Actual: Then the Orange rect disappears entirely!

Expected: I would expect it to span the entire bottom row, like so:

This is the behavior we should at least make sure works (I can open separate issue if it's not related to this one), and have a test for. FYI @azchohfi as we'll need to port whatever fixes we make here to WinUI as well. I'm going to spec it up as I expect it to work for now.

[AndrewKeepCoding]

Not sure if this is the same or a different scenario, so would be good to discuss/confirm if it's the same or a different bug.

@michael-hawker I believe this is a different issue. We get this bug because the WrapPanel doesn't create a new row when the last child's DesiredWidth is 0. In this case, we can check if the position.U is larger than the parentMeasure.U.

I took a quick look and it seems that we can address this by updating the condition like so:

void Arrange(UIElement child, bool isLast = false)
{
    ...

    var desiredMeasure = new UvMeasure(Orientation, child.DesiredSize);
--  if ((desiredMeasure.U + position.U + paddingEnd.U) > parentMeasure.U)
++  if ((desiredMeasure.U + position.U + paddingEnd.U) > parentMeasure.U || position.U >= parentMeasure.U)
    {
        // next row!
        position.U = paddingStart.U;
        position.V += currentRow.Size.V + spacingMeasure.V;

        _rows.Add(currentRow);
        currentRow = new Row(new List<UvRect>(), default);
    }

    ...
}

Let me know if you want me to create a new issue for this one.

[michael-hawker]

Ah, thanks @AndrewKeepCoding for the confirmation! Appreciate your insights. I'll create a new issue #743 and we can open a new PR with some added tests and things for that scenario outside of this one then. Let me know on that issue if you're also interested in taking a look.

[AndrewKeepCoding]

@michael-hawker Can we merge this first, before I start working on the PR for the new row issue? Just to avoid working with conflicts later.🙂

[michael-hawker]

Thanks for adding the breakdown and pictures to the original post @AndrewKeepCoding, that was super useful with the simple examples! 🦙❤️

Actually, the only thing I'll suggest is that we add a note about this behavior in the doc somewhere. @AndrewKeepCoding mind updating the WrapPanel.md to call this out (You can use the > [!NOTE] syntax)?

(You can also see where I had added a StretchChild section in the new spec/doc draft for WinUI here)

@Arlodotexe mind just giving this a once-over on your own? Otherwise, I think we're all set!

[AndrewKeepCoding]

mind updating the WrapPanel.md to call this out (You can use the > [!NOTE] syntax)?

I just pushed a commit for the note.

BTW, I noticed that the > [!NOTE] syntax doesn't seem to render in MarkdownTextBlock. It's the same on the WCT Gallery app from the store. Is this a known issue?

[AndrewKeepCoding]

It also looks like we cannot use Binding to the current Page from a template using ElementName. I need to remove the bindings for HorizontalSpacing and VerticalSpacing to avoid crashes. You can see the same behavior (crash) on the StaggeredPanelSample page.

<ItemsControl.ItemsPanel>
    <ItemsPanelTemplate>
        <controls:WrapPanel x:Name="sampleWrapPanel"
                            Padding="12"
                            HorizontalSpacing="{Binding HorizontalSpacing, ElementName=ThisSamplePage, Mode=OneWay}"
                            VerticalSpacing="{Binding VerticalSpacing, ElementName=ThisSamplePage, Mode=OneWay}" />
    </ItemsPanelTemplate>
</ItemsControl.ItemsPanel>

[michael-hawker]

@AndrewKeepCoding yes, we haven't added support in the app back for the special note syntax, it's a known issue (I'm not sure if it's tracked though anywhere). I can't remember if we're using the new or old Markdown control still, if old, it'd be a matter of copying the code from the old gallery. If new, I don't think that's supported yet for extensions.

I believe the app runs with AOT, so there are issues with regular bindings. 😢

[michael-hawker]

Thanks @AndrewKeepCoding for taking a look at this and adding all the tests! 🦙❤️