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

TemplateBinding with CornerRadius on Border does not work anymore #21747

Closed
WMLPB opened this issue Apr 10, 2024 · 12 comments · Fixed by #22800
Closed

TemplateBinding with CornerRadius on Border does not work anymore #21747

WMLPB opened this issue Apr 10, 2024 · 12 comments · Fixed by #22800
Assignees
Labels
area-controls-button Button, ImageButton area-xaml XAML, CSS, Triggers, Behaviors fixed-in-8.0.60 fixed-in-9.0.0-preview.5.24307.10 i/regression This issue described a confirmed regression on a currently supported version platform/android 🤖 s/needs-attention Issue has more information and needs another look t/bug Something isn't working
Milestone

Comments

@WMLPB
Copy link

WMLPB commented Apr 10, 2024

Description

Hi all,

I use my own Button control inherited from TemplatedView; with a CornerRadius property.

public static readonly BindableProperty CornerRadiusProperty =
      BindableProperty.Create(nameof(CornerRadius), typeof(CornerRadius), typeof(Button), new CornerRadius(6.0));

In the button’s ControlTemplate I have a Border with a RoundRectangle StrokeShape and a TemplateBinding to the before mentioned CornerRadius property.

<Border.StrokeShape>
      <RoundRectangle CornerRadius="{TemplateBinding CornerRadius}" />
</Border.StrokeShape>

And I have a Style to set the CornerRadius property.

<Style TargetType="e:Button" ApplyToDerivedTypes="True">
      <Setter Property="CornerRadius" Value="6,6,6,6" />
      <Setter Property="ControlTemplate" Value="{StaticResource ButtonTemplate}" />
</Style>

After updating to MAUI 8.0.20 this doesn’t work anymore – tested on Android. The button in the UI doesn’t have round corners anymore. If I remove the TemplateBinding and set the CornerRadius in the ControlTemplate directly, it works again.

<Border.StrokeShape>
      <RoundRectangle CornerRadius="6,6,6,6" />
</Border.StrokeShape>

I can live with this work around for the moment. Thank you very much for reading.

Steps to Reproduce

No response

Link to public reproduction project repository

No response

Version with bug

8.0.20 SR4

Is this a regression from previous behavior?

Yes, this used to work in .NET MAUI

Last version that worked well

8.0.14 SR3.1

Affected platforms

Android

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

No response

@WMLPB WMLPB added the t/bug Something isn't working label Apr 10, 2024
@PureWeen PureWeen added this to the .NET 8 SR4 milestone Apr 10, 2024
@PureWeen PureWeen added the area-controls-button Button, ImageButton label Apr 10, 2024
@dotnet-policy-service dotnet-policy-service bot added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Apr 10, 2024
@PureWeen PureWeen added the s/needs-repro Attach a solution or code which reproduces the issue label Apr 10, 2024
@dotnet-policy-service dotnet-policy-service bot added the s/no-recent-activity Issue has had no recent activity label Apr 15, 2024
@omghb
Copy link

omghb commented Apr 15, 2024

We run into the exact same issue that our TemplateBinding for a Border.CornerRadius does not work anymore after updating MAUI to version 8.0.20.

Regression issue:

  • Version it still worked: 8.0.14
  • Version this got broken: 8.0.20

We see this issue on all our supported platforms

  • Android (v14)
  • iOS (v17)
  • Windows (v10)

@dotnet-policy-service dotnet-policy-service bot added s/needs-attention Issue has more information and needs another look and removed s/needs-repro Attach a solution or code which reproduces the issue s/no-recent-activity Issue has had no recent activity labels Apr 15, 2024
@Redth Redth added the potential-regression This issue described a possible regression on a currently supported version., verification pending label Apr 15, 2024
@PureWeen
Copy link
Member

@omghb can you please attach a repro?

@mattleibow
Copy link
Member

mattleibow commented Apr 15, 2024

I managed to repro this, but looks to be the Border.CornerRadius specifically. I also tested with a label Text:

<VerticalStackLayout>
    <local:MyButton CornerRadius="10,10,10,10" Text="New Text">
        <local:MyButton.ControlTemplate>
            <ControlTemplate>
                <Border Stroke="#C49B33"
                        StrokeThickness="4"
                        Background="#2B0B98"
                        Padding="16,8"
                        HorizontalOptions="Center">
                    <Border.StrokeShape>
                        <RoundRectangle CornerRadius="{TemplateBinding CornerRadius}" />
                    </Border.StrokeShape>
                    <Label Text="{TemplateBinding Text}"
                        TextColor="White"
                        FontSize="24"
                        FontAttributes="Bold" />
                </Border>
            </ControlTemplate>
        </local:MyButton.ControlTemplate>
    </local:MyButton>
</VerticalStackLayout>
public class MyButton : TemplatedView
{
	public static readonly BindableProperty CornerRadiusProperty = BindableProperty.Create(
		nameof(CornerRadius), typeof(CornerRadius), typeof(MyButton), new CornerRadius(6.0),
		propertyChanged: (bindable, oldValue, newValue) =>
		{
			Debug.WriteLine($"CornerRadius changed from '{oldValue}' to '{newValue}'");
		});

