Apply padding to the text attachements

[j-piasecki]

Summary:

Paddings are not applied to inline views in text, this PR fixes that.

Closes https://github.com/facebook/react-native/issues/42099

Changelog:

[GENERAL] [FIXED] - Fixed padding not being applied to inline views in text

Test Plan:

A simple test case

      <Text style={{paddingHorizontal: 40, paddingVertical: 40, backgroundColor: 'green', width: 300 }}>
        <View style={{backgroundColor: 'red', width: 50, height: 50}} />
        foobar foobar
        <View style={{backgroundColor: 'red', width: 50, height: 50}} />
      </Text>

iOS before	iOS after	Android before	Android after

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	19,460,041	+14,284
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	22,833,515	+15,224
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: c96c8933745b72148856e62f13553d7a2f5d4f6b Branch: main

[NickGerleman]

First things that come to mind:

1. Android and iOS will separately set frame in their own versions of TextLayoutManager. So, before making changes at this specific layer, it would be good to check if iOS does the same thing. Though I can see we are only passing the content frame (frame minus border and padding) as layoutConstraints when measuring string, so I would guess everything is relative to this frame.
2. If we are not offsetting by padding, we are also likely missing space taken by paragraph border as well (also part of the contentFrame insets)
3. We should be reading Fabric LayoutMetrics (see above in same function) instead of from Yoga Node directly. It has contentInsets set which is I think what we're looking for

[j-piasecki]

Android and iOS will separately set frame in their own versions of TextLayoutManager. So, before making changes at this specific layer, it would be good to check if iOS does the same thing. Though I can see we are only passing the content frame (frame minus border and padding) as layoutConstraints when measuring string, so I would guess everything is relative to this frame.

It seems like it, though the exact mechanism differs between Android and iOS. Both pass layoutConstraints to the relevant measure methods, which are then passed on to the platform-specific code.

On iOS, the text storage with the maximum width is created and used to measure text: https://github.com/facebook/react-native/blob/be06fd4e22a500128d202436600381b8bc17b3f5/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm#L48-L54

On Android, the min/max size is passed to the java measure method: https://github.com/facebook/react-native/blob/be06fd4e22a500128d202436600381b8bc17b3f5/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp#L138-L149 and then the max width is used to calculate the text layout: https://github.com/facebook/react-native/blob/be06fd4e22a500128d202436600381b8bc17b3f5/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java#L511-L519

When it comes to drawing, iOS uses the content frame directly: https://github.com/facebook/react-native/blob/be06fd4e22a500128d202436600381b8bc17b3f5/packages/react-native/React/Fabric/Mounting/ComponentViews/Text/RCTParagraphComponentView.mm#L134

On Android the insets are passed on to the native view as padding: https://github.com/facebook/react-native/blob/be06fd4e22a500128d202436600381b8bc17b3f5/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp#L623

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

But in both cases, the measurement logic itself is unaware of the padding.

[j-piasecki]

If we are not offsetting by padding, we are also likely missing space taken by paragraph border as well (also part of the contentFrame insets) We should be reading Fabric LayoutMetrics (see above in same function) instead of from Yoga Node directly. It has contentInsets set which is I think what we're looking for

Yes, and yes. Before I saw that the padding itself is not passed to the measurement I tried to access the padding in the measureContent where layoutMetrics are not yet initialized. Then I copied it without updating 🤦.

Using contentInsets also addresses the second point since it includes borders.

[j-piasecki]

@NickGerleman Friendly ping in case you missed the response.

[NickGerleman]

@j-piasecki yeah I think the only change needed here is just moving to content insets

[facebook-github-bot]

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@NickGerleman merged this pull request in facebook/react-native@12aef32b82325ab143b6ed421f3cbf0f8be66f60.

[github-actions[bot]]

This pull request was successfully merged by @j-piasecki in 12aef32b82325ab143b6ed421f3cbf0f8be66f60.

^{When will my fix make it into a release? | How to file a pick request?}