Incorrect XAML codegen for INotifyDataErrorInfo

[Sergio0694]

Discovered in https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3569

Describe the bug
It looks like that the XAML codegen when binding a TextBox to a property from a viewmodel implementing INotifyDataErrorInfo is incorrect, which causes the UI not to be properly updated then an invalid property is toggled back to valid, if this is the last invalid property in the viewmodel. Toggling other properties seems to work just fine instead.

Steps to reproduce the bug

Steps to reproduce the behavior:

1. Clone https://github.com/mvegaca/WindowsTemplateStudio/tree/Mockup-WinUI3-ObservableValidator
2. Open the app, go do the Forms WCT page
3. Click "Submit", all fields will be invalid
4. Start filling in fields with valid values
5. Observe that whatever field you leave for last, it's impossible to make it valid again

Expected behavior

Fields should all be updated correctly in the UI.

Screenshots

Version Info

NuGet package version:

Windows app type:

UWP	Win32
	Yes

Windows 10 version	Saw the problem?
Insider Build (xxxxx)
May 2020 Update (19041)	Yes
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)

Device form factor	Saw the problem?
Desktop	Yes
Xbox
Surface Hub
IoT

Additional context

Here is my analysis of the issue, from https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3569#issuecomment-772555360.

I'm looking at the generated .g.cs backing file and it seems to me there might be something off here.
The registration of INotifyDataErrorInfo itself is done correctly right next to INotifyPropertyChanged:

public void UpdateChildListeners_ViewModel(global::WinUIDesktopApp.ViewModels.FormWCTViewModel obj)
{
    if (obj != cache_ViewModel)
    {
        // ...
        if (obj != null)
        {
            cache_ViewModel = obj;
            ((global::System.ComponentModel.INotifyPropertyChanged)obj).PropertyChanged += PropertyChanged_ViewModel;
            ((global::System.ComponentModel.INotifyDataErrorInfo)cache_ViewModel).ErrorsChanged += ErrorsChanged_ViewModel;
        }
    }
}

Now, consider the situation where the bug happens, so when we have a viewmodel with just one remaining field that is then updated, validated again and then marked as valid. The viewmodel now has 0 errors, and it raises ErrorsChanged to notify the UI of this change in errors. We expect the view to clear the errors in that control bound to this property, which is not happening. This is the code that is invoked when ErrorsChanged is raised:

public void ErrorsChanged_ViewModel(object sender, global::System.ComponentModel.DataErrorsChangedEventArgs e) 
{
    FormWCTPage_obj1_Bindings bindings = TryGetBindingObject();
    if (bindings != null)
    {
        string propName = e.PropertyName;
        if (global::System.String.IsNullOrEmpty(propName))
        {
            bindings.UpdateErrors_ViewModel((global::System.ComponentModel.INotifyDataErrorInfo)sender, "OrderID");
            // All other properties here...
        }
        else
        {
            bindings.UpdateErrors_ViewModel((global::System.ComponentModel.INotifyDataErrorInfo)sender, propName);
        }
    }
}

In this case propName is "ShipsTo", so the second branch is taken - we can move on to UpdateErrors_ViewModel:

private void UpdateErrors_ViewModel(global::System.ComponentModel.INotifyDataErrorInfo sender, string propertyName)
{
    if (this.initialized)
    {
        switch (propertyName)
        {
            // All other properties here...
            case "ShipTo":
            {
                UpdateErrors_(obj6, sender, "ShipTo");
                break;
            }
        }
    }
}

Just a wrapper to actually switch the bound property name and the respecting control, this is fine.
Now on to the possibly faulty method:

private void UpdateErrors_(global::Microsoft.UI.Xaml.Controls.Control control, global::System.ComponentModel.INotifyDataErrorInfo sender, string propertyName)
{
    if (sender.HasErrors)
    {
        UpdateInputValidationErrors(control, sender.GetErrors(propertyName));
    }
}

But here, sender.HasErrors is false now, because we just validated the last incorrect property. So this method just returns and the UI is not updated at all, which explains why we're seeing that behavior in the last field being modified.

This is what makes me say it looks like a XAML codegen issue. It seems to me that there should at least be another branch in UpdateErrors_ that just clears all the errors in the target control without doing other checks in case the linked viewmodel has no errors left anymore.

[StephenLPeters]

@Sergio0694 is this issue in Winui2 and/or Winui3?

[Sergio0694]