	public static readonly BindableProperty TextProperty = BindableProperty.Create(
		nameof(Text), typeof(string), typeof(MyButton), "Default",
		propertyChanged: (bindable, oldValue, newValue) =>
		{
			Debug.WriteLine($"Text changed from '{oldValue}' to '{newValue}'");
		});

	public CornerRadius CornerRadius
	{
		get => (CornerRadius)GetValue(CornerRadiusProperty);
		set => SetValue(CornerRadiusProperty, value);
	}

	public string Text
	{
		get => (string)GetValue(TextProperty);
		set => SetValue(TextProperty, value);
	}
}

I see the text is applied, and the events for BOTH properties are firing. So, something is not moving from the property to the UI.

image

@mattleibow
Copy link
Member

If I make the entire StrokeShape property a template binding, it also works.

@mattleibow
Copy link
Member

mattleibow commented Apr 15, 2024

Looks like this change: https://github.com/dotnet/maui/pull/21151/files#r1523373115

Seems that we used to:

AddLogicalChild(visualElement);

Now we

SetInheritedBindingContext(visualElement, BindingContext);

This makes sense as the stroke is not meant to be a "child" of the border, but rather is just a set of values.

@mattleibow mattleibow added the area-xaml XAML, CSS, Triggers, Behaviors label Apr 15, 2024
@mattleibow
Copy link
Member

Seems that this is broken now that we are no longer making the shape a CHILD, and as a result, it never gets the PARENT that is used here:

public static async Task<Element> FindTemplatedParentAsync(Element element)
{
if (element.RealParent is Application)
return null;
var skipCount = 0;
element = await GetRealParentAsync(element);
while (!Application.IsApplicationOrNull(element))
{
var controlTemplated = element as IControlTemplated;
if (controlTemplated?.ControlTemplate != null)
{
if (skipCount == 0)
return element;
skipCount--;
}
if (element is ContentPresenter)
skipCount++;
element = await GetRealParentAsync(element);
}
return null;
}

Basically, the templating system ONLY looks up the parent-child path, and we have removed the shape from that path.

@mattleibow
Copy link
Member

@PureWeen @StephaneDelcroix was the change in #21151 to no longer parent the shape the correct one? If it was correct, should there be an alternate path for non-parented-but-still-logical-children? Is that just the normal logical children and there is a flaw in the way the logical/virtual child trees are being re-used?

@mattleibow mattleibow added i/regression This issue described a confirmed regression on a currently supported version and removed potential-regression This issue described a possible regression on a currently supported version., verification pending labels Apr 15, 2024
@PureWeen PureWeen modified the milestones: .NET 8 SR4, .NET 8 SR5 Apr 18, 2024
@jsuarezruiz jsuarezruiz self-assigned this Apr 23, 2024
@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Apr 23, 2024

In this change: https://github.com/dotnet/maui/pull/21151/files#r1523373115
We stopped using the AddLogicalChild method which is responsible for setting the Parent, inheriting the BindingContext, subscribing to ResourcesChanged. In that same PR we apply changes to set the BC and subscribing to ResourcesChanged but we never set the Parent. Without setting the parent DynamicResources or TemplateBinding won't work.
Created a PR here #21997 setting the Parent using a WeakRef but is not enough. Using a StrokeShape from resources with several Borders, each time that reference is being used we are overriding the previous parent.
See ##18925 (comment)

Thoughts?, Ideas?

For example, use a weak reference to set the parent, and also implement x:Shared Attribute - XAML | Microsoft Learn

@albyrock87
Copy link
Contributor

Using x:Shared may be a good idea as long as there's an analyzer reporting an error when it's not used on values subclassing BindableObject. Otherwise the only option is a deep clone like it's done here

else if (Value is IList<VisualStateGroup> visualStateGroupCollection)

@JulioVillanueva
Copy link

JulioVillanueva commented Apr 25, 2024

This comment was really good a few months* ago and come with two solutions, we applied one, what do you think of the second one? (Solution B):
#18925 (comment)

@albyrock87
Copy link
Contributor

Follow up of my comment above.
#21747 (comment)

We can introduce a new interface:

public interface IStyleSetterValueProvider {
    object ProvideValue();
}

This interface would be explicitly implemented in VisualStateGroups class and *Shape classes.
The method will basically return a deep clone.

When Setter encounters this kind of values, it will use the returned value of that method.

Well, we can find better names, I just wanted to give an idea.

@WMLPB
Copy link
Author

WMLPB commented Jun 26, 2024

Thank you very much for your help. The Template Binding with CornerRadius works again with MAUI version 8.0.60. But I’m sorry, I have a new CornerRadius issue with GradientDrawable on Android. Please see:

#23267

@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-button Button, ImageButton area-xaml XAML, CSS, Triggers, Behaviors fixed-in-8.0.60 fixed-in-9.0.0-preview.5.24307.10 i/regression This issue described a confirmed regression on a currently supported version platform/android 🤖 s/needs-attention Issue has more information and needs another look t/bug Something isn't working
Projects
Status: Done