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

[XC] Fix x:DataType resolution for BindingContext #21454

Merged
merged 2 commits into from
Apr 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions src/Controls/src/Build.Tasks/SetPropertiesVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -382,14 +382,26 @@ static IEnumerable<Instruction> CompileBindingPath(ElementNode node, ILContext c

INode dataTypeNode = null;
IElementNode n = node;

// Special handling for BindingContext={Binding ...}
// The order of checks is:
// - x:DataType on the binding itself
// - SKIP looking for x:DataType on the parent
// - continue looking for x:DataType on the parent's parent...
IElementNode skipNode = null;
if (IsBindingContextBinding(node))
{
skipNode = GetParent(node);
}

while (n != null)
{
if (n.Properties.TryGetValue(XmlName.xDataType, out dataTypeNode))
if (n != skipNode && n.Properties.TryGetValue(XmlName.xDataType, out dataTypeNode))
{
break;
if (n.Parent is ListNode listNode)
n = listNode.Parent as IElementNode;
else
n = n.Parent as IElementNode;
}

n = GetParent(n);
}

if (dataTypeNode is null)
Expand Down Expand Up @@ -486,6 +498,25 @@ static IEnumerable<Instruction> CompileBindingPath(ElementNode node, ILContext c
yield return Create(Ldnull);
yield return Create(Newobj, module.ImportReference(ctorinforef));
yield return Create(Callvirt, module.ImportPropertySetterReference(context.Cache, bindingExtensionType, propertyName: "TypedBinding"));

static IElementNode GetParent(IElementNode node)
{
return node switch
{
{ Parent: ListNode { Parent: IElementNode parentNode } } => parentNode,
{ Parent: IElementNode parentNode } => parentNode,
_ => null,
};
}

static bool IsBindingContextBinding(ElementNode node)
{
// looking for BindingContext="{Binding ...}"
return GetParent(node) is IElementNode parentNode
&& ApplyPropertiesVisitor.TryGetPropertyName(node, parentNode, out var propertyName)
&& propertyName.NamespaceURI == ""
&& propertyName.LocalName == nameof(BindableObject.BindingContext);
}
}

static IList<(PropertyDefinition property, TypeReference propDeclTypeRef, string indexArg)> ParsePath(ILContext context, string path, TypeReference tSourceRef, IXmlLineInfo lineInfo, ModuleDefinition module)
Expand Down
54 changes: 54 additions & 0 deletions src/Controls/tests/Xaml.UnitTests/Issues/Maui21434.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
using System;
using Microsoft.Maui.ApplicationModel;
using Microsoft.Maui.Controls.Core.UnitTests;
using Microsoft.Maui.Dispatching;

using Microsoft.Maui.UnitTests;
using NUnit.Framework;

namespace Microsoft.Maui.Controls.Xaml.UnitTests;

public partial class Maui21434
{
public Maui21434()
{
InitializeComponent();
}

public Maui21434(bool useCompiledXaml)
{
//this stub will be replaced at compile time
}

[TestFixture]
class Test
{
[SetUp]
public void Setup()
{
Application.SetCurrentApplication(new MockApplication());
DispatcherProvider.SetCurrent(new DispatcherProviderStub());
}

[TearDown] public void TearDown() => AppInfo.SetCurrent(null);

[Test]
public void BindingsDoNotResolveStaticProperties([Values(false, true)] bool useCompiledXaml)
{
var page = new Maui21434(useCompiledXaml);
Assert.That(page.ParentTextLabel?.Text, Is.EqualTo("ParentText"));
Assert.That(page.ChildTextLabel?.Text, Is.EqualTo("ChildText"));
}
}
}

public class ParentViewModel21434
{
public string Text => "ParentText";
public ChildViewModel21434 Child { get; } = new();
}

public class ChildViewModel21434
{
public string Text => "ChildText";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use a different property name than the one on the parent, to see if there's a compilation error (should not)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original issue had the same property name, so I stuck to that in the test. Before the fix when the property names were equal there was no compilation error, but the value displayed in the Label was incorrect (there was a mismatch between the TSource of the binding and the context object type it was applied with). It might be worth testing both cases.

}
15 changes: 15 additions & 0 deletions src/Controls/tests/Xaml.UnitTests/Issues/Maui21434.xaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?xml version="1.0" encoding="UTF-8"?>
<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
xmlns:local="clr-namespace:Microsoft.Maui.Controls.Xaml.UnitTests"
x:Class="Microsoft.Maui.Controls.Xaml.UnitTests.Maui21434"
x:DataType="local:ParentViewModel21434">
<ContentPage.BindingContext>
<local:ParentViewModel21434 />
</ContentPage.BindingContext>

<VerticalStackLayout>
<Label Text="{Binding Text}" x:Name="ParentTextLabel" />
<Label BindingContext="{Binding Child}" Text="{Binding Text}" x:DataType="local:ChildViewModel21434" x:Name="ChildTextLabel" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we be even smarter, and infer the x:DataType of the label ? not in all cases, but in this one it looks like we could...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could, I opened a new issue so that we can discuss all the cases when we could infer x:DataTypes (#21834)

</VerticalStackLayout>
</ContentPage>
Loading