Fix issue where setting StaggeredLayout.ColumnWidth to NaN would cause an exception

[winston-de]

Fixes #4063

The exception was caused by having NaN the value of width in item.Element.Measure. This PR fixes this by adding a check if DesiredColumnWidth is NaN, and if so setting it instead to the available width.

PR Type

Bugfix

What is the current behavior?

When DesiredColumnWidth is NaN, an exception will be thrown when measuring item sizes, which causes a crash or will freeze the app.

What is the new behavior?

If DesiredColumnWidth is NaN, items are stretched to fit the available width.
Example:

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link:
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

Other information

[ghost]

Thanks winston-de for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[Kyaa-dost]

Thanks, @winston-de for the quick fix! Looks good and does not seem to be crashing anymore 🚀

[Rosuavio]

May I ask how you encountered this, was the value generated from something else or did you want a "use available with" option?

[winston-de]

@RosarioPulella I encountered this error when experimenting with different values for the column width. The reason I chose the use available width behavior was because that's how the staggered panel control handles NaN for the column width. If you'd like, I can change it, as the main goal of this PR is just to prevent the crash from occurring.

[Rosuavio]

So I tried the sample code (more or less) in its own project and added a button to update the DesiredColumnWidth to NaN.
Once I click the button the app crashed. I think that this is reasonable behavior as if we don't throw an exception than we would have to pick a value to use in this scenario, and I don't think we have a clear idea of what a user wanted if they tried to put NaN in this field.

I think its important that NaN remains invalid, and that its made explicit threw a crash (since we cannot make it explicit at compile time). Run time exceptions are not good, but an app behaving in an unexpected way could lead to harder to understand issues. Perhaps we should check the input sooner though and throw a more relevant error.

Also the sample app should not freeze if a sample throws an exception. The sample app needs to be able to isolate it's self from issues in samples, that is a separate issue that needs to be investigated.

[Rosuavio]

At first I was thing that maybe we should just throw a more relevant exception in
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/afcf18f0a77a15a95cd6bdd03a962a282c6e899c/Microsoft.Toolkit.Uwp.UI.Controls.Primitives/StaggeredLayout/StaggeredLayout.cs#L300-L304
But we also have the option of reverting to e.OldValue in the occurrence of an invalid e.NewValue. Not sure what would be best any thoughts @michael-hawker ?

[michael-hawker]

@skendrot thoughts on this change?

[michael-hawker]

One other thing to point out, that is if the intent is to 'clear' the default value using {x:Null} is probably another go-to alternative over NaN. In the StaggeredPanel case this causes the availableWidth to be used and the image to fill the available space. However, in the StaggeredLayout case it causes nothing to be rendered visibly.

[XAML-Knight]

From WPF, for controls that derive from FrameworkElement, NaN was implied to mean "not set". Properties such as Width and Height backed to dependency properties, where their default values were set to NaN. NaN would continue to be the values for these properties, until they were explicitly given a value. ActualWidth & ActualHeight (in this example) were more reliable values to read from.

[michael-hawker]

Thanks for the background @XAML-Knight. Seems similar setup in UWP. Though the "Auto" value will get converted to NaN as well. Interesting that trying to type Auto doesn't work outside of Width/Height.

[ghost]

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.