Microsoft-ui-xaml: FlowLayoutAlgorithm.Generate() should take into account the spacing after the current element

[KPixel]

Currently, FlowLayoutAlgorithm considers that there is enough space for the current item if it fits:

https://github.com/microsoft/microsoft-ui-xaml/blob/1ced221991d3abd51875d24d69221afa878b6c5a/dev/Repeater/FlowLayoutAlgorithm.cpp#L324
However, it doesn't add minItemSpacing for that current item.

This is a problem because UniformGridLayout computes the number of items per line with the assumption that they all include that spacing:

https://github.com/microsoft/microsoft-ui-xaml/blob/1ced221991d3abd51875d24d69221afa878b6c5a/dev/Repeater/UniformGridLayout.cpp#L142

So, we can have a weird visual bug in the following scenario.

Steps to reproduce the bug

Steps to reproduce the behavior:

1. Create an ItemsRepeater with:
   Layout = UniformGridLayout { Orientation = Orientation.Vertical, MinItemWidth = 100, MinItemHeight = 40, MinRowSpacing = 10, MinColumnSpacing = 10 };
2. Resize the window.Height to be exactly 490
3. Scroll forward until the first line is no longer visible.
4. You should obtain something similar to this:

In this example, the FlowLayoutAlgorithm allows the item "♯9" to be in the first line because:

double remainingSpace = Minor(availableSize) - (MinorStart(previousElementBounds) + MinorSize(previousElementBounds) + minItemSpacing + Minor(desiredSize));
// 0 = 490 - (450 + 40 + 10 + 40);

Which means that it generates 10 lines.

However, UniformGridLayout computes 9 lines:

const int itemsPerLine = Minor(availableSize) / GetMinorSizeWithSpacing(context));
// 9 = 490 / (40 + 10)

Expected behavior

If we change remainingSpace to substract an additional minItemSpacing, then there will only be 9 lines and the weird bug will not occur:

If we want to allow 10 lines in this scenario, UniformGridLayout needs to be update everywhere in how it computes the number of items per line...

Update: This bug seems to only occur when Orientation == Vertical. And the suggested fix should only be applied in that case (otherwise, it causes the bug to occur when Horizontal).

Version Info
Master branch on GitHub (as of the creation of this issue).

[ranjeshj]

@KPixel Thanks for the in-depth details. I'm a bit surprised about it repro'ing for just one orientation since most of this code is working on major/minor (scrolling vs not direction) instead of vertical/horizontal. When you switched orientation, did you change the ScrollViewer to switch its scrolling orientation as well ?. Would you be able to contribute a fix ?

[KPixel]

I had also assumed that the bug would occur in both orientations.
I did change the ScrollViewer as required.

Regarding the fix, sorry, I can't submit a pull request.
But my understanding is that we should just change:

https://github.com/microsoft/microsoft-ui-xaml/blob/1ced221991d3abd51875d24d69221afa878b6c5a/dev/Repeater/FlowLayoutAlgorithm.cpp#L324

To:

double currentItemSpacing = GetScrollOrientation() == ScrollOrientation::Vertical ? 0.0 : minItemSpacing;
double remainingSpace = Minor(availableSize) - (MinorStart(previousElementBounds) + MinorSize(previousElementBounds) + minItemSpacing + Minor(desiredSize) + currentItemSpacing);

But maybe I am missing something (esp now that I see that it only occurs when Orientation == Vertical).

[chingucoding]

@KPixel I'm having difficulties creating the visuals you show in the screenshots. Can you share a repro app or the sample code you used?

[KPixel]

@chingucoding I will try to put together a sample app later today.

[KPixel]

My code has changed a lot since I created this issue, and I can't make an isolated repro app.

By the way, while working on it, I stumbled on another bug: (Maybe it deserves its own issue)

C# public sealed class Issue2373ItemsRepeaterPage : Page { public Issue2373ItemsRepeaterPage() { var repeater = new ItemsRepeater { ItemsSource = Enumerable.Range(0, 1000).Select(i => new Border { Background = new SolidColorBrush(Colors.Blue), Child = new TextBlock { Text = "#" + i } }).ToArray(), Layout = new UniformGridLayout { MinItemWidth = 100, MinItemHeight = 40, MinRowSpacing = 10, MinColumnSpacing = 10 } }; Content = new ScrollViewer { Content = repeater }; } }

When you scroll to the end then scroll back to the start, Item #0 is not visible. Then, trying to scroll to the end again causes COMException: "ItemsRepeater's child not found in its Children collection."
Tested with version 2.4.2 and 2.5.0.prerelease.

[chingucoding]

My code has changed a lot since I created this issue, and I can't make an isolated repro app.

That's unfortunate. Maybe someone else is able to create a repro or I'll need to dig deeper on how to repro this.

When you scroll to the end then scroll back to the start, Item #0 is not visible. Then, trying to scroll to the end again causes COMException: "ItemsRepeater's child not found in its Children collection."

That definitely sounds like a bug that we should track in a separate issue. Can you create a new issue for that @KPixel ?

[KPixel]

I've created issue #2834 for the new bug.

Regarding this issue, do you see value in where I thought the bug was? (The difference between how FlowLayoutAlgorithm computes the items in a line and how UniformGridLayout computes the number of items per line).
It seemed to make sense that if the spacing is large enough, the discrepancy between these two algos could cause glitches... But maybe I'm missing something.

[chingucoding]

It seems that you tried your fix and it seemed to work so I assume it's not completely wrong. However what confuses me a bit is that your fix is by essentially subtracting minItemSpacing twice if we are in horizontal mode.
Given that horizontal and vertical should behave the same and your fix breaks some existing unit tests, it might actually be caused somewhere else. But that's just a guess, I would need to get the bug under the debugger and find out what exactly causes the second and following lines to render differently. Given that there are not many areas where we differ between vertical and horizontal orientation, I am even more unsure what is actually going on here.

So in conclusion, I would say, your idea makes sense that the algorithms are clashing here, but I wouldn't go that far to say it is 100% that, I wouldn't be that surprised if there is something else going on.

One thing that would have been interesting to look into would be if the behavior would be reproducable in both directions if you use square items (assuming you didn't test that). Maybe the fact that the items in your screenshot are rectangular would explain why those behave differently based on orientation.