【iOS】Fixes ellipsis carries background from trimmed text

[zhongwuzw]

Summary:

Fixes #37926.

Changelog:

[IOS] [FIXED] - Fixes ellipsis carries background from trimmed text

Test Plan:

Demo code:

<Text numberOfLines={1} ellipsizeMode={'tail'}>Long text <Text style={{backgroundColor: 'red'}}>Nested text</Text></Text>

Before fix:

After fix:

[javache]

I don't think we want to mutate _textStorage in the draw path. Would it be possible to do so in the layout path? Also, could you verify this behaviour in the new architecture too?

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	21,284,129	+49,399
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	24,481,163	+49,241
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 778fcecf357b92f8dd5bab07914aa8563796d0f7 Branch: main

[zhongwuzw]

I don't think we want to mutate _textStorage in the draw path. Would it be possible to do so in the layout path? Also, could you verify this behaviour in the new architecture too?

@javache Thanks for the review, I have no idea how to do it in the layout path, my thought is we can mutate the textStorage before drawing and restore the textStorage which we mutated after drawing.

And I tested in new arch and it has the same issue.

[javache]

I would look at making this change in RCTTextShadowView and RCTTextLayoutManager (for the new architecture)

[zhongwuzw]

@javache Friendly ping, thank you for any feedback :)

[github-actions[bot]]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[zhongwuzw]

@javache Seems we can't put it into background thread In new arch, because we use separate textstorage for measure and draw path, shared of textstorage was removed in https://github.com/facebook/react-native/pull/43914.

[zhongwuzw]

any one can help to review? cc. @sammy-SC @javache

[javache]

Thanks for investigating this. The changes look reasonable, but can we add gating here so we only execute this if numberOfLines is set?

[zhongwuzw]

@javache Done :)

[facebook-github-bot]

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

[cipolleschi]

Hi @zhongwuzw, I know that you are fixing a specific issue, but there could be other style properties that we want to remove from the ellipsis.

I think that the ellispis should keep the same style of the character that precedes it, not to have weird artifacts on the screen. what do you think?

[zhongwuzw]

@cipolleschi Hi, I just keep the same color related attributes as the character that precedes it. Other attributes like fontSize would impact the layout of the text.

[facebook-github-bot]

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

[javache]

This is failing internal builds with

packages/react-native/Libraries/Text/Text/RCTTextShadowView.mm:262:64: error: declaration shadows a local variable [-Werror,-Wshadow]
                                     NSTextContainer *_Nonnull textContainer,

packages/react-native/Libraries/Text/Text/RCTTextShadowView.mm:263:46: error: declaration shadows a local variable [-Werror,-Wshadow]
                                     NSRange glyphRange,

[zhongwuzw]

@javache done 😀

[facebook-github-bot]

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

[facebook-github-bot]

@javache merged this pull request in facebook/react-native@fcb6cdc7108103527268aef42a77e530f9482668.

[github-actions[bot]]

This pull request was successfully merged by @zhongwuzw in fcb6cdc7108103527268aef42a77e530f9482668.

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

[facebook-github-bot]

This pull request has been reverted by b1c5432b13e832a253d8b9b2a92b355d2fe91f09.