Touch Behavior Should Automatically Set BindingContext

[Axemasta]

Now we have delivered the first cut of the TouchBehavior, I believe we need to seriously discuss adding a way to automatically set the BindingContext of the behavior, based on the BindingContext of the bindables the behavior is attached to.

When we developed the idea we chose not to set the BindingContext based on the maui documentation which is quoted:

.NET MAUI does not set the BindingContext of a behavior, because behaviors can be shared and applied to multiple controls through styles.

When using the TouchBehavior we are asking out consumers to manually set the BindingContext of the behavior. The reason maui call this out is because behaviors are likely to be shared in styles so obviously this introduces risks to having a shared BindingContext. The context is different for the TouchBehavior where a new instance is applied for every view using it. We could make the following additions to the feature:

1. Add a new property AutoSetBindingContext which is a bool. If true on attaching the platform implementation will set the BindingContext of the TouchBehavior instance to the VisualElement passed into the effect as per the PlatformBehavior<VisualElement> contract.
2. Add an analyzer / document the utter 💩 out of using TouchBehavior in shared styles. I believe the auto bindable approach would avert the issue of shared binding contexts since they will auto inherit the context of the view

I can see issues like #1783 being a common complaint with this feature in its current form and it will make using the TouchBehavior more like using the rest of Maui, I've personally never had to worry about BindingContexts to this degree before!

Code changes to the behavior are trivial, adding a new bindable property and the following lines to each platform implementation:

protected override void OnAttachedTo(VisualElement bindable, UIView platformView)
{
	if (AutoSetBindingContext)
	{
		BindingContext = bindable.BindingContext;
	}

	//...
}

I have tested this locally by removing the manual binding context setting & setting AutoSet to true and the sample works as expected!

[pictos]

For inspiration we can use the code on our BaseBehavior.

I like the idea of having a bool flag to control it, that way we will not break anyone. I would that flag should NOT be bindable, this will be less flexible for users but will make our life easier. Also I can't see a place where they will change that value too often during runtime, if the user needs that scenario he can implement that prop. as bindable

[TheCodeTraveler]

Edit I'm wrong. We already automatically set the BindingContext for our behaviors.

I strongly recommend against auto-setting the Behavior.BindingContext because it breaks the .NET MAUI Engineering team's intended functionality for Behaviors.

The .NET MAUI team purposefully chose for Behaviors to not inherit their attached view's BindingContext because the same behavior can be applied to multiple VisualElements.

Yes, I agree, it's annoying for folks to open "Bugs" on our repo about it. Yes, I agree, it's different functionality than Xamarin.Forms. Yes, I agree, this would likely make life easier for many developers. I understand that auto-setting Behavior.BindingContext would be well-intended, but we should not purposefully break .NET MAUI's paradigm.

Behaviors are new to many developers who previously used Effects in Xamarin.Forms. It will take time for .NET MAUI developers to learn about the nuance of Behavior.BindingContext. Ultimately it's just a new paradigm that developers will become familiar with over time.

To demonstrate how auto-setting a Behavior's BindingContext could cause unintended consequences, here's an example:

class MainPage : ContentPage
{
  public MainPage()
  {
      var touchBehavior = new TouchBehavior();

      var label = new Label();
      label.Behaviors.Add(touchBehavior); // <-- If we auto-set `TouchBehavior.BindingContext`, TouchBehavior.BindingContext  == Label.BindingContext

      var entry = new Entry();
      entry.Behaviors.Add(touchBehavior); // <-- If we auto-set `TouchBehavior.BindingContext`, now TouchBehavior.BindingContext  == Entry.BindingContext. Is the developer expecting this? Is this what they intended? Likely not.

      Content = new VerticalStackLayout
      {
            Children = 
            {
                  label,
                  entry
            }
      };
  }
}

[Axemasta]

The intended implementation for this feature would be to have the auto bind feature be an opt in, so by default it wouldn't be set automatically.

The way I see it is that we are already "breaking" the maui paradigm for behaviors by setting the binding context, the reason maui doesn't set it is because of shared styles that can be referenced in multiple places. The touch behavior by nature is applied to specific views and can / should not be shared by multiple views.

Since the behavior is 1:1 (behavior : view) I don't think there is any issue with automatically setting the binding context. I think it needs to be heavily documented that the behavior should absolutley not be placed in a shareable llocation (ie a ResourceDictionary), we could write an analyzer for this use case to warn against it.

I can see based on the sample code provided how on paper this could be an issue however this setup wouldn't actually work and would cause a ton of issues for the developer since the same touch behavior instance is being used on multiple visual elements. It will attach to all views but only correctly apply animations to the last view it was added to. We probably need to throw an exception if a developer does this TouchBehaviorAlreadyAppliedToOtherViewException (or something similar) to guard from developers sharing TB instances between multiple views because regardless of BindingContext, they're actually going to have alot of other issues by sharing TB's!

The value add here in my opinion can't be understanded, especially since relative bindings don't seem to work with the TB at all (I saw this alot during development). I think if we restricted the TB to attach to a single view, the auto binding context should be opted in immediately because there is no risk of wierd shared contexts.

Attached is an example of what happens when attaching the same instance to multiple controls, we absolutely want to avoid this from being possible!

Screen.Recording.2024-04-02.at.09.38.11.mov

[TheCodeTraveler]

Well dang! If we've already set this precedence that all of our Behaviors already do it, then yea we kinda also gotta do it for TouchBehavior.

@Axemasta Any chance you could push out a PR that changes the base class on TouchBehavior from PlatformBehavior<VisualElement> to BaseBehavior<VisualElement>?

We should get this merged ASAP and released as a HotFix.

I totally forgot about BaseBehavior when reviewing the TouchBehavior PR! 😅 🤦

[Axemasta]

Sure I'll publish something soon 👍

[TheCodeTraveler]

@Axemasta After digging through this a bit more, it looks like we have a few more Behaviors that also don't inherit from BaseBehavior.

Don't worry about submitting a PR - I'll take care of it because I have to update all the other Behaviors too.

[TheCodeTraveler]

Pull Request submitted: #1791

[CliffAgius]

Closed as answered, if closed in error please let us know to re-open.