Xamarin.forms: [Bug] string.Empty causes InvalidOperationException($"Cannot convert \"{value}\" into {typeof(Color)}")

[ChaseFlorell]

Description

ColorTypeConverter throws InvalidOperationException on an empty string

Steps to Reproduce

1. Add a Boxview to a page and set the color to {Binding MyColor}
2. Add a MyColor property to your viewmodel public string MyColor { get;set; } = "";
3. Run

Expected Behavior

App should not crash

Actual Behavior

App crashes with InvalidOperationException
https://github.com/xamarin/Xamarin.Forms/blob/6b827ac010d52203a0653a00b8b736c9a3bbcf1e/Xamarin.Forms.Core/ColorTypeConverter.cs#L281

Basic Information

• Version with issue: 4.6.0.529-pre3
• Last known good version: unknown
• IDE: Rider
• Platform Target Frameworks:
  
  
  
  • iOS:
  
  
  • Android: 10.0
  
  
  • UWP:
  
  
• Android Support Library Version:
• Nuget Packages:
• Affected Devices:
  Google Pixel 4XL
  Samsung Galaxy S8

Screenshots

Reproduction Link

https://github.com/ChaseFlorell/ColorConverterCrash

Workaround

You need to attach a Converter to the binding and handle the empty string.

[ChaseFlorell]

This is likely something I can contribute a fix for, just need the go-ahead

[jsuarezruiz]

Setting a color string (Example: #FFFFFF), ColorTypeConverter will convert it https://github.com/xamarin/Xamarin.Forms/blob/6b827ac010d52203a0653a00b8b736c9a3bbcf1e/Xamarin.Forms.Core/ColorTypeConverter.cs#L21

What would you like to modify? Validate empty string here https://github.com/xamarin/Xamarin.Forms/blob/6b827ac010d52203a0653a00b8b736c9a3bbcf1e/Xamarin.Forms.Core/ColorTypeConverter.cs#L23?

[ChaseFlorell]

That is correct, I'd like to change the null check to

if(!string.IsNullOrWhitespace(value))

The rational for me is that we have a back end system that stores an unset color to an empty string, and therefore currently requires me to add a custom converter to the value everywhere it's used in order to prevent a crash.

[jsuarezruiz]

It is a simple change but it also involves breaking changes. It can affect those who handle the exception to manage these cases, etc. You could use a fallback value in the binding or a converter to handle the empty string case.

[ChaseFlorell]

A fallback value doesn't catch the exception, we still get a hard crash as unable to convert "" to a Color.

We have a converter now that does the job, but it's a pile of unnecessary code every time we want to use a color out of the database.

What would it take to make it so that null and "" both leverage the Fallback value, and if there is no fallback value, just don't do anything? That would make more sense.

I think for the people who are trapping that exception now, like us, will welcome the fix so that we don't have to add so much verbosity to our xaml.

<BoxView Color="{Binding SeedColour, Converter={StaticResource ColorConverter}, ConverterParameter={StaticResource White}}" />

[ChaseFlorell]

It can affect those who handle the exception to manage these cases, etc.

So you want to leave the scenario in place so that users who have had to build custom converters to work around it are protected? I don't follow this train of thought. Our app has a custom converter to protect us from the crash, I'd much rather be able to remove this converter.

If you really do think it needs to stay (I wholeheartedly disagree), then at least make it so that the FallbackValue can be picked up, and if there is no FallbackValue, then (and this makes me sad) throw the exception.

Please see my attached repro to see the crash.

[nickrandolph]

What I don't understand about the response here is the fact that when the bound value returns a valid response (eg empty string, since it's a string property) that the converter throws an exception.... which in theory you can handle but it's buried within the xaml resource loading. The suggested fix by @ChaseFlorell deals with this oversight and would be a welcome solution to this issue.

"It's a breaking change" isn't an answer as to whether this should be done or not, it just governs which version it gets release in. If it truly is considered a breaking change (versus just fixing a bug) then it just needs to go into the appropriate version increment.

As an aside, @ChaseFlorell is trying to fix the type of bug that plagues new developers working with XAML and XF - the fact that bindings can just crash the application for seemingly no apparent reason is such a common source of frustration for new XAML developers. Every effort should be made to improve XF to the point where these sources of frustration don't exist until you're really pushing the boundaries of what's possible with XF. This is clearly not one of those cases.

@ChaseFlorell my suggestion would be to PR the fix for this and then let the community drive whether the PR should be accepted or not.

[ChaseFlorell]

https://twitter.com/ChaseFlorell/status/1253699034356649985

[StephaneDelcroix]

@ChaseFlorell: string.Empty is not a valid color, so I think throwing an exception is the right thing to do here.

[StephaneDelcroix]

also An extremely popular framework that you use has a bug that causes a hard crash. Thanks for the extremely popular, but I don't see this as a bug ?

The rational for me is that we have a back end system that stores an unset color to an empty string . I get that. But in a MVVM model, you can, but you're not forced, to exposed the Model properties directly into the ViewModel.

I'm closing this, but if you can make a good argument that it's really a bug, we'll consider reopening

[ChaseFlorell]

@StephaneDelcroix I disagree, null is not a valid color either, and it doesn't throw, I think you should _either_ be returning the same value as null _or_ using the Fallback value.

A hard shutdown is not helpful to anyone here; and having to write a converter to work around it is really not the best approach.

[StephaneDelcroix]

oh, you're totally right, the app shouldn't crash on a binding failure. I misread that. binding errors should be caught and logged (but that doesn't mean we will accept "" as Color)