Fix OnPlatform + Setter when no match for current platform #17061
Conversation
When OnPlatforfm is used for a Setter value, and there's no match for the current platform, fix so that the property isn't set (same behavior as Xamarin.Forms). Previously, XamlC would report a "Missing Value For Setter" error when this happens and non-compiled XAML would crash with an NRE. This PR contains fixes for both, ensuring the XAML compiler doesn't show an error, then ensuring it doesn't crash at runtime. Fixes #12064
BretJohnson
requested review from
jsuarezruiz,
rachelkang and
StephaneDelcroix
|
I think this needs to be a XAML test as well so that we can ensure the XamlC and XamlG code is the same. Maybe this file: https://github.com/dotnet/maui/blob/main/src/Controls/tests/Xaml.UnitTests/OnPlatformTests.cs |
|
What do you mean by a XAML test? You mean a non compiled XAML test in addition to the compiled XAML test that's there currently? |
|
Oops, I linked the wrong file, but there are 2 types of xaml tests - and this is the one that I was actually talking about: https://github.com/dotnet/maui/blob/main/src/Controls/tests/Xaml.UnitTests/OnPlatform.xaml.cs |
|
You may just be able to add to that test... It seems to have a bunch of things you are doing. |
| { | ||
| public Issue12604() | ||
| { | ||
| InitializeComponent(); |
There was a problem hiding this comment.
please look at the other tests, to see how we test for both runtime and compiled Xaml
| if (valueNode == null) | ||
| throw new XamlParseException("Missing Value for Setter", (IXmlLineInfo)node); | ||
| yield break; |
There was a problem hiding this comment.
I'm afraid this will have effects outside of OnPlatform
There was a problem hiding this comment.
@StephaneDelcroix - Yes, this change can have effects outside of OnPlatform, but previously those cases failed with the exception here, so the risk of breaking anything, making things worse, seems small - it previously just error'ed out. That said, if you can tell me how this code is actually used outside of OnPlatform, like suggest some test scenarios, I can test those.
There was a problem hiding this comment.
The property would not be set, right?. So for example, here:
<Style x:Key="ButtonStyle0" TargetType="Button">
<Setter Property="FontSize" Value="{OnPlatform iOS=36}" />
</Style>
The FontSize property on Android, would have the default value?
I updated, moving all my tests there, which also covers testing compiled and non-compiled XAML. |
|
This is ready to merge, from my perspective, once @StephaneDelcroix approves (or suggests additional test cases). |
|
@StephaneDelcroix ping on this; can you take a look; thanks |
When OnPlatforfm is used for a Setter value, and there's no match for the current platform, fix so that the property isn't set (same behavior as Xamarin.Forms). Previously, XamlC would report a "Missing Value For Setter" error when this happens and non-compiled XAML would crash with an NRE. This PR contains fixes for both, ensuring the XAML compiler doesn't show an error, then ensuring it doesn't crash at runtime.
Fixes #12064