[Feature] Make T for BehaviorBase<T> constraint to DependencyObject or FrameworkElement

[Rosuavio]

Describe the problem this feature would solve

BehaviorBase<T> supports a lot of our behaviors but as far as its own API for consumption it feels like I can be refined.
Currently T is constrained to UIElement which is more specific than Behavior<T>'s DependencyObject. So BehaviorBase<T> cannot be associated with any DependencyObject like Behavior making it usable in less scenarios.

On the other hand its behavior from its docs

2. Whenever initially fails, this method will subscribe to to allow lazy initialization.

Only works with FrameworkElements but T is constrained to UIElement.

While the behavior

1. Prevent duplicate initialization that can happen (prevent multiple OnAttached calls);

will work regardless of the type it feels like constraining T to UIElement is an awkward middle ground.

Describe the solution

Embrace the usefulness outside of the T is FrameworkElement case and make the class usable in more scenarios.

public abstract class BehaviorBase<T> : Behavior<T>
-        where T : UIElement
+        where T : DependencyObject

or

Embrace the behavior of the T is FrameworkElement case and make the class constrained to scenarios where both 1 and 2 work.

public abstract class BehaviorBase<T> : Behavior<T>
-        where T : UIElement
+        where T : FrameworkElement

Describe alternatives you've considered

Do nothing.

Additional context & Screenshots

Hello, 'RosarioPulella! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

[michael-hawker]

Good points @RosarioPulella, let's worry about this discussion for 7.1 and focus on the 7.0.1 bugs first.

I think the underlying Behaviors SDK has made improvements in this area as well compared to when we first started. So I think the proper solution would be to try removing our BehaviorBase class all together and if there are remaining issues, we should investigate patching the Behavior SDK itself.

See that discussion here and the original PR over there which I think addressed that concern here.

[Rosuavio]

@Sergio0694 what do you think, does one solution make more sense, or should we leave this as it is.

[Sergio0694]

Absolutely agree that the current UIElement constraint is a bit pointless, since it doesn't really add anything of value. Personally I wouldn't be able to recall off the top of my head a case where I've ever wanted to use a behavior on something that was not also a FrameworkElement, so the second choice seems fine to me, if the major point of the implementation is specifically to support those events for FrameworkElement-s (especially because otherwise those OnAssociatedObjectLoaded, Unloaded and SizeChanged events wouldn't really make sense in the first place) 😄

A couple notes/questions while goign through the BehaviorBase<T> implementation (reference source below):

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/9b23e4f8711a463e02d1b8fad6a9537c1de21200/Microsoft.Toolkit.Uwp.UI.Behaviors/BehaviorBase.cs#L43-L55

1. The XML docs say that one point of this class is that it avoids repeated initialization, as there's the HandleAttach method that calls Initialize, which is then overridden by the user, and that's only called once. But, the event handlers are still added every time and they're not cleared (no -= beforehand), so aren't we technically still potentially causing multiple handlers being added? In particular, shouldn't we be only adding these handlers the first time the attaching was successful?
2. If the user is supposed to insert their logic into Initialize and Uninitialize and not here, then OnAttached and OnDetaching should be sealed, right?
3. Shouldn't Initialize and Uninitialize be called On___ for naming consistency with protected virtual methods?
4. We're always subscribing to SizeChanged, which is potentially expensive during resize operations, even if the inherited classes are not using it. Should we have some customization logic to let consumers specify which methods they want to support (as an optimization), or would that be overkill?
5. If we want to restrict T to FrameworkElement, what about FrameworkElementBehavior<T> rather than BehaviorBase<T>? Would make the purpose of the type clearer for the name (ie. "some behavior specifically supporting FrameworkElement-s) rather than a generic "Base" version, which personally makes me think "but... isn't Behavior<T> a base class already?".

Just some general thoughts, hope this helps! 🙂

[Rosuavio]

@Sergio0694 I agree, changing it to FrameworkElement would be clearer. If you want I can update my PR taking into account your notes or if you want you can open one. I know your working on the source generators so let me know.

[Sergio0694]

I can make a PR if you'd like me to take care of this, sure!
I do need something else to do when taking a break from the other PR after all ahah 😄