Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed (iOS) Relayout not Working for Nested Inline #41352

Closed
wants to merge 2 commits into from

Conversation

ehsemloh
Copy link
Contributor

@ehsemloh ehsemloh commented Nov 7, 2023

Summary:

React Native, the text inline is made versatile by design. Being managed in an alien layout logic (i.e., text paragraph), inline views work seamlessly as if in normal flex layout. The capacities such as animation and relayout, however, requires extra efforts on native layer.

This PR fixed one critical issue for inline, i.e., setState() is not working when inline contains nested views.

Closes #41348

Demo (Fixed)

Screen.Recording.2023-11-07.at.20.42.57.mov

Problem in technical:

This issue is caused by a bug in RCTShadowView::sizeThatFitsMinimumSize() which accidentally unlink children (of yoga nodes) with their parent (owner). More specifically, on the critical path, it

  1. first shallow clones the current node
  YGNodeRef clonedYogaNode = YGNodeClone(self.yogaNode);
  1. then calls YGNodeCalculateLayout() using the cloned node
  2. deallocate the cloned node YGNodeFree()

One unseen implication of YGNodeFree() is to unlink all its children (because of the shallow clone)

  for (size_t i = 0; i < childCount; i++) {
    auto child = node->getChild(i);
    child->setOwner(nullptr);
  }

Next, let's examine,

How nullptr of owner can cause the broken setState() of nested inline views

The orphan children has two consequences:
a. the changes on child node (setState()) cannot be propagated to the parent (YGNodeMarkDirty -> node->markDirtyAndPropagate(););
b. YGNodeCalculateLayout() (yoga::calculateLayoutImpl) will create new children instances when orphan is detected (see below)

  node->cloneChildrenIfNeeded(); // line 1599 # CalculateLayout.cpp

Both compounded are contributing the failed setState(). Respectively,
a causes early return of YGNodeCalculateLayout() because parent is recognized as not dirty;
b clones a new dirty node which replaces the child which is supposed to be cleaned within YGNodeCalculateLayout(). And this is the dirty node detected by the assertion mentioned in the issue description #41348.

The fix:

The fix introduced in this PR is to relink the children with their parent in RCTShadowView::sizeThatFitsMinimumSize()

Changelog:

[IOS] [FIXED] - setState is not working for nested inline views in text

Test Plan:

Test directly in rn-tester
https://docs.google.com/spreadsheets/d/1IJg64d27p3JXiGrDYgqEGyroDn8mk4KgI-LISUOSEQg/edit?usp=sharing

@facebook-github-bot 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 Nov 7, 2023
@NickGerleman
Copy link
Contributor

LGTM

@facebook-github-bot
Copy link
Contributor

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

Copy link

This pull request was successfully merged by @ehsemloh in 9b33e75.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Nov 10, 2023
Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
Summary:
React Native, the text inline is made versatile by design. Being managed in an alien layout logic (i.e., text paragraph), inline views work seamlessly as if in normal **flex** layout. The capacities such as animation and relayout, however, requires extra efforts on native layer.

This PR fixed one critical issue for inline, i.e., `setState()` is not working when inline contains nested views.

Closes facebook#41348

Demo (Fixed)

https://github.com/facebook/react-native/assets/149237137/2b42d657-4024-476b-bf0c-be25ef4f8c0c

## Problem in technical:

This issue is caused by a bug in `RCTShadowView::sizeThatFitsMinimumSize()` which accidentally unlink children (of yoga nodes) with their parent (owner). More specifically, on the critical path, it
1. first **shallow** clones the current node
```
  YGNodeRef clonedYogaNode = YGNodeClone(self.yogaNode);
```
2. then calls `YGNodeCalculateLayout()` using the cloned node
3. deallocate the cloned node `YGNodeFree()`

One unseen implication of `YGNodeFree()` is to unlink all its children (because of the **shallow** clone)

```
  for (size_t i = 0; i < childCount; i++) {
    auto child = node->getChild(i);
    child->setOwner(nullptr);
  }
```

Next, let's examine,

**How nullptr of owner can cause the broken `setState()` of nested inline views**

The orphan children has two consequences:
**a**. the changes on child node (`setState()`) cannot be propagated to the parent (`YGNodeMarkDirty` -> `node->markDirtyAndPropagate();`);
**b**. `YGNodeCalculateLayout()` (`yoga::calculateLayoutImpl`) will create new children instances when orphan is detected (see below)

```
  node->cloneChildrenIfNeeded(); // line 1599 # CalculateLayout.cpp
```

Both compounded are contributing the failed `setState()`. Respectively,
**a** causes early return of `YGNodeCalculateLayout()` because parent is recognized as not *dirty*;
**b** clones a new *dirty* node which replaces the child which is supposed to be *cleaned* within `YGNodeCalculateLayout()`. And this is the *dirty* node detected by the assertion mentioned in the issue description facebook#41348.

## The fix:

The fix introduced in this PR is to relink the children with their parent in `RCTShadowView::sizeThatFitsMinimumSize()`

## Changelog:

[IOS] [FIXED] - `setState` is not working for nested inline views in text

Pull Request resolved: facebook#41352

Test Plan:
Test directly in rn-tester
TBD

Reviewed By: yungsters

Differential Revision: D51071338

Pulled By: NickGerleman

fbshipit-source-id: 1f3d8a3e1e03cb11577f903e43f2c2cce9e07b6e
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. Merged This PR has been merged. 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 Relayout of Nested Inline View Doesn't Work
3 participants