Numberbox throws StackOverflowException when you clear it and it is TwoWay databound to a Double (using Binding, not x:Bind)

[sonnemaf]

Describe the bug
You get a StackOverflowException when you clear a NumberBox which Value is databound (x:Bind, TwoWay) to a property of the type Double. This happens when you use the X button in the TextBox or when you have no Text and do a LostFocus. It doesn't happen with an Int32.

Steps to reproduce the bug
Take the following code, start the app and clear the Salary or Bonus NumberBoxes.

<Page x:Class="NumberBoxTest.MainPage"
      xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
      xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
      xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
      xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
      xmlns:winui="using:Microsoft.UI.Xaml.Controls"
      mc:Ignorable="d"
      Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">

    <Grid>
        <StackPanel Width="300"
                    Spacing="8"
                    Margin="8">
            <TextBox Header="Name"
                     Text="{x:Bind Employee.Name, Mode=OneTime}"
                     MaxLength="10"
                     TextWrapping="Wrap" />
            <winui:NumberBox Header="Salary"
                             NumberFormatter="{x:Bind DutchDecimalFormatter}"
                             SpinButtonPlacementMode="Hidden"
                             AcceptsExpression="True"
                             IsWrapEnabled="true"
                             Value="{x:Bind Employee.Salary, Mode=TwoWay}" />
            <winui:NumberBox Header="Bonus"
                             NumberFormatter="{x:Bind DutchDecimalFormatter}"
                             Minimum="0"
                             Maximum="10000"
                             SmallChange="100"
                             SpinButtonPlacementMode="Inline"
                             Value="{x:Bind Employee.Bonus, Mode=TwoWay}" />
            <winui:NumberBox Header="Age"
                             Minimum="16"
                             Maximum="100"
                             SpinButtonPlacementMode="Compact"
                             Value="{x:Bind Employee.Age, Mode=TwoWay}" />
            <TextBlock Text="{x:Bind Employee.Name, Mode=OneWay}" />
            <TextBlock Text="{x:Bind Employee.Salary, Mode=OneWay}" />
            <TextBlock Text="{x:Bind Employee.Bonus, Mode=OneWay}" />
            <TextBlock Text="{x:Bind Employee.Age, Mode=OneWay}" />
        </StackPanel>
    </Grid>
</Page>

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace NumberBoxTest.Models {

    class Employee : INotifyPropertyChanged {
        private string _name;
        private double _salary;
        private double _bonus;
        private int _age;

        public event PropertyChangedEventHandler PropertyChanged;

        protected virtual void OnPropertyChanged(string propertyName) {
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
        }

        public string Name {
            get => _name;
            set {
                if (value != _name) {
                    _name = value;
                    OnPropertyChanged(nameof(this.Name));
                }
            }
        }

        public double Salary {
            get => _salary;
            set {
                if (value != _salary) {
                    _salary = value;
                    OnPropertyChanged(nameof(this.Salary));
                }
            }
        }

        public double Bonus {
            get => _bonus;
            set {
                if (value != _bonus) {
                    _bonus = value;
                    OnPropertyChanged(nameof(this.Bonus));
                }
            }
        }

        public int Age {
            get => _age;
            set {
                if (_age != value) {
                    _age = value;
                    OnPropertyChanged(nameof(Age));
                }
            }
        }

    }
}

using NumberBoxTest.Models;
using Windows.Globalization.NumberFormatting;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Controls;

namespace NumberBoxTest {
    public sealed partial class MainPage : Page {

        private DecimalFormatter DutchDecimalFormatter { get; } = new DecimalFormatter(new[] { "nl-NL" }, "NL") {
            IsGrouped = true,
            FractionDigits = 2,
            NumberRounder = new IncrementNumberRounder {
                Increment = 0.25,
                RoundingAlgorithm = RoundingAlgorithm.RoundUp
            }
        };

        internal Employee Employee { get; } = new Employee {
            Name = "Fons",
            Salary = 2000,
            Bonus = 100,
            Age = 50
        };

        public MainPage() {
            this.InitializeComponent();
        }

    }
}

Expected behavior
Not a crash, databindings should never crash.

It the Minimum property is set you can set the Value to it. This is also done if the type is a Int32. If there is no Minimum you should use Double.MinValue and not Double.NaN.

Maybe you can also add an AllowClear property to the NumberBox. This would also remove the X from the TextBox.

This exception does not occur when there is an IsNaN check in the properties of the Employee class.

public double Salary {
    get => _salary;
    set {
        if (value != _salary) {
            //_salary = value;
            _salary = Double.IsNaN(value) ? 0 : value;
            OnPropertyChanged(nameof(this.Salary));
        }
    }
}

public double Bonus {
    get => _bonus;
    set {
        if (value != _bonus) {
            //_bonus = value;
            _bonus = Double.IsNaN(value) ? 0 : value;
            OnPropertyChanged(nameof(this.Bonus));
        }
    }
}

Screenshots

Version Info

NuGet package version:
Microsoft.UI.Xaml 2.3.191129002

[YuliKl]

@teaP can you take a look?

[sonnemaf]

Would it be possible to add an EmptyValue property to the NumberBox (type Double)? This would hold the value which the NumberBox gets when you clear the Text.

Just an idea.

<winui:NumberBox Header="Salary"
                 EmptyValue="0"
                 Value="{x:Bind Employee.Salary, Mode=TwoWay}" />

[teaP]

@sonnemaf, could you suggest the EmptyValue property on #1736? Thanks!

For the bug itself, this isn't an issue with NumberBox so much as the property system itself. Unfortunately, NaN isn't equal to itself, as you've probably noticed. If you want to allow the box to be empty, you could check for