@StephenLPeters The repro that was provided to me from the Windows Template Studio (linked in my post above) is using a packaged Win32 WinUI 3 app. Haven't tried to repro this with UWP WinUI 2 yet since I know there's no official support for INotifyDataErrorInfo there anyway.

[StephenLPeters]

@MikeHillberg or @Austin-Lamb what is the state of data validation in winui3?

[michael-hawker]

FYI @stevenbrix as well.

I know the Input Validation is still listed as 'preview' on the new roadmap for WinUI 3, but if this is indeed a simple bug fix, it'd be great to see if the generation could be patched.

Windows Template Studio is trying to show this as an example in their WinUI templates on top of our new .NET Standard based MVVM Toolkit in the Windows Community Toolkit; so it'd be great if we'd have that working as intended and be able to gather more feedback from the community for this feature as it moves through it's preview phase. 🙂

[mrlacey]

To prove this isn't an issue with WIindows Community Toolkit, or Martin's Template Studio code, here's a minimal repro which uses neither:

Using

• WinUI Preview 3 (Microsoft.WinUI 3.0.0-preview3.201113.0)
• Visual Studio 16.8.4

Steps to reproduce the bug

1. Create a new Blank UWP app
2. Replace the contents of MainPage.xaml with

    <StackPanel Orientation="Vertical" HorizontalAlignment="Center" VerticalAlignment="Center">
        <TextBox Header="First Name"
                 MinWidth="200"
                 Text="{x:Bind ViewModel.FirstName, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" />
        <TextBox Header="Last Name"
                 MinWidth="200"
                 Text="{x:Bind ViewModel.LastName, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" />
    </StackPanel>

3. Replace MainPage.xaml.cs with

    public sealed partial class MainPage : Page
    {
        public MainViewModel ViewModel { get; set; }

        public MainPage()
        {
            ViewModel = new MainViewModel();
            this.InitializeComponent();
        }
    }

4. Add the following class

    public class MainViewModel : INotifyPropertyChanged, INotifyDataErrorInfo
    {
        private string firstName;
        private string lastName;

        public string FirstName
        {
            get => firstName;
            set
            {
                firstName = value;
                PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(FirstName)));
                ErrorsChanged(this, new DataErrorsChangedEventArgs(nameof(LastName)));
            }
        }
        public string LastName
        {
            get => lastName;
            set
            {
                lastName = value;
                PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(LastName)));
                ErrorsChanged(this, new DataErrorsChangedEventArgs(nameof(LastName)));
            }
        }

        public event PropertyChangedEventHandler PropertyChanged;

        public IEnumerable<object> GetErrors(string propertyName)
        {
            switch (propertyName)
            {
                case "FirstName":
                    if (string.IsNullOrWhiteSpace(FirstName))
                    {
                        yield return "FirstName cannot be blank";
                    }
                    break;
                case "LastName":
                    if (string.IsNullOrWhiteSpace(LastName))
                    {
                        yield return "LastName cannot be blank";
                    }
                    break;
                default:
                    break;
            }
        }

        public bool HasErrors => string.IsNullOrWhiteSpace(FirstName) || string.IsNullOrWhiteSpace(LastName);

        public event EventHandler<DataErrorsChangedEventArgs> ErrorsChanged;
    }

5. Run the app.
6. Put focus in the "First Name" textbox.
7. Discover that there's no way to get rid of the error warning in the second (Last Name) textbox.

(Only tested with UWP on Desktop)

[mrlacey]

(Only tested with UWP on Desktop [PC])

Actually, (thanks to the power of a minimal repro,) I can confirm that the same behavior is seen in a WinUI for Desktop app.

The above repro needs to following modifications for Desktop:

• MainWindow, not MainPage
• In MainViewModel the signature of GetErrors becomes public IEnumerable GetErrors(string propertyName)

[huoyaoyuan]

I'm also hitting this. I used my custom implementation and implementation in WCTK to prove it isn't implementation issue of INotifyDataErrorInfo.

[stevenbrix]

Thanks all for the input here. IIRC, we took the design based off the way the WPF Binding engine would interact with the INDEI. The Binding code doesn't have intelligence to know which errors were removed, so the source of the errors needs to clear them every time before running validation logic. @mrlacey I don't see the implementation of the ErrorsChanged method in your example, is it doing this?

[mnikonov]

