Refactor `TextFragmentList` into `AttributedString` #42701

cubuspl42 · 2024-01-29T11:09:57Z

Summary:

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

This is a part of my work on #42602.

Changelog:

[INTERNAL] [CHANGED] - Refactor TextFragmentList into AttributedString

Test Plan:

...preparing it to support another layer over fragments.

analysis-bot · 2024-01-29T11:42:37Z

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: cfc0ba0
Branch: main

NickGerleman · 2024-01-29T21:23:36Z

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.

NickGerleman · 2024-01-29T21:25:02Z

...eactAndroid/src/main/java/com/facebook/react/views/text/attributedstring/AttributedString.kt

+
+package com.facebook.react.views.text.attributedstring
+
+/** Interface for an attributed string */


This comment, and same for AttributedStringFragment are not meaningful to someone who wants to learn what this interface represents and is used for.

cubuspl42 · 2024-01-29T22:27:47Z

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?

Rename facebook::react::AttributedString tofacebook::react::DenormalizedText
- In consequence, we'll also havefacebook::react::DenormalizedText::Fragment
Split facebook::react::TextAttributes to facebook::react::TextAttributes and facebook::react::DenormalizedText::FragmentAttributes
Refactor com.facebook.react.views.text.TextAttributeProps to com.facebook.react.views.text.denormalized.DenormalizedTextFragmentAttributes, nuking the unused attributes (closing T63643819)
Refactor com.facebook.react.views.text.fragments.TextFragmentList to com.facebook.react.views.text.denormalized.DenormalizedText
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 #42703, modulo the naming.

(4) and (5) is this PR.

cubuspl42 · 2024-01-29T22:34:07Z

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 · 2024-01-30T00:20:26Z

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.

Split facebook::react::TextAttributes to facebook::react::TextAttributes and facebook::react::DenormalizedText::FragmentAttributes

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 · 2024-01-30T08:37:01Z

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 · 2024-01-30T08:50:13Z

Word idea: flat / flattened

cubuspl42 · 2024-01-30T09:02:27Z

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.

react-native/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp

Line 289 in ab72950

auto serializedAttributedString = toDynamic(attributedString);

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

Line 259 in d4c3311

    
           buildSpannableFromFragments(context, attributedString.getArray("fragments"), sb, ops);

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 · 2024-01-30T20:36:05Z

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 · 2024-01-31T09:30:47Z

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

The AttributedString is deserialized here:

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

Line 247 in d4c3311

