Repository navigation
Conversation
A ParagraphShadowNode cloned with neither new props nor new children (for example by Yoga because a sibling changed) dropped its memoized content and measured layouts. layout() then rebuilt the attributed string, which no longer compared equal to the one in state because the fragments' parentShadowView layout metrics had changed, so every such sibling got a state update and was re-set on the UI thread. With prepared text layout the sibling was also laid out again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Attachments hold raw pointers to the children the content was built from, and layout() replaces those children with laid-out clones. A clone that kept that content pointed at nodes only the previous revision still owned, and once that revision was released, layout() read freed memory and crashed in ParagraphShadowNode::layout. Only keep content that has no attachments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
🤖 AI code reviewDecision: Ready for human review (with comments) Overall PR risk: Medium. This change modifies existing behavior for Paragraph content build and prepared-layout reuse across revisions. It affects text measurement and mounting, mainly on Android, with direct rollback by reverting the file. The one kept finding is that carried prepared layouts ignore point-scale change and can keep a stale layout after pixel-density change until a later remeasure. 🟡 Warning (1)
This review is advisory — it never blocks a merge and never auto-approves. |
…graph (#59162) Summary: Fixes #59161. When `LayoutAnimation` changes the height of a `<Text>` on iOS (for example a "read more" toggle from `numberOfLines={2}` to no limit), words jump between lines during the animation instead of the text unrolling with its final line breaks. **Current behavior.** LayoutAnimation interpolates the paragraph's `LayoutMetrics` on every frame. `RCTParagraphComponentView` uses that intermediate content frame as its drawing frame, so `drawRect:` lays the text out again in a container of the in-between size. With no line limit, `RCTTextLayoutManager` sets the container's `lineBreakMode` to `NSLineBreakByClipping`, which applies to the last line that fits: that line isn't word-wrapped, it takes the rest of the text and clips it at the right edge. So on every frame the last visible line shows words that belong to the next line, cut off at the edge, and they jump down once the next line is revealed. Lines also appear only once they fit completely. Width animations have the same problem sideways: the text re-wraps on every frame. The old architecture didn't have this problem. `RCTTextShadowView` laid the text out once, at the final frame size, and the animation only changed the view's frame. Android (with `enableLayoutAnimationsOnAndroid`) also unrolls height changes correctly. **With this change.** The cxx `ParagraphState` (used outside of Android) stores the size of the content frame the paragraph was laid out with. `ParagraphShadowNode` sets it in `layout()`, where the layout metrics are final. `RCTParagraphComponentView` lays the text out in that size and lets the text view clip it to its current bounds. LayoutAnimation already builds every intermediate view from the final one and never interpolates state ("keep the mounting layer consistent with the final state"), so this needs no animation-specific code. When nothing is animating, the stored size equals the current content frame size: layout metrics are only set together with a `layout()` call (`YogaLayoutableShadowNode::layout`, `layoutTree`). Static rendering is therefore unchanged, including text whose frame is shorter than its content. Android keeps its current state handling (`#ifdef ANDROID`): its views don't use the size, and a state update there goes through JNI and resets the text. I also tried the one-line alternative of using `NSLineBreakByWordWrapping` for unlimited lines. It stops the words from jumping, but lines still pop in only once they fit, and it changes the static rendering of text whose frame is shorter than its content. **Cost.** A paragraph whose size changes without a content change now gets a new `ParagraphState` on iOS. Such a size change already re-measures and redraws the text, and on current `main` the attributed string comparison in `updateStateIfNeeded` fails after layout anyway (see #59063), so iOS already creates a new state on most layouts. ## Changelog: [IOS] [FIXED] - Fix words jumping between lines while `LayoutAnimation` resizes a `Text` Pull Request resolved: #59162 Test Plan: **Before (left) and after (right)**, RNTester Playground, iPhone Air simulator (iOS 27.0), 3-second linear `LayoutAnimation`. Expand from `numberOfLines={2}` to no limit. Before: the last visible line is clipped at the edge and there is a gap below it. After: the final lines are revealed, and the partially visible line is clipped mid-line. <img width="1100" height="398" alt="expand-ba-frame" src="https://github.com/user-attachments/assets/a0ef8fad-5c70-4c67-a9a2-0e2cd2ee156f" /> Width 50% → 100%. Before: the text re-wraps on every frame. After: it keeps its final line breaks and is uncovered from the right. <img width="1100" height="398" alt="width-ba-frame" src="https://github.com/user-attachments/assets/6c04b9e9-d03d-4751-894e-12443065af37" /> Videos of both are at the end of the test plan. Repro used for the issue and the videos: ```tsx const TEXT = Array.from({length: 40}, (_, i) => `word${i + 1}`).join(' '); function Playground() { const [expanded, setExpanded] = useState(false); return ( <View style={{padding: 20}}> <Pressable onPress={() => { LayoutAnimation.configureNext({ duration: 3000, update: {type: LayoutAnimation.Types.linear}, }); setExpanded(value => !value); }}> <Text>{expanded ? 'Collapse' : 'Expand'}</Text> </Pressable> <Text style={{fontSize: 22}} numberOfLines={expanded ? undefined : 2}> {TEXT} </Text> </View> ); } ``` - **Static rendering.** Full-resolution screenshots of a screen with five static cases were pixel-identical before and after; only the status-bar clock differed. The cases were `height` shorter than the text, `numberOfLines={2}` with ellipsis, padding + border + `textAlign: 'center'` + `lineHeight`, nested styles with an inline view, and `flexShrink` in a fixed-height container. - **Fantom.** `yarn fantom packages/react-native/Libraries/Text/__tests__/Text-itest.js packages/react-native/Libraries/LayoutAnimation/__tests__/LayoutAnimation-itest.js` gives 157 passed. Fantom builds the same non-Android `ParagraphState` path as iOS. To build the tester locally I had to add `-Wno-error=deprecated-literal-operator`, because the bundled `nlohmann/json.hpp` fails with Xcode 27's clang; that's unrelated to this change. - **Cost.** I used a temporary Fantom benchmark, not part of this PR, that resizes a container of 100 or 1000 texts without changing their content. It ran in dev mode, since optimized mode isn't supported in OSS, 5 interleaved runs per variant. Medians were 4.07 ms → 3.44 ms for 100 texts and 50.9 ms → 42.2 ms for 1000 texts. Run-to-run noise (about ±30%) was larger than any difference. - **Android.** `./gradlew :packages:rn-tester:android:app:installDebug -PreactNativeArchitectures=arm64-v8a` builds. With `enableLayoutAnimationsOnAndroid` forced on locally, the expand animation unrolls correctly before and after this change. I noticed separately that Android re-wraps text during width animations, because `SurfaceMountingManager.updateLayout` calls `measure()` on every frame and `TextView` rebuilds its layout there; the old architecture doesn't. This PR doesn't address that. - **Formatting.** clang-format 21.1.2 (`--dry-run -Werror`) passes on the changed files. Expand, before (left) and after (right): https://github.com/user-attachments/assets/e33bd41a-b427-4e76-b936-219f03489685 Width animation, before (left) and after (right): https://github.com/user-attachments/assets/439db994-47a0-4f61-8259-3d8bcbcff2d8 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: cipolleschi Differential Revision: D126054764 Pulled By: javache fbshipit-source-id: bc40ec40dcafc692c081c54736f8f40a08cb684c
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Measured layouts are now carried over to clones with no new props or children, including the ones configureYogaTree makes when the pixel density changes. measureContent and findUsableLayout matched them only by constraints or size, so the paragraph reused a layout prepared at the old density and, with fixTextLayoutOnPixelDensityChange, never updated its state to the new density. Only reuse layouts prepared at the current pointScaleFactor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I also pushed 3ee16a3, which keeps #59049 working with this change. Claude's explanation:
|
|
@javache has imported this pull request. If you are a Meta employee, you can view this in D126131800. |
|
This pull request was successfully merged by @tomekzaw in e7766ab When will my fix make it into a release? | How to file a pick request? |
|
Reverting - this broke
|
|
This pull request has been reverted by ad2f451. |
… clones" (#59285) Summary: Re-land of #59063, which was reverted in ad2f451 because it broke `textAdjustsDynamicLayout` on Android. This PR has four commits: the Android fix for that regression, #59063 unchanged, fixes for the two prepared-layout cache concerns raised in the revert, and a fix for a crash in #59063 that I found while measuring memory. **#59063 (unchanged).** A `ParagraphShadowNode` cloned without new props or children (e.g. by Yoga, because a sibling changed) keeps its attributed string and measured layouts. Unchanged siblings then get no state update and no re-layout. On a Galaxy A16, changing one of 1000 labels takes 66% less time to pixels than on current `main` (benchmarks at the end). **1. Why `textAdjustsDynamicLayout` broke, and the fix (`ReactTextView`).** `ReactTextView` fits `adjustsFontSizeToFit` text by shrinking the font size spans of its Spannable in place. When the view grows, `onSizeChanged` schedules another fit, but that fit starts from the largest span it finds, which is the already-shrunk size. The text can shrink further but never grow back. It only grew back when a state update happened to deliver a new Spannable with the original sizes. #59063 removed the spurious state update the example relied on, so the text stayed small after the container grew. This is also a bug on `main` today, just one that depends on order: in RNTester, *Set Height to 60 → 20 → 40* right after opening the example leaves the text small at 40, because no state update lands on that last resize. With this change, the view keeps the font size spans the text had before the first fit and restores them, along with the paint's base text size, before each new fit. **2. Carried content is validated before its layouts are used.** `measureContent` looked up a carried layout before `getContent` checked that the carried content was built for the current font size multiplier and layout direction. With `enablePreparedTextLayout`, a font scale change while the app is running reused layouts measured at the old scale, so text was drawn at the new size into boxes measured at the old one and was cut off. The check now runs first. **3. Equal measured sizes don't imply equal layouts.** `findUsableLayout` matches a layout by its measured size. Text with `adjustsFontSizeToFit` can measure the same under different constraints with a different font size, so its prepared layouts are no longer carried across revisions. When several layouts of the same size exist, the one measured for the current revision is preferred over carried ones. **4. Prepared layouts no longer pile up across revisions (crash fix).** #59063 copied every prepared layout of the source into the clone, and new measurements were added to them, so when only the constraints kept changing (e.g. a container being resized while its texts stay the same) the layouts accumulated across revisions. Each one holds a JNI global reference on Android, and with `enablePreparedTextLayout` the app aborted with *JNI ERROR: global reference table overflow* after about 20 resizes of a container with 300 texts. Now `layout()` keeps only the layout it used and a clone carries at most that one, so a paragraph holds one carried layout plus those measured in its own revision. ## Changelog: [GENERAL] [FIXED] - Avoid state updates and re-layout of unchanged `Text` siblings when a sibling changes [ANDROID] [FIXED] - `adjustsFontSizeToFit` text grows back when its container grows Pull Request resolved: #59285 Test Plan: - `./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests "com.facebook.react.views.text.*"`: 94 tests pass. The new `adjustsFontSizeToFitGrowsBackWhenContainerHeightGrows` fails without the `ReactTextView` change (font size stays at 4 instead of 40). - `yarn format-check-java`, `format-check-kotlin` and `format-check-cpp` pass on the changed files. - RNTester `textAdjustsDynamicLayout`, release build on an Android emulator: | steps | `main` | #59063 alone | this PR | |---|---|---|---| | open → 40 | grows | stays small | grows | | open → 60 → 20 → 40 | stays small | stays small | grows | - Live changes compared pixel by pixel with a fresh remount of the same state, using a local RNTester screen (not part of this PR) with an `adjustsFontSizeToFit` text, mixed LTR/RTL texts, nested spans and a counter. Release builds; Android on a Galaxy A16, iOS on an iPhone Air simulator (iOS 26.5). ✓ = identical to the remount. | live change | Android default | Android `enablePreparedTextLayout` | iOS | |---|---|---|---| | container height 20 → 40 → 60 → 20 → 40 (`adjustsFontSizeToFit`) | ✓ (#59063 alone: ✗) | ✓ | ✓ | | `direction` LTR → RTL → LTR on the parent | ✓ | ✓ | ✓ | | update one sibling's text | ✓ | ✓ | ✓ | | font scale 1.0 → 1.3 → 1.0 / Dynamic Type L → XXL → L | ✓ | ✓ (#59063 alone: ✗, text clipped) | ✓ | | density 450 → 360 → 450 dpi, `fixTextLayoutOnPixelDensityChange` on | ✓ | ✓ | n/a | - Text with inline views (Text examples → *Inline views*, including the RTL ones), both Android text paths: four live font scale changes, no crash, text reflows correctly. - Not run: Fantom (the bundled `nlohmann/json.hpp` doesn't build with this machine's Apple clang, so I'm relying on CI), and a physical iPhone. **Benchmarks.** Samsung Galaxy A16, RNTester release builds of current `main` (c7429b0) and this PR, with a text benchmark screen that is not part of this PR. The two builds alternated round by round; medians of 12 runs (4 rounds × 3 runs after a warm-up run), battery 27.6–27.9 °C. | changing one item, right after mount | `main` | this PR | change | |---|--:|--:|--:| | 1000 labels: time to pixels | 225 ms | 77 ms | −66% | | 1000 labels: UI-thread CPU | 178 ms | 38 ms | −79% | | 1000 labels, `enablePreparedTextLayout`: time to pixels | 250 ms | 74 ms | −70% | | 1000 labels, `enablePreparedTextLayout`: JS-thread CPU | 153 ms | 41 ms | −74% | | 300 strings: time to pixels | 149 ms | 36 ms | −76% | | 300 strings: UI-thread CPU | 139 ms | 35 ms | −75% | | 100-post feed: time to pixels | 95 ms | 88 ms | −7% | Mounts: time to pixels is unchanged (−2% to +4%, within run-to-run noise). With `enablePreparedTextLayout`, mounting 1000 labels uses about 10 ms more JS-thread CPU and about 40 ms more total CPU (+9%) in every round, probably from copying the prepared layouts into clones, without a change in time to pixels. Two earlier rounds were left out: in them both builds produced many extra frames per update, probably because Android Studio's screen mirroring was attached to the phone at the time. This PR was still faster in those rounds (1000 labels: 164 ms vs 2406 ms). **Prepared layouts per update** (1000 labels, `enablePreparedTextLayout`, counted with a temporary log in `measureContent`): changing one label prepares 1001 layouts on `main` (every sibling) and 1 with this PR. The fourth commit doesn't change this. **Memory.** Android emulator (Pixel 10 Pro AVD, API 37), release builds, `dumpsys meminfo` total PSS 10 s after the sequence with the tree still mounted, medians of 3 runs: | sequence | `main` | this PR | change | |---|--:|--:|--:| | 1000 labels, 20 single-label updates | 117.5 MB | 110.1 MB | −6% | | same, `enablePreparedTextLayout` | 136.4 MB | 109.2 MB | −20% (Java heap 23.3 → 9.1 MB) | | 300 labels, container resized 120 times (40 widths) | 101.7 MB | 94.6 MB | −7% | | same, `enablePreparedTextLayout` | 120.1 MB | 121.0 MB | +1% | | same, 400 resizes, `enablePreparedTextLayout` | 143.7 MB | 144.3 MB | +0% | Without the fourth commit, the prepared resize sequences crashed (see 4. above). The live-change comparisons, the `textAdjustsDynamicLayout` table and the Galaxy A16 benchmarks above were run before the fourth commit, which changes only which prepared layouts a clone keeps; the layout counts and memory numbers include it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: christophpurrer Differential Revision: D126622505 Pulled By: javache fbshipit-source-id: 490447c113263f4335ffc7ff4ffc540751518a6a
Summary:
When Yoga lays out a container again, it clones every child whose layout it touches, including unchanged siblings. A
ParagraphShadowNodecloned that way (no new props, no new children) drops its memoizedcontent_(the attributed string) andmeasuredLayouts_.Current behavior. In
layout()the clone rebuilds itsAttributedStringand compares it with the one in itsParagraphState. After the paragraph's first commit the comparison fails even though nothing changed.Fragment::operator==comparesparentShadowView.layoutMetrics, and the string stored in state captured those metrics before the paragraph had been laid out. SoupdateStateIfNeededcreates a new state for every sibling paragraph:UPDATE_STATEmount instruction. The UI thread rebuilds its Spannable, callssetText, lays the text out again and redraws it.ReactTextViewManager.updateState, 1000setTextand 1000onDrawcalls for the one change.enablePreparedTextLayout, the clone also has no measured layout to reuse, so every sibling is laid out again on the JS thread.With this change. A clone whose fragment has neither props nor children keeps
content_andmeasuredLayouts_.getContentdrops both if the font-size multiplier or layout direction they were built for no longer matches. Unchanged siblings then produce no state update and no re-layout.A kept layout is only reused at the pixel density it was prepared for. After a density change the paragraph prepares a new layout, so the state refresh from #59049 (
fixTextLayoutOnPixelDensityChange) still applies withenablePreparedTextLayout.Content with attachments (inline views inside the text) is not kept: the attachments point at the children the content was built from, and
layout()replaces those children with laid-out clones, so in a later revision they would point at nodes only the previous revision owns.Why. Any update inside a container with many text siblings (non-virtualized lists, settings screens, forms, tables) currently costs UI-thread work for every sibling, and with prepared text layout also a JS-thread layout for every sibling.
Changelog:
[GENERAL] [FIXED] - Avoid state updates and re-layout of unchanged
Textsiblings when a sibling changesTest Plan:
./gradlew :packages:rn-tester:android:app:assembleDebug -PreactNativeArchitectures=arm64-v8aandyarn format-check-cpppass on this branch.nlohmann/json.hppfails with this machine's Apple clang. I'm relying on CI for it.enablePreparedTextLayout.-fsyntax-onlybut not run; no density change was tested on a device.ParagraphShadowNode::layout, reading an attachment the previous revision had released), the current version in none.Benchmarks. RNTester release build with a text benchmark screen that is not part of this PR. Medians, main vs. this PR, variants interleaved.
Emulator (Pixel 10 Pro AVD, API 37), current
mainvs. this branch, 6 runs:enablePreparedTextLayoutPhysical Samsung Galaxy A16, the same change on an earlier
main(d4d0813969c), 8 runs. #59016 was applied in both arms.🤖 Generated with Claude Code