Bug 52484 - XAML editor allows incorrect syntax when setting element properties
Summary: XAML editor allows incorrect syntax when setting element properties
Status: RESOLVED ANSWERED
Alias: None
Product: Forms
Classification: Xamarin
Component: Forms ()
Version: 2.3.4
Hardware: PC Windows
: --- normal
Target Milestone: ---
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2017-02-13 21:14 UTC by Rob Stoffers
Modified: 2017-06-21 14:05 UTC (History)
4 users (show)

Tags: xaml debug element
Is this bug a regression?: ---
Last known good build:


Attachments
sample solution showing the issue (81.89 KB, application/x-zip-compressed)
2017-02-13 21:14 UTC, Rob Stoffers
Details
updated sample showing slightly different scenario (96.86 KB, application/x-zip-compressed)
2017-02-18 00:17 UTC, Rob Stoffers
Details
Image showing the result (76.98 KB, image/png)
2017-02-18 00:23 UTC, Rob Stoffers
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED ANSWERED

Description Rob Stoffers 2017-02-13 21:14:09 UTC
Created attachment 19804 [details]
sample solution showing the issue

If you have the following classes...:

public class Test1 : View
{
    public static readonly BindableProperty BrushProperty =
        BindableProperty.Create("Brush", typeof(Xamarin.Forms.Color), typeof(Test1), Color.Green);

    public Xamarin.Forms.Color Brush
    {
        get { return (Xamarin.Forms.Color)this.GetValue(BrushProperty); }
        set { this.SetValue(BrushProperty, value); }
    }
}

public class Test2 : View
{
    public static readonly BindableProperty BrushProperty =
        BindableProperty.Create("Brush", typeof(Xamarin.Forms.Color), typeof(Test2), Color.Green);

    public Xamarin.Forms.Color Brush
    {
        get { return (Xamarin.Forms.Color)this.GetValue(BrushProperty); }
        set { this.SetValue(BrushProperty, value); }
    }
}

... and then try to set the Brush property in XAML like so...:

<local:Test1>
    <local:Test2.Brush>#FF0000</local:Test2.Brush>
</local:Test1>

... the project builds and runs just fine although the Brush property doesn't get set to red which is expected.  What I don't think is expected is that the project builds.  There should be a build error present letting developers know that this syntax won't work.  In WPF it displays something like "the attached property Test2.Brush is not defined on Test1 or one of its base classes".

I have attached a sample solution showing this behavior in both Xamarin.Forms and WPF.  Make sure to do a nuget restore after opening the solution.
Comment 1 Rob Stoffers 2017-02-18 00:17:13 UTC
Created attachment 19872 [details]
updated sample showing slightly different scenario
Comment 2 Rob Stoffers 2017-02-18 00:19:24 UTC
I attached an update sample that shows a different spin on this.  The XAML parser seems perfectly happy to accept elements that don't even have a Brush property on them in order to set the main elements Brush property.  I.e.:

<local:ViewWithBrush x:Name="viewWithBrushProperty1">
	<local:ViewWithoutBrush.Brush>#FF0000</local:ViewWithoutBrush.Brush>
</local:ViewWithBrush>

ViewWithoutBrush doesn't have a Brush property but it doesn't complain and it even goes and sets the Brush property that does exist on ViewWithBrush.  Does the parser not care about what the element name is before the property you are trying to set?  Seems a little weird.
Comment 3 Rob Stoffers 2017-02-18 00:23:09 UTC
Created attachment 19873 [details]
Image showing the result

This image shows that it accepts the invalid XAML and updates the top element's Brush property.  The Brush is being used to update the views BackgroundColor in the OnBrushChanged property changed handler.
Comment 4 Paul DiPietro [MSFT] 2017-02-23 21:05:19 UTC
Thank you for the reproductions. I'm not familiar enough with the XAML implementation to know if how this acts is intended, but will see if I can't get a better understanding. I'll set this to assigned to acknowledge this is being looked into until I get some more information.
Comment 5 Stephane Delcroix 2017-06-21 14:05:19 UTC
This is possible because the core of Xamarin.Forms currently doesn't make any difference between 'normal' BindableProperties and 'attached' ones.

The property Test2.BrushProperty is actually set on Test1, and could be retrieved using `GetValue()`.

Fixing this would be a breaking change for all legacy code, and for everyone creating an attached BP using Create() instead of CreateAttached().

We could Log a warning on the console at runtime when this is misused, and, 
with XamlC on, we could issue a warning at compilation time. But for the time being, I prefer to close this.