private static Spannable createSpannableFromAttributedString(

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

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

Lines 28 to 29 in d4c3311

    
           import com.facebook.react.bridge.ReadableArray; 
        
           import com.facebook.react.bridge.ReadableMap;

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

react-native/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp

Line 289 in ab72950

auto serializedAttributedString = toDynamic(attributedString);

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 · 2024-01-31T09:38:35Z

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 · 2024-02-05T09:56:51Z

@NickGerleman Bump

cubuspl42 · 2024-02-08T17:04:53Z

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

NickGerleman · 2024-02-08T22:00:42Z

@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 · 2024-02-09T09:36:06Z

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: #42701 (comment)

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 · 2024-02-09T09:38:36Z

@mdvacca

Could you share you insight regarding to what Nick asked here... #42701 (comment)

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 · 2024-02-11T01:09:57Z

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 · 2024-02-18T01:05:19Z

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

react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/StateWrapper.java

Lines 15 to 20 in 6780e00

    
           /** 
        
            * This is a wrapper that can be used for passing State objects from Fabric C++ core to 
        
            * platform-specific components in Java. State allows you to break out of uni-directional dataflow 
        
            * by calling updateState, which communicates state back to the C++ layer. 
        
            */ 
        
           public interface StateWrapper {

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

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

react-native/packages/react-native/Libraries/Components/TextInput/TextInput.js

Line 1190 in 8ff05b5

viewCommands.setTextAndSelection(

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

Renderer dispatchCommand

react-native/packages/react-native/Libraries/Renderer/implementations/ReactFabric-dev.js

Line 27633 in 8ff05b5

function dispatchCommand(handle, command, args) {

UIManager.cpp dispatchCommand

react-native/packages/react-native/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp

Line 597 in 8ff05b5

if (methodName == "dispatchCommand") {

and it is received from the Java ViewManager updateState or updateExtraData

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java

Lines 363 to 366 in 8ff05b5

    
             /** 
        
              * Subclasses can implement this method to receive state updates shared between all instances of 
        
              * this component type. 
        
              */

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java

Lines 252 to 263 in 8ff05b5

    
             /** 
        
              * Subclasses can implement this method to receive an optional extra data enqueued from the 
        
              * corresponding instance of {@link ReactShadowNode} in {@link 
        
              * ReactShadowNode#onCollectExtraUpdates}. 
        
              * 
        
              * <p>Since css layout step and ui updates can be executed in separate thread apart of setting 
        
              * x/y/width/height this is the recommended and thread-safe way of passing extra data from css 
        
              * node to the native view counterpart. 
        
              * 
        
              * <p>TODO T7247021: Replace updateExtraData with generic update props mechanism after D2086999 
        
              */ 
        
             public abstract void updateExtraData(@NonNull T root, Object extraData);

The implementation in ReactTextInputManager.java of updateState

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java

Line 1357 in 8ff05b5

public Object updateState(

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:

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

react-native/packages/react-native/Libraries/Components/TextInput/TextInput.js

Lines 1164 to 1166 in 8ff05b5

    
           // This is necessary in case native updates the text and JS decides 
        
           // that the update should be ignored and we should stick with the value 
        
           // that we have in JS.

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 · 2024-02-18T02:22:39Z

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

react-native/packages/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-dev.js

Line 5377 in 4238e31

ReactNativePrivateInterface.UIManager.createView(

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:

react-native/packages/react-native/Libraries/Renderer/implementations/ReactFabric-dev.js

Line 5391 in 4238e31

"RCTRawText", // viewName

react-native/packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp

Line 65 in 17d2c8a

std::shared_ptr<ShadowNode> UIManager::createNode(

Explanation of the iOS API here Expensify/App#17767 (comment)

cubuspl42 · 2024-02-19T08:57:46Z

@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 · 2024-02-19T08:59:28Z

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

fabOnReact · 2024-02-21T00:04:06Z

#42701 (comment) #42701 (comment)
I'm just sharing my knowledge with you, hopefully we both learn something from each other. Thanks.

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jan 29, 2024

cubuspl42 force-pushed the android-AttributedString branch from bb434e9 to 37692d4 Compare January 29, 2024 11:14

Refactor TextFragmentList into AttributedString

a57ee9a

...preparing it to support another layer over fragments.

cubuspl42 force-pushed the android-AttributedString branch from 37692d4 to a57ee9a Compare January 29, 2024 11:15

cubuspl42 mentioned this pull request Jan 29, 2024

Implement the support for the inline code blocks #42602

Open

NickGerleman reviewed Jan 29, 2024

View reviewed changes

cubuspl42 marked this pull request as draft February 22, 2024 15:32

cubuspl42 mentioned this pull request Feb 22, 2024

Initial implementations of spans #43150

Open

fabOnReact mentioned this pull request Feb 28, 2024

✨ Migrate to new-arch (react-native 0.74, Fabric/TurboModules/CodeGen/bridgeless) mrousavy/react-native-vision-camera#2614

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `TextFragmentList` into `AttributedString` #42701

Refactor `TextFragmentList` into `AttributedString` #42701

cubuspl42 commented Jan 29, 2024

analysis-bot commented Jan 29, 2024

NickGerleman commented Jan 29, 2024

NickGerleman Jan 29, 2024

cubuspl42 commented Jan 29, 2024 •

edited

cubuspl42 commented Jan 29, 2024

NickGerleman commented Jan 30, 2024

cubuspl42 commented Jan 30, 2024 •

edited

cubuspl42 commented Jan 30, 2024

cubuspl42 commented Jan 30, 2024

NickGerleman commented Jan 30, 2024

cubuspl42 commented Jan 31, 2024

cubuspl42 commented Jan 31, 2024

cubuspl42 commented Feb 5, 2024

cubuspl42 commented Feb 8, 2024

NickGerleman commented Feb 8, 2024

cubuspl42 commented Feb 9, 2024

cubuspl42 commented Feb 9, 2024

NickGerleman commented Feb 11, 2024 •

edited

fabOnReact commented Feb 18, 2024 •

edited

fabOnReact commented Feb 18, 2024 •

edited

cubuspl42 commented Feb 19, 2024

cubuspl42 commented Feb 19, 2024

fabOnReact commented Feb 21, 2024 •

edited


		package com.facebook.react.views.text.attributedstring

		/** Interface for an attributed string */

Refactor TextFragmentList into AttributedString #42701

Are you sure you want to change the base?

Refactor TextFragmentList into AttributedString #42701

Conversation

cubuspl42 commented Jan 29, 2024

Summary:

Changelog:

Test Plan:

analysis-bot commented Jan 29, 2024

NickGerleman commented Jan 29, 2024

NickGerleman Jan 29, 2024

Choose a reason for hiding this comment

cubuspl42 commented Jan 29, 2024 • edited

cubuspl42 commented Jan 29, 2024

NickGerleman commented Jan 30, 2024

cubuspl42 commented Jan 30, 2024 • edited

cubuspl42 commented Jan 30, 2024

cubuspl42 commented Jan 30, 2024

NickGerleman commented Jan 30, 2024

cubuspl42 commented Jan 31, 2024

cubuspl42 commented Jan 31, 2024

cubuspl42 commented Feb 5, 2024

cubuspl42 commented Feb 8, 2024

NickGerleman commented Feb 8, 2024

cubuspl42 commented Feb 9, 2024

cubuspl42 commented Feb 9, 2024

NickGerleman commented Feb 11, 2024 • edited

fabOnReact commented Feb 18, 2024 • edited

fabOnReact commented Feb 18, 2024 • edited

cubuspl42 commented Feb 19, 2024

cubuspl42 commented Feb 19, 2024

fabOnReact commented Feb 21, 2024 • edited

Refactor `TextFragmentList` into `AttributedString` #42701

Refactor `TextFragmentList` into `AttributedString` #42701

cubuspl42 commented Jan 29, 2024 •

edited

cubuspl42 commented Jan 30, 2024 •

edited

NickGerleman commented Feb 11, 2024 •

edited

fabOnReact commented Feb 18, 2024 •

edited

fabOnReact commented Feb 18, 2024 •

edited

fabOnReact commented Feb 21, 2024 •

edited