Refactor `TextFragmentList` into `AttributedString`

[cubuspl42]

Summary:

Refactor TextFragmentList into AttributedString, preparing it to support another layer over fragments.

This is a part of my work on https://github.com/facebook/react-native/issues/42602.

Changelog:

[INTERNAL] [CHANGED] - Refactor TextFragmentList into AttributedString

Test Plan:

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	17,066,833	+1
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	20,450,440	+8
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: cfc0ba04a1fa147517e94d1f952e2a8fe62e6e9c Branch: main

[NickGerleman]

Context, that you may have already found, is that AttributedString is a specific structure, that is how Fabric represents text fragments (on both Android and iOS). https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.h

I think some of the original naming around here, around bridge vs attributedstring, is because the Paper path is not backed by an AttributedString from Fabric, but a similar Java side structure.

For that reason, naming like BridgeAttributedString might make some confusion.

[cubuspl42]

Context, that you may have already found, is that AttributedString is a specific structure, that is how Fabric represents text fragments (on both Android and iOS). https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.h

I think some of the original naming around here, around bridge vs attributedstring, is because the Paper path is not backed by an AttributedString from Fabric, but a similar Java side structure.

For that reason, naming like BridgeAttributedString might make some confusion.

It's a good point.

I'm thinking about a fully-processed <Text> derivative. Denormalized and effective.

AttributedString is not a perfect name for multiple reasons. To begin with, it's also the name of the iOS class we use...

Would you agree that this set of actions would result in consistent naming and semantics?

1. Rename facebook::react::AttributedString tofacebook::react::DenormalizedText
   • In consequence, we'll also havefacebook::react::DenormalizedText::Fragment
2. Split facebook::react::TextAttributes to facebook::react::TextAttributes and facebook::react::DenormalizedText::FragmentAttributes
3. Refactor com.facebook.react.views.text.TextAttributeProps to com.facebook.react.views.text.denormalized.DenormalizedTextFragmentAttributes, nuking the unused attributes (closing T63643819)
4. Refactor com.facebook.react.views.text.fragments.TextFragmentList to com.facebook.react.views.text.denormalized.DenormalizedText
5. Refactor com.facebook.react.views.text.fragments.TextFragment to com.facebook.react.views.text.denormalized.DenormalizedTextFragment

We would have two implementation paths for the same concept of denormalized text.

In other words, I'm suggesting that what we do in Paper is the same thing we do in Fabric, just using a different programming language and different nomenclature.

I will have to do (2) anyway, modulo the naming.

(3) will be a follow-up to https://github.com/facebook/react-native/pull/42703, modulo the naming.

(4) and (5) is this PR.

[cubuspl42]

Also, it's not a secret that my main point is extending this DenormalizedText concept with a new layer, which I currently codename shards (to avoid naming conflict with Android Span).

Technically, this layer could also be implemented both on Paper and Fabric. I have only a Fabric implementation, though. Whether we have to support Paper first-class in the context of my project is another topic.

[NickGerleman]

AttributedString is not a perfect name for multiple reasons. To begin with, it's also the name of the iOS class we use...

I would agree with that. Fabric layer has some other examples of that as well though (e.g. text attachments). "normalized" is overloaded enough that I think DenormalizedText is less clear in intent than AttributedString (a string, with sections of attributes).

I would recommend keeping it's naming as is, as it's also relatively well-known, and often used at this point.

2. Split facebook::react::TextAttributes to facebook::react::TextAttributes and facebook::react::DenormalizedText::FragmentAttributes
3. Refactor com.facebook.react.views.text.TextAttributeProps to com.facebook.react.views.text.denormalized.DenormalizedTextFragmentAttributes, nuking the unused attributes (closing T63643819)

Splitting this up makes a lot of sense to me. It does seem like we are storing unrelated bits in each use case.

[cubuspl42]

I do not feel strongly about any particular name. What's more important is what is named is the same as something else. I'm suggesting that Paper builds the same data structure as Fabric, let it be named AttributedString, DenormalizedText, AttributedText, FooText or whatever.

This foo text structure currently has one layer of foo fragments. I want to expand it so it's a built of foo shards, where each foo shard is built of foo fragments.

As I see it, it would be more consistent if the Java class structure and the C++ class structure were named the same way, as it is the same thing (even if the implementation is not always shared).

Please let me know what would be an acceptable way forward. Maybe we could "borrow" the name AttributedString with its semantics, detaching its meaning from facebook::react::AttributedString? An appropriate comment could help understand this reasoning.

We could still shift some of the naming towards fragments (mainly TextAttributeProps).

Otherwise, we're blocked because the existing naming is unfortunate and/or inconsistent, and we can't change that because the names are relatively well-known.

[cubuspl42]

Word idea: flat / flattened

[cubuspl42]

