Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ColorPickerButton Binding errors #4721

Closed
2 of 14 tasks
HavenDV opened this issue Aug 12, 2022 · 6 comments · Fixed by #4729 or #4730
Closed
2 of 14 tasks

ColorPickerButton Binding errors #4721

HavenDV opened this issue Aug 12, 2022 · 6 comments · Fixed by #4729 or #4730
Assignees
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥 controls 🎛️ WinUI 💠 Related to WinUI 3 Version or when paired with External can mean requires fix in WinUI 2/3.
Milestone

Comments

@HavenDV
Copy link
Contributor

HavenDV commented Aug 12, 2022

Describe the bug

<controls:ColorPickerButton SelectedColor="{x:Bind ViewModel.CustomColor, Mode=TwoWay, Converter={StaticResource SystemDrawingColorToColorConverter}}"/>

I am getting the following error messages:
However, everything seems to work.

What's the converter code you're using? It seems like SelectedColor is probably null to start with, getting passed in to your converter, which is throwing an exception (and caught by the XAML stack)?
Check the output window for any thrown Exceptions that might not be enabled in Visual Studio's exception settings.
If all else fails, create/use an empty converter and put a breakpoint in it to see what the actual value being passed is
Target confuses me, as if it's some kind of internal converter.

Error: Converter failed to convert value of type 'null' to type 'Color'; BindingExpression: Path='Color' DataItem='CommunityToolkit.WinUI.UI.Controls.ColorPicker'; target element is 'Microsoft.UI.Xaml.Controls.GridView' (Name='null'); target property is 'SelectedValue' (type 'Object'). 
Error: Converter failed to convert value of type 'null' to type 'Color'; BindingExpression: Path='Color' DataItem='CommunityToolkit.WinUI.UI.Controls.ColorPicker'; target element is 'Microsoft.UI.Xaml.Controls.GridView' (Name='null'); target property is 'SelectedValue' (type 'Object'). 

These errors occur at the moment the button is clicked, at this moment the converter is not called.
This is preceded by a call to IValueConverter.Convert with a non-null value, as is the initial initialization.

I wasn't able to check the values via ColorToDisplayNameConverter because the symbols seem to be missing
image
image
image
While not likely related to this package, I can see pdb files inside. Most likely, I'm doing something wrong, because debugging only works for <DebugType>embedded</DebugType> libraries for me.

Regression

No response

Reproducible in sample app?

  • This bug can be reproduced in the sample app.

Steps to reproduce

<controls:ColorPickerButton SelectedColor="{x:Bind ViewModel.CustomColor, Mode=TwoWay, Converter={StaticResource SystemDrawingColorToColorConverter}}"/>

Expected behavior

Without errors

Screenshots

2022-08-12_12-16-59.mp4

No response

Windows Build Number

  • Windows 10 1809 (Build 17763)
  • Windows 10 1903 (Build 18362)
  • Windows 10 1909 (Build 18363)
  • Windows 10 2004 (Build 19041)
  • Windows 10 20H2 (Build 19042)
  • Windows 10 21H1 (Build 19043)
  • Windows 11 21H2 (Build 22000)
  • Other (specify)

Other Windows Build number

No response

App minimum and target SDK version

  • Windows 10, version 1809 (Build 17763)
  • Windows 10, version 1903 (Build 18362)
  • Windows 10, version 1909 (Build 18363)
  • Windows 10, version 2004 (Build 19041)
  • Other (specify)

Other SDK version

No response

Visual Studio Version

2022

Visual Studio Build Number

17.3

Device form factor

No response

Nuget packages

7.1.2

Additional context

Related code:
https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/main/Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/ColorPicker.xaml#L156-L181
https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/main/Microsoft.Toolkit.Uwp.UI/Converters/ColorToDisplayNameConverter.cs

Help us help you

Yes, I'd like to be assigned to work on this item.

@HavenDV HavenDV added the bug 🐛 An unexpected issue that highlights incorrect behavior label Aug 12, 2022
@ghost ghost added the needs triage 🔍 label Aug 12, 2022
@ghost
Copy link

ghost commented Aug 12, 2022

Hello HavenDV, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

@michael-hawker michael-hawker added this to the 7.1.3 milestone Aug 15, 2022
@michael-hawker michael-hawker added controls 🎛️ WinUI 💠 Related to WinUI 3 Version or when paired with External can mean requires fix in WinUI 2/3. and removed needs triage 🔍 labels Aug 15, 2022
@michael-hawker
Copy link
Member

@HavenDV I think you're saying there may be an issue with the initialization/converter being used in the template of the control?

Do you know if this is happening in UWP as well or just WinUI 3? It'd be great if we could define a test case in our unit tests which can replicate this issue and then find a solution.

I'll assign to you to take a look at. If we can find a solution in the next week/week-and-a-half, we should be able to include in our upcoming hotfix release.

(As for the PDB Debugging issue see dotnet/sdk#1458)

@HavenDV
Copy link
Contributor Author

HavenDV commented Aug 18, 2022

Yes, the problem manifested itself in both WinUI and UWP. SampleApp also shows this issue:
image

As I understand it, I can add validation directly to existing test:
https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/main/UITests/UITests.Tests.Shared/Controls/ColorPickerButtonTest.cs
The only problem is that I do not understand how I can find out if there are Binding Failures at the moment a debugger must be attached to an application to be able to send a binding failure via DebugSettings.BindingFailed += (_, args) => Log.Error($"Binding failed: {args.Message}");
https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.debugsettings.bindingfailed?view=winrt-22621

@michael-hawker
Copy link
Member

@HavenDV the Page part of the UITests happen at the app level, so you could have the page register to the event and change some element on the page based on success/failure. Then in the test file side just look for that element to have changed after invoking the change in the test to see the result as a simple form of communication between the two sides.

For the sample app, that's just running the sample app locally with the existing sample (no changes) to exhibit the issue, eh?

@michael-hawker michael-hawker moved this from Done to In Progress in Windows Community Toolkit Aug 19, 2022
@HavenDV
Copy link
Contributor Author

HavenDV commented Aug 19, 2022

Yes, the problem is present in the sample application, you only need to have a debugger attached to see it.
The same goes for tests. The DebugSettings.BindingFailed event won't fire without a debugger attached to the app (not tests), which is why I didn't add it to the PR

@robloo
Copy link
Contributor

robloo commented Aug 20, 2022

I confirmed this issue in the SampleApp.

image

As stated, it is caused by the two-way binding of a nullable GridView.SelectedValue with the non-nullable ColorPicker.Color property. Going from ColorPicker -> GridView is fine but the convert back when there is no selection will attempt to set null to the Color property.

This isn't a critical issue but the binding failure shouldn't happen.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥 controls 🎛️ WinUI 💠 Related to WinUI 3 Version or when paired with External can mean requires fix in WinUI 2/3.
Projects
Status: Done
3 participants