Skip to content

Fix text inside Modal not updating after a live font scale change - #59383

Open
simongt wants to merge 6 commits into
react:mainfrom
simongt:fix/font-scale-dirty-owners
Open

simongt wants to merge 6 commits into
react:mainfrom
simongt:fix/font-scale-dirty-owners

Conversation

@simongt

@simongt simongt commented Oct 23, 2026 •

Copy link
Copy Markdown

Summary:

Fixes #59375.

On iOS, text inside a <Modal> keeps its old size after a live text size change, and the same happens under react-native-screens screens. On the font scale pass, configureYogaTree clones the modal in place, and its descriptor's setSize() marks the clone dirty without its parent. The text's dirty flag then stops at the modal on its way up, the modal's parent looks unchanged, and Yoga reuses its cached layout for the whole subtree. An input-like node with no modal above it goes stale the same way, since completeClone() dirties its in-place clone without the parent.

configureYogaTree now applies the rule updateYogaChildren() uses when a parent is rebuilt: as the walk unwinds, a node marks itself dirty if a child it just configured came back dirty. Hidden (display: none) nodes are skipped, since they lay out no children.

The RTL test asked for in review also showed why a direction override under a root that swaps left and right (swapLeftAndRightInRTL) was configured again on every pass. configureYogaTree skips a configured child when its stored wasLeftAndRightSwapped matches the swap passed down from the root, but layout() stored the child's own direction, so an override never matched. Each pass cloned that subtree's children in place, and with the rule above, a clone that came back dirty got laid out again. layout() now stores the swap that was applied, taken from the parent, so the subtree is only configured again when that swap changes. Nothing else reads wasLeftAndRightSwapped.

Changelog:

[GENERAL] [FIXED] - Text inside Modal now updates after a live font scale change

Test Plan:

Added four tests to ViewTest.cpp, on a FontScaleLayoutTest fixture with a text-like and an input-like measurable leaf whose size follows the font scale:

  • fontScaleChangeLaysOutMeasurableNodesInsideModal: both leaves inside a ModalHostView, at font scale 1, 1.5, then 1. Fails without the change (the stored scale stays 1 and the height stays 10 instead of 15), passes with it.
  • fontScaleChangeLaysOutInputLikeNodeWithoutModal: the input-like leaf with no modal. Fails without the change, passes with it.
  • fontScaleChangeSkipsHiddenSubtreeUntilShown: a font scale change under a display: none view doesn't lay the hidden view out, and the leaf gets the new scale once it's shown. Passes without the change, and fails if the display: none check is removed.
  • rtlDirectionOverrideIsConfiguredOnlyWhenTheSwapChanges: a direction: ltr subtree with the input-like leaf under a root that swaps left and right. Turning the root RTL configures the subtree again and lays the leaf out. An unrelated commit after that keeps the geometry, doesn't clone the leaf, and leaves it out of affectedNodes. Fails without the layout() change, passes with it.

With only setSize() and setPositionType() dirtying on change instead, the input leaf in the modal test still fails.

There's no OSS runner for these gtests, so I ran ViewTest.cpp with a local CMake and googletest setup on macOS (Homebrew folly and glog rather than the pinned versions), with -Wall -Wextra -Werror on the two changed files: 20/20 pass with the change. Formatted with clang-format 21.0.0 (the repo pins 21.1.2).

On an iPhone 17 Pro simulator (iOS 26.5), with React Native built from source:

  • The repro from [iOS][Fabric] Text inside Modal keeps its old size after a live text size change (works on 0.86.3) #59375 (https://github.com/simongt/rn-modal-text-size-repro) on 0.88.0 with this change: after a live change to XXXL or AX-XXXL and back, the screen matches a fresh launch at the same size every time. Without the change, the text in the modal keeps the default size.
  • The same repro with RTL forced, plus a direction: 'ltr' view next to a view whose width changes over six commits. On stock 0.88.0 each of those commits cloned the two children of the direction: 'ltr' view in place; with this change none did. A fresh launch matches stock 0.88.0 at the default size and at XXXL, and a live change to XXXL and back matches a fresh launch.
  • Our app, which uses an expo-router native stack: going to XXXL and back to the default size now returns to the starting screen. Before, the text stayed large until the screen remounted.

Not tested on Android.

Text under a Modal, an InputAccessoryView or a react-native-screens
screen kept its old size after a live font scale change on iOS.
setSize(), which those descriptors call from adopt(), dirties a node
without its ancestors, and markDirtyAndPropagate() stops at the first
dirty owner. The clean parent above it then reuses its cached layout,
so the subtree never gets the new metrics.

Walk every owner in the font scale block of configureYogaTree instead,
and stop at a display: none owner, which lays out no children.
@meta-cla

meta-cla Bot commented Oct 23, 2026

Copy link
Copy Markdown

Hi @simongt!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@meta-cla

meta-cla Bot commented Oct 23, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 23, 2026
@meta-cla

meta-cla Bot commented Oct 23, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-tools facebook-github-tools Bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Oct 23, 2026

@javache javache left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configureYogaTree is already supposed to walk the tree, so adding an extra walk here seems incorrect.

How about fixing ModalHostView so it recurses into its children for these calls, even if it's already dirty? Or changing the top-level bailout for IsDirty here? The current one does not seem sufficient to handle this.

configureYogaTree already walks the tree, so the extra walk over the
owners is gone. Instead, apply the rule of updateYogaChildren() as the
walk unwinds: after configuring a child, a node marks itself dirty when
the child came back dirty, and display: none nodes are skipped. This
covers clones dirtied by setSize() in a descriptor's adopt(), such as a
Modal, and clones dirtied by completeClone(), such as a TextInput with
no Modal above it.