Also having this issue :(

[Sergio0694]

Hey @stevenbrix, can you clarify on what you mean here?

"we took the design based off the way the WPF Binding engine would interact with the INDEI. The Binding code doesn't have intelligence to know which errors were removed, so the source of the errors needs to clear them every time before running validation logic."

As in, considering that types implementing INDEI would just have to respect the contract defined by the interface, I'm not sure I understand what exactly needed (needs?) to be done to specifically account for WPF's binding engine with respect to this 🙂

I guess I'm confused because from what I can see here, regardless of the reason why the current implementation of the XAML compiler ended up being what we have today, the final codegen just seems incorrect to me. As in, to recap, we have this:

private void UpdateErrors_(global::Microsoft.UI.Xaml.Controls.Control control, global::System.ComponentModel.INotifyDataErrorInfo sender, string propertyName)
{
    if (sender.HasErrors)
    {
        UpdateInputValidationErrors(control, sender.GetErrors(propertyName));
    }
}

Which is generated for all individual controls that bind to a property from a viewmodel with INDEI. The issue is that when the last property becomes valid, this branch is not taken because the viewmodel itself no longer has errors, so the UI is never updated and just goes out of sync. There are no errors left in the viewmode, yet the last control was never refreshed and keeps displaying stale errors. What I'm saying is this just seems like a codegen issue regardless of the original motivation 🤔

Just as an example, either of these two options would fix the issue already:

// Option 1, just remove the branch entirely. When the property no longer
// has errors, the control just receives an empty list of errors and refreshes.
UpdateInputValidationErrors(control, sender.GetErrors(propertyName));

OR:

if (sender.HasErrors)
{
    UpdateInputValidationErrors(control, sender.GetErrors(propertyName));
}
else
{
    // Option 2, fast path to just clear all validation errors from a control
    ClearInputValidationErrors(control);
}

Thoughts?
I lack context on the rest of the XAML compiler infrastructure to know how expensive it would be to actually implement this change, but at least from here it seems not too expensive, and it'd be nice if this was at least fixed in a new WinUI 3 release, so that developers using either the MVVM Toolkit or other MVVM libraries would finally be able to use INDEI with modern apps too 😄

[stevenbrix]

Gotcha, thanks for your patience, I'm finally there :)

I agree that this if (sender.HasErrors) check could just be removed. Removing the check would be my vote, because the UpdateInputValidationErrors method will already do the right thing.

[mrlacey]

@mrlacey I don't see the implementation of the ErrorsChanged method in your example, is it doing this?

Just closing the loop here. Yes, I see the same in the generated code with my repro.
With a WinUI for Desktop app, in the generated code:

• MainWindow_obj1_Bindings.Initialize calls MainWindow_obj1_Bindings.Update
• MainWindow_obj1_Bindings.Update calls MainWindow_obj1_Bindings.Update_
• MainWindow_obj1_Bindings.Update_ call MainWindow_obj1_Bindings.Update_ViewModel
• MainWindow_obj1_Bindings.Updat_ViewModel calls bindingsTracking.UpdateChildListeners_ViewModel
• bindingsTracking.UpdateChildListeners_ViewModel subscribes to PropertyChanged_ViewModel and ErrorsChanged_ViewModel
• ErrorsChanged_ViewModel calls bindings.UpdateErrors_ViewModel
• bindings.UpdateErrors_ViewModel calls UpdateErrors_
• UpdateErrros_ looks like this:

private void UpdateErrors_(global::Microsoft.UI.Xaml.Controls.Control control, global::System.ComponentModel.INotifyDataErrorInfo sender, string propertyName)
{
    if (sender.HasErrors)
    {
        UpdateInputValidationErrors(control, sender.GetErrors(propertyName));
    }
}

which is the same as Sergio showed above

[Sergio0694]

@stevenbrix Awesome, glad we're all on the same page now 😄

Removing that if (sender.HasErrors) check does seem to be the easiest solution, yeah (just mentioned the other alternative too for completeness, but I agree that just removing the check would be easier and still work just fine). Do you think there's a chance such a change could make it into the production-ready WinUI release that's going to be published next month? Assuming implementing this fix isn't too complex, it'd be nice to actually have a working INDEI framework support from the start with WinUI 3, considering all the interest in this area by so many devs especially in LoB scenarios. Without that, all consumers trying to use ObservableValidator from the MVVM Toolkit or an equivalent custom class would just end up facing the same issue otherwise.

[michael-hawker]

FYI @MikeHillberg, this should be a simple fix to enable customers to provide feedback on this feature; otherwise it's broken.

[RealTommyKlein]

Sorry for the delayed response on this (and thanks @azchohfi for pinging me!) , but the fix is checked in for the upcoming Reunion 1.0 fall release.