I think some of the original naming around here, around bridge vs attributedstring, is because the Paper path is not backed by an AttributedString from Fabric, but a similar Java side structure.

For that reason, naming like BridgeAttributedString might make some confusion.

Actually, wait.

https://github.com/facebook/react-native/blob/ab729507153c5a0f4d69a2dfb19b4125f8fbd78e/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp#L289

https://github.com/facebook/react-native/blob/d4c33112961673e09f5bfccaeeff284fb321fe63/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java#L259

I must admit that I'm sometimes lost in the forest of Paper, Fabric, bridges, MapBuffers, legacies, and duplications... But isn't this facebook::react::AttributedString being serialized over the bridge?

[NickGerleman]

I must admit that I'm sometimes lost in the forest of Paper, Fabric, bridges, MapBuffers, legacies, and duplications... But isn't this facebook::react::AttributedString being serialized over the bridge?

It is serialized, but not over the bridge.

Unlike on iOS, where we have separate Fabric and Paper implementations of components like Text, Android shares the same underlying view managers for both (in Java). So we need to go over a JNI boundary, to call Java from C++.

MapBuffer is a specific structure that was added to make this serialization over the JNI boundary more efficient.

Note that while we use Java View Managers under Fabric/new architecture, the Java ShadowNode's are only used in Paper.

So, Java ShadowNode logic is old architecture, and both TextLayoutManager and TextLayoutManagerMapBuffer are used in Fabric.

IIRC we had rolled out the changes to use MapBuffer for text serialization everywhere, so I don't actually know why/where TextLayoutManager was still being used. @mdvacca do you have context into this?

[cubuspl42]

@NickGerleman Maybe let's focus on deserialization, because that's what the BridgeAttributedString is abstracting.

The AttributedString is deserialized here:

https://github.com/facebook/react-native/blob/d4c33112961673e09f5bfccaeeff284fb321fe63/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java#L247

And it uses com.facebook.react.bridge.* APIs (focus on bridge):

https://github.com/facebook/react-native/blob/d4c33112961673e09f5bfccaeeff284fb321fe63/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java#L28-L29

As I understood, this code is deserializing what was serialized here:

https://github.com/facebook/react-native/blob/ab729507153c5a0f4d69a2dfb19b4125f8fbd78e/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp#L289

Did I get something wrong? Is this code "not using a bridge", although it uses com.facebook.react.bridge.* APIs?

Respectfully, is it possible that you also got lost in this labyrinth? 😃

[cubuspl42]

As I understood this, Fabric and bridge are not mutually exclusive, and Fabric can be configured to use the bridge, although MapBuffer serialization is preferred.

[cubuspl42]

@NickGerleman Bump

[cubuspl42]

@NickGerleman Bump again. Let me know if I can help make anything clearer

[NickGerleman]

@cubuspl42 please see previous feedback. I don't think this change is a positive in terms of naming, wrt how existing structures are set up.

[cubuspl42]

Thank you for all of your feedback, including the explanations of the RN architecture. Unfortunately, I have a strong feeling that there's some misunderstanding going on here, and I cannot resolve it without your help.

I might have contributed to the misunderstanding with my initial response to your feedback: https://github.com/facebook/react-native/pull/42701#issuecomment-1915685740

Are you 100% confident that your feedback is relevant to the discussed code?

As I see it, the new JVM AttributedString interface is...

• Fabric-specific
• a view of facebook::react::AttributedString

Do you see it differently?

[cubuspl42]

@mdvacca

Could you share you insight regarding to what Nick asked here... https://github.com/facebook/react-native/pull/42701#issuecomment-1917847140

IIRC we had rolled out the changes to use MapBuffer for text serialization everywhere, so I don't actually know why/where TextLayoutManager was still being used. @mdvacca do you have context into this?

...and give this PR a second pair of eyes in general? I'd like to move this forward soon, as I think that the changes are less controversial than they appear.

[NickGerleman]

Hey @cubuspl42, there are several requests for changes, and comments on the PR, not addressed or reflected in the revision I am seeing at least. Please do not ping until these are addressed.

There are some nuances to these names. Let's not rename them as part of this work, that you are trying to unlock.

[fabOnReact]

Some updates are from JS --> CPP --> Java using components APIs like AndroidTextInputState.cpp

https://github.com/facebook/react-native/blob/6780e00fcb515af04ff91fba7f28ff7152e3210f/ReactAndroid/src/main/java/com/facebook/react/uimanager/StateWrapper.java#L15-L20

As explained in the below comments, you can add comments and log the values passed to cpp:

• https://github.com/facebook/react-native/pull/33468#discussion_r835036889
• https://github.com/fabOnReact/react-native-notes/issues/12#issuecomment-1078727833
• https://github.com/fabOnReact/react-native-notes/issues/12#issuecomment-1077084459
• https://github.com/fabOnReact/react-native-notes/issues/12#issuecomment-1078550879