Replace the hidden-node test, which passed without the fix, and drop the
unchanged-scale guard, which could not fail. Add a test for an
input-like node without a Modal, and one that a hidden subtree is not
laid out until it is shown.
@simongt

simongt commented Oct 23, 2026

Copy link
Copy Markdown
Author

Hey @javache, thanks for looking this over.

configureYogaTree is already supposed to walk the tree, so adding an extra walk here seems incorrect.

Moved the fix into the existing configureYogaTree walk, so the extra pass is gone.

One clarification on the root cause: the walk down is fine, it reaches the text under the modal and marks it dirty. The break is on the way up. When the font scale changes, the modal gets cloned in place, and its descriptor's setSize() marks the clone dirty without telling its parent. So the text's dirty flag stops at the modal on its way up (it's already dirty), the modal's parent looks unchanged, and Yoga reuses its cached layout for that whole subtree.

How about fixing ModalHostView so it recurses into its children for these calls, even if it's already dirty? Or changing the top-level bailout for IsDirty here? The current one does not seem sufficient to handle this.

On your suggestions:

  • Making setSize() and setPositionType() dirty only when the value changes, like setPadding() already does, fixes the modal. But an in-place TextInput clone gets dirtied the same way in completeClone(), so the input case in the test still fails with only that change.
  • If you meant the early return in markDirtyAndPropagate(), removing it would make every Yoga style setter walk to the root.

So now, as the existing walk unwinds, a node marks itself dirty if a child it just configured came back dirty, the same rule updateYogaChildren() uses when a parent is rebuilt. Hidden nodes are skipped, and it's still behind the font scale flag. It also turned out to be wider than the modal: an input with no modal above it goes stale on main the same way, so there's a test for that now, plus one that a hidden subtree isn't laid out until it's shown (updated in c037ba4).


Ok to fold the setSize() and setPositionType() optimization into this PR as well? (If you'd rather not have it here, I can post that change as a separate PR.)

Upstream removed enableFontScaleChangesUpdatingLayout (react#59389). The
conflict is in the font block of configureYogaTree: keep the upstream
condition without the flag, and keep our comment on it. The post-order
dirty check in the child loop also used the flag, so it is ungated now,
like the font block.
@simongt

simongt commented Oct 27, 2026

Copy link
Copy Markdown
Author

Merged main to pick up the font scale flag removal. The new check in the child loop was behind the same flag, so it's ungated too, same as the font block. The modal and input tests still fail without it and pass with it.

Since this file has been changing a lot lately, I'm a bit worried the PR will keep drifting and picking up conflicts the longer it sits. Could you please take another look when you get a chance? Thanks @javache

// An in-place clone can come back dirty without dirtying this node, from
// `setSize()` in a descriptor's `adopt()` or from `completeClone()`.
// Apply the rule of `updateYogaChildren()` so Yoga doesn't skip it.
if (yogaLayoutableChildren_[i]->yogaNode_.isDirty() &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it explicit that we need to re-read yogaLayoutableChildren_[i] because cloneChildInPlace may have changed.

Suggested change
if (yogaLayoutableChildren_[i]->yogaNode_.isDirty() &&
const auto& updatedYogaNode = yogaLayoutableChildren_[i]->yogaNode_;
if (updatedYogaNode.isDirty() &&

@simongt simongt Oct 28, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied, thanks (child still points at the node from before the clone, so the name makes the re-read deliberate).

@javache javache left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One non-blocking test suggestion: cover an RTL root with a direction override under swapLeftAndRightInRTL, followed by an unrelated commit with unchanged font scale. This PR explicitly changes ancestor layout behaviour in that case, but the new tests don't cover it. Assert stable geometry and the expected affectedNodes behaviour.

@meta-codesync

meta-codesync Bot commented Oct 28, 2026

Copy link
Copy Markdown

@javache has imported this pull request. If you are a Meta employee, you can view this in D128040309.

`cloneChildInPlace()` replaces `yogaLayoutableChildren_[i]`, so `child`
still refers to the node from before the clone. Name the re-read node so
the post-order dirty check visibly reads the configured clone.
Under an RTL root that swaps left and right, a `direction: ltr` subtree
stores no swap in its layout metrics, so `configureYogaTree` configures
it again on every pass and clones its children in place. An input-like
leaf comes back dirty from that clone, and the post-order dirty check
lays it out again. Pin that the geometry stays the same and which nodes
are affected.
`configureYogaTree` skips a configured child when its stored
`wasLeftAndRightSwapped` matches the swap passed down from the root.
`layout()` stored the child's own direction instead, so a `direction`
override under a root that swaps left and right never matched. That
subtree was configured again and cloned in place on every pass, and a
clone that came back dirty was laid out again.

`wasLeftAndRightSwapped` has no other reader. Store the swap that was
applied, taken from the parent, so the subtree is configured again only
when the swap changes. The RTL test now flips the root to RTL, then
asserts that an unrelated commit keeps the geometry, leaves the leaf out
of `affectedNodes`, and does not clone it.
@simongt

simongt commented Oct 28, 2026

Copy link
Copy Markdown
Author

One non-blocking test suggestion: cover an RTL root with a direction override under swapLeftAndRightInRTL, followed by an unrelated commit with unchanged font scale. This PR explicitly changes ancestor layout behaviour in that case, but the new tests don't cover it. Assert stable geometry and the expected affectedNodes behaviour.

Added the test (which showed that subtree re-configured every pass because layout() stored each node's own direction). It now stores the applied swap, so unrelated commits skip it. Ready for another look whenever you get a chance.

@simongt
simongt requested a review from javache October 28, 2026 17:50

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[iOS][Fabric] Text inside Modal keeps its old size after a live text size change (works on 0.86.3)

2 participants