Move react tag to an attached property

[jonthysell]

Description

This PR defines a new "ReactTag" attached property to store react tag values on XAML Dependency Objects, rather than using FrameworkElement's general-purpose Tag property.

Some downstream users (i.e. custom controls) expect to be able to use FE's Tag property for their own purposes, so overriding it ourselves can limit, if not break, integration with RN and XAML.

This PR updates the internal SetTag and GetTag helper methods to make this transition as seamless as possible. SetTag sets both the new property and the old FrameworkElement Tag, to improve compatiiblity with older ViewManagers who expect that behavior. GetTag always uses the new property, so we aren't broken if a ViewManager overwrites FE's Tag.

This PR also exposes the new property under XamlHelpers::ReactTagProperty() for external modules who want to get the definitive tag value of a XAML element.

Closes # 4187

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Microsoft Reviewers: Open in CodeFlow

[acoates-ms]

Do we have a sense of the performance of this vs using a specific interface on the existing Tag property? My understanding is that custom properties have some bad performance characteristics compared to the built in properties. And this is a property we are setting on almost every element in the RN tree.

[jonthysell]

Do we have a sense of the performance of this vs using a specific interface on the existing Tag property? My understanding is that custom properties have some bad performance characteristics compared to the built in properties. And this is a property we are setting on almost every element in the RN tree.

The most performant properties are backed by regular fields in objects - whereas some properties are stored in the XAML property system (with boxing and all that). As far as I can tell, the FrameworkElement::Tag property is already using the property system. I don't know if there's a perf difference between a property we defined internally and an external attached property.

[jonthysell]

Converting to draft as I can't prove that everything still works.

[jonthysell]

With the sample apps being fixed, I've finally confirmed that this replacement of properties actually works.

[rozele]

If this change lands, what's the rollout plan? In Messenger for example, we rely on the implementation detail that the React tag is set to the FrameworkElement::Tag property in various view managers and native modules. Obviously it's not great to do this, but if we're doing it I'm assuming others may be as well.

It's easy enough to fix, but perhaps the first step should be to release an API to get the tag from a view in something like XamlUIService, make a note somewhere that this is the only supported / reliable way to get the tag, then finally roll out this breaking change (hoping that most apps have started using the "supported" way of getting a React tag).

[asklar]

If this change lands, what's the rollout plan? In Messenger for example, we rely on the implementation detail that the React tag is set to the FrameworkElement::Tag property in various view managers and native modules. Obviously it's not great to do this, but if we're doing it I'm assuming others may be as well.

It's easy enough to fix, but perhaps the first step should be to release an API to get the tag from a view in something like XamlUIService, make a note somewhere that this is the only supported / reliable way to get the tag, then finally roll out this breaking change (hoping that most apps have started using the "supported" way of getting a React tag).

I had fixed up our usages of the tag internally in #9593 - you may be able to use the helper from that header, if not, it seems reasonable to add a winrt api for it for modules to call.

[rozele]

Labeled the PR as a breaking change at least, since strictly speaking it changes how apps should interface with RN.

[jonthysell]

This PR already uses the GetTag and SetTag helpers from #9593:

• SetTag still sets both the FrameworkElement::Tag property in addition to the new ReactTag property
• GetTag returns the value of ReactTag property

Internally, all code is updated to use ReactTag.

The new API ReactTagFromElement exposes GetTag for consumers needing a single method to convert an element into a tag.

Consumers needing to get the tag, and currently reading FrameworkElement::Tag directly to get it:

• Can do nothing, and it's still there and will still work as before
• If they're very XAML savvy, and want to call GetValue, can use the exposed ReactTagProperty.
• If they just want a single function, they could switch to calling ReactTagFromElement

Consumers needing to set the tag (aka RNX), and currently setting FrameworkElement::Tag:

• Switch to calling SetValue with the new ReactTagProperty

Finally, consumers/controls who want to get/set FrameworkElement::Tag for their own purposes can do so without interfering with RNW.

@asklar RNX will need to update its getting and setting (with either a compile time version macro or a runtime API check) but I don't think it's that big of an issue.

@rozele Is Msssenger setting the tag or just reading it? If you're just reading it you might not have to do anything.

[asklar]

@asklar RNX will need to update its getting and setting (with either a compile time version macro or a runtime API check) but I don't think it's that big of an issue.

@jonthysell could we add a macro in version.h or somewhere else, that says "has React tag API", so we don't have to check against a version number?

[jonthysell]

@asklar RNX will need to update its getting and setting (with either a compile time version macro or a runtime API check) but I don't think it's that big of an issue.

@jonthysell could we add a macro in version.h or somewhere else, that says "has React tag API", so we don't have to check against a version number?

@asklar Do we want to give people the option to override and go back to the previous behavior? I could use the macro myself within GetTag - though that wouldn't work if you consumed the precompiled NuGet.

[asklar]

@asklar

        Alexander Sklar
        FTE RNX will need to update its getting and setting (with either a compile time version macro or a runtime API check) but I don't think it's that big of an issue.

@jonthysell

        Jon Thysell (JAUNTY)
        FTE could we add a macro in version.h or somewhere else, that says "has React tag API", so we don't have to check against a version number?

@asklar Alexander Sklar FTE Do we want to give people the option to override and go back to the previous behavior? I could use the macro myself within GetTag - though that wouldn't work if you consumed the precompiled NuGet.

no I think the new behavior is righteous (in fact the old behavior caused an issue with adaptive cards also setting the tag and thinking it was the only one doing so)

[rozele]

@rozele Is Msssenger setting the tag or just reading it? If you're just reading it you might not have to do anything.

We just read the tag using FrameworkElement::TagProperty. We haven't yet migrated to the XamlUIService::GetReactTag property I added in https://github.com/microsoft/react-native-windows/pull/10403

So, we'll either need to switch to the ReactTagProperty or switch to the XamlUIService API once one of our PRs merge