Fix Text borders not rendering.

[rectified95]

Forking the JS component until a native solution is workable.
Closes #5187

Microsoft Reviewers: Open in CodeFlow

EDIT:
While I was working on this PR, we ingested a portion of changes from RN, which included a new behaviour in View.js whereby TextAncestor gets reset in order to enable certain scenarios of nesting View inside Text (eg. inline vs block images). It's impossible on Windows anyway, so overriding in Text.windows.js that should be fine.

[acoates-ms]

Does this PR break refs? I think you would need to forward the ref to the inner Text element.

Also, doesn't this PR break text flow layout if you try to put a border on a nested Text element?

[NickGerleman]

I'm also a little concerned about whether this will introduce JS behavior changes. We've been pretty bad about JS correctness issues when we've forked components, so it could be useful to have a short test plan here to understand if we're breaking scenarios.

Some offline discussion earlier with Igor was that this is hard to paper over in native (@kmelmon having more context). Architecturally, we'd love to have native abstract over differences to the JS layer, which this change kind of goes against, but I don't have enough familiarity to understand why this is hard for native. I think it would be useful to understand architectural gaps here.

[kmelmon]

Igor and I discussed the options for implementing this in native, they are all very complex/expensive to implement. Briefly those options were:

1. Refactor the code we currently have for View
2. Drop down to Composition
   Both of these options are multiple dev weeks of work. Option Create ReactContentBaseMSModule Extensions #1 leads to us having multiple XAML elements in the tree, so isn't much better than wrapping with an extra View. Option Roslyn analyzer for NativeModuleBase #2 would potentially give us a perf advantage but is the most risky/expensive to implement. This is what led to the "wrap-with-a-view" option. It's clearly the least expensive option, so unless it introduces more functional problems that can't be overcome, I would greatly prefer this solution so that we can optimize our limited dev resources.

---

In reply to: 674347472 [](ancestors = 674347472)

[acoates-ms]

Do you have fixes for the issues I posted above? Wrapping an inner text with a view will break all kinds of things.

[kmelmon]

[acoates-ms]

Can you verify this simple sample?

<Text>This text is <Text style={{color:'red', borderWidth:1, borderColor:'black'}}>outlined</Text> and laid out within the normal text run, so will wrap etc as normal text.</Text>

I'm worried that this PR as is will break text. -- Its probably better to not have the border than break the apps layout.

[rectified95]

@acoates-ms I'm verifying the nested text behavior - was going get back to you once I was done. I'll also try your sample :)
So far, I've checked that Android doesn't render border for the nested text tag.

I had also been thinking about refs - can you elaborate what exactly would break with this change and how to mitigate it?

[rectified95]

@acoates-ms You have a point - the nested Text doesn't render:

However, since neither Android, nor iOS renders borders for nested Text elements, perhaps we could document that this doesn't work? Granted, it'd better if we matched their behavior and still rendered the inner text without borders, but it's a matter of how we handle an apparently unsupported combination of props on an element.
The tradeoff here is having no borders at all, vs failing more explicitly for a case that doesn't work on other platforms anyway.
What do you think?

[acoates-ms]

The text component already has code to detect if its nested text or not. See the references to TextAncenstor within the text component.

[rectified95]

@acoates-ms I've solved the edge case you pointed out, and added refs. See the updated PR description for a screenshot of your sample. :) Thanks for the tip about detecting nesting!

[rectified95]

@acoates-ms Applied all your suggestions re: avoiding unnecessary object spreads, passing along more margin props.
Also, fixed the case where nested text inside a bordered outer one stopped working after we ingested a change in View.js

[rectified95]

@acoates-ms All CI checks passed, comments resolved.