if (value != _salary && !(Double.IsNaN(value) && Double.IsNaN(_salary))) { ... }

or similar.

[robloo]

@teaP @jevansaks @SavoySchuler I understand why this was closed. However, it's a design bug that is unfortunately pretty fundamental to the control. Because of this, it really should not be closed. It is a bug because it is using double.NaN in a way that breaks the dependency property system.

Fixing this may not be trivial. There is the EmptyValue property suggestion. However, I would suggest switching the backing type to a nullable double (CalendarDatePicker does this).

This also needs to be front and center in the documentation.

[jevansaks]

IMHO it is a bug in the DependencyProperty system but it's not something we can fix in all cases for WinUI 2.x. That said, I think we could fix it for x:Bind scenarios because those go through the property setter (which we control), but not Binding.

Would that be sufficient for everyone for now?

[robloo]

@jevansaks As I understand it, the DependencyProperty system use object.Equals() internally in SetValue(). If object.Equals() returns false, the property will be changed and an event raised. Double as we all know overrides object.Equals() for its type. That is where double.NaN is defined not equal to double.NaN. You dare not change that implementation either. It's an IEEE standard that NaN != NaN.

So unless you specially handle this like null in object.Equals() (which is also bad and may break code all over the place). There is no way to fix this in the dependency property system in a way that fits the architecture.

This is why I strongly feel we should switch to double? (nullable double) as the Value type of the NumberBox. That would fix this in a way that doesn't break ANY design principles or require any deeper changes. Fixing this in the dependency system is probably not the correct way to do this (unless I'm missing something).

I'm bringing this up so strongly now because we are on the edge of releasing V1 of the NumberBox. Switching from double to double? after V1 would cause a lot of problems. Adding an EmptyValue property would be unintuitive. We need to have an idea of how to fix this before V1 is released. Patching in the property setter for x:bind certainly will help, but it does nothing for the overall big-picture.

Bottom-line I don't want this simple design oversight to turn into a permanent 'feature' of the NumberBox because we don't fix it now.

[jevansaks]

I believe we tried double? and it also had issues because OS XAML doesn't handle this well in a bunch of cases. I'll let @teaP speak to that. It may still be the lesser of the two problems.

We did already ship NumberBox v1 but if this is painful enough then I agree we should revisit and possibly switch to double?.

[robloo]

@jevansaks, Ok, I understand if V1 is already wrapped up and shipped. (I need to go update some Nuget packages as I missed this!) Awesome work :)

I thought double? should work in XAML just fine considering other struct value types are nullable such as DateTimeOffset in CalendarDatePicker. I would be curious how this works so well if the XAML implementation has issues here -- but you know a lot more than I do about that.

[ranjeshj]

Turns out that there is a platform bug with nullable types outside of types in the framework which will cause it to not work in down-level OS versions so we cannot do that unfortunately. That will however not be an issue once the platform is moved out of the OS in WinUI3.

That said.. there is an solution that perhaps will help in the mean time which works fine when using x:Bind (so the sample above should work without the stack overflow crash). x:Bind is recommended over Binding for performance and debugger friendly reasons anyway so this feels like a reasonable compromise. The PR is here if you would like to see the details of the change.

[robloo]

It sounds like we should add the needs winUI 3 tag and still keep this open. x:Bind is a good interim solution but since the patch still doesn't work with Binding it's a pretty fundamental issue. It's going to cause a lot of first-time users of the control issues that shouldn't happen (especially those coming over from existing WPF/Silverlight knowledge where Binding is just muscle memory).

[ranjeshj]

@robloo I agree. We can revisit this in WinUI3.

[robloo]

@ranjeshj x:Bind doesn't work in ResourceDictionary. This is a problem as I'm trying to use the NumberBox in a custom TemplatedControl as visualized below. I'm attempting to use NumberBox as a replacement for TextBox that already exists in control template and uses TextBoxRegex Validation from the community toolkit.

Is there an easy work-around for this? (I am using Two-way data binding)

I wouldn't have closed this issue considering x:Bind does not solve the stated problem in all cases. That said, should I create a new Issue?

[ranjeshj]

Sorry. I did not mean to close this bug. The intention was to keep this bug open for the binding case (need winui3 for that). We have a bot that does some bookeeping. When I submitted the fix for x:Bind, the bot closed the bug as part of the PR submit since the PR mentioned this bug.

[ranjeshj]

@robloo I did not spent too much time trying to find a workaround for the Binding case. Would it be possible for you to share a small standalone repro of your specific scenario ? I wonder if there is a way to add a check for NaN somewhere in the callstack to avoid the stack overflow as a temporary workaround in the app code.

[robloo]

@ranjeshj Ok, thanks for explaining about the bot auto-closing this issue. That makes sense!

Concerning a repro app, I'm not sure that's needed. My understanding is x:Bind simply isn't supported in Styles. This includes the default style defined in a ResourceDictionary (Generic.xaml) of a TemplatedControl. If that's my misunderstanding please let me know.

If you are suggesting I use Binding instead of x:Bind since it's a Style, I also don't think it could be done in the control or view model even with some additional code. The problem is in the dependency property system itself and IEEE spec saying NaN != NaN. The property changed callback would fire after set, so there is no way for me to intercept this like you did for x:Bind.

Edit: Thinking less generically, I can always just subscribe to the ValueChanged event and also set Value directly in code-behind. Doing so means the control and it's default style/template require more named elements which is non-ideal but a work-around for now.

[misaya]

is this issue closed ？