For example for a controlled TextInput component that send updates onChangeText from JS to Java

function ControlledTextInput() {
  const [value, setValue] = React.useState("")
  const [errorMessage, setErrorMessage] = React.useState(null)
  const onChangeCallback = (text) => { 
     setText(text);
     if (text.length > 5) {
       setErrorMessage("not valid text")
    } else {
       setErrorMessage(null)
    }
  }
  return (
     <TextInput onChange={onChangeCallback}>
        <Text errorMessageAndroid={errorMessage}>{value}</Text>
     </TextInput>
  )
}

Text, Selection and other updates triggered onChangeText are sent through the codegen action

https://github.com/facebook/react-native/blob/8ff05b5a18db85ab699323d1745a5f17cdc8bf6c/packages/react-native/Libraries/Components/TextInput/TextInput.js#L1190

which uses dispatchCommand to send updates from JS --> CPP --> Java

Renderer dispatchCommand

https://github.com/facebook/react-native/blob/8ff05b5a18db85ab699323d1745a5f17cdc8bf6c/packages/react-native/Libraries/Renderer/implementations/ReactFabric-dev.js#L27633

UIManager.cpp dispatchCommand

https://github.com/facebook/react-native/blob/8ff05b5a18db85ab699323d1745a5f17cdc8bf6c/packages/react-native/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp#L597

and it is received from the Java ViewManager updateState or updateExtraData

https://github.com/facebook/react-native/blob/8ff05b5a18db85ab699323d1745a5f17cdc8bf6c/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java#L363-L366

https://github.com/facebook/react-native/blob/8ff05b5a18db85ab699323d1745a5f17cdc8bf6c/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java#L252-L263

The implementation in ReactTextInputManager.java of updateState

https://github.com/facebook/react-native/blob/8ff05b5a18db85ab699323d1745a5f17cdc8bf6c/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java#L1357

The complete implementation is available at https://github.com/facebook/react-native/pull/33468/files. I did not work for a long time on this and some of the info may be not correct.

NEXT STEPS:

1. Further testing the codegen actions in TextInput.js

• I would try to remove the codegen action from TextInput.js
• My theory is that the text of a controlled textinput does not update anymore onChangeText. The value prop sent from JavaScript TextInput.js to ReactEditText, does not over-ride the internal text state of the Java EditText
• Explained here https://github.com/facebook/react-native/blob/8ff05b5a18db85ab699323d1745a5f17cdc8bf6c/packages/react-native/Libraries/Components/TextInput/TextInput.js#L1164-L1166

2. Further investigating how react-native implements react-reconciler and uses JNI to send data from JS --> CPP --> Java

• the react-native renderer documentation is available at https://github.com/facebook/react/tree/main/packages/react-reconciler
• UIManager.cpp seems to be responsible for sending data from JS to Java using JNI https://github.com/fabOnReact/react-native-notes/blob/b5786376f4efc590386860a574d29db90f3cddaf/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp#L487
• I believe updates are sent using commitUpdate
• Here you can find docs about the new renderer

[fabOnReact]

In the case of Text component, we discussed this in https://discord.com/channels/1147256486762913884/1147308463546957894/1185783170726105229

When you add jsx in React <Text>Hello World</Text> In the old architecture (Paper): The renderer function createTextInstance uses UIManager to add the native view https://github.com/facebook/react-native/blob/4238e311a0743de1209ae26e9ec5c09543af1d8b/packages/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-dev.js#L5377 The Android UIManager API adds the view (ReactTextView) to your app https://github.com/fabriziobertoglio1987/react-native/blob/4238e311a0743de1209ae26e9ec5c09543af1d8b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java#L418

In the case of Fabric renderer: https://github.com/facebook/react-native/blob/4238e311a0743de1209ae26e9ec5c09543af1d8b/packages/react-native/Libraries/Renderer/implementations/ReactFabric-dev.js#L5391 https://github.com/facebook/react-native/blob/17d2c8acb9b193f9af0bcd318abff324e29933b5/packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp#L65

Explanation of the iOS API here https://github.com/Expensify/App/issues/17767#issuecomment-1530891357

[cubuspl42]

@fabOnReact

Helllo Fabrizio,

could I ask you what your role is on the React Native project and whether your comments are related to my PR? Thank you.

[cubuspl42]

Regarding this PR, I'm currently pivoting, and I'll likely close it.

[fabOnReact]

https://github.com/facebook/react-native/pull/42701#issuecomment-1918732459 https://github.com/facebook/react-native/pull/42701#issuecomment-1935605209 I'm just sharing my knowledge with you, hopefully we both learn something from each other. Thanks.