Skip to content

Commit e24ce70

Browse files
grahammendickfacebook-github-bot
grahammendick
authored and
facebook-github-bot
committed
Migrate needsCustomLayoutForChildren check to the new architecture (#34254)
Summary: Fixes #34120 The new React Native architecture doesn't check `needsCustomLayoutForChildren` so it wrongly positions native views on Android. In #34120 there are videos comparing the positioning of a native action view in the old and the new architecture. This PR passes the parent tag to the `updateLayout` method of the `SurfaceMountingManager`. The `SurfaceMountingManager` calls `needsCustomLayoutForChildren` on the parent view manager (copied the code from the `NativeViewHierarchyManager` in the old architecture). **NOTE** - I wasn't sure where to get the parent shadow view from so I've put in my best guesses where I could and left it as `{}` otherwise. ## Changelog [Android] [Fixed] - Migrate `needsCustomLayoutForChildren` check to the new architecture Pull Request resolved: #34254 Test Plan: I checked the fix in the repro from #34165. Here is a video of the action view closing using the native button that is now visible in the new architecture. https://user-images.githubusercontent.com/1761227/180607896-35bf477f-4552-4b8a-8e09-9e8c49122c0c.mov Reviewed By: cipolleschi Differential Revision: D38153924 Pulled By: javache fbshipit-source-id: e2c77fa70d725a33ce73fe4a615f6d884312580c
1 parent 1ce23ce commit e24ce70

File tree

10 files changed

+53
-31
lines changed

10 files changed

+53
-31
lines changed

‎ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,10 @@ CppMountItem CppMountItem::UpdatePropsMountItem(
4444
CppMountItem CppMountItem::UpdateStateMountItem(ShadowView const &shadowView) {
4545
return {CppMountItem::Type::UpdateState, {}, {}, shadowView, -1};
4646
}
47-
CppMountItem CppMountItem::UpdateLayoutMountItem(ShadowView const &shadowView) {
48-
return {CppMountItem::Type::UpdateLayout, {}, {}, shadowView, -1};
47+
CppMountItem CppMountItem::UpdateLayoutMountItem(
48+
ShadowView const &shadowView,
49+
ShadowView const &parentView) {
50+
return {CppMountItem::Type::UpdateLayout, parentView, {}, shadowView, -1};
4951
}
5052
CppMountItem CppMountItem::UpdateEventEmitterMountItem(
5153
ShadowView const &shadowView) {

‎ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,9 @@ struct CppMountItem final {
4646

4747
static CppMountItem UpdateStateMountItem(ShadowView const &shadowView);
4848

49-
static CppMountItem UpdateLayoutMountItem(ShadowView const &shadowView);
49+
static CppMountItem UpdateLayoutMountItem(
50+
ShadowView const &shadowView,
51+
ShadowView const &parentView);
5052

5153
static CppMountItem UpdateEventEmitterMountItem(ShadowView const &shadowView);
5254

‎ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ static inline int getIntBufferSizeForType(CppMountItem::Type mountItemType) {
7979
case CppMountItem::Type::UpdatePadding:
8080
return 5; // tag, top, left, bottom, right
8181
case CppMountItem::Type::UpdateLayout:
82-
return 6; // tag, x, y, w, h, DisplayType
82+
return 7; // tag, parentTag, x, y, w, h, DisplayType
8383
case CppMountItem::Type::UpdateOverflowInset:
8484
return 5; // tag, left, top, right, bottom
8585
case CppMountItem::Undefined:
@@ -381,7 +381,7 @@ void FabricMountingManager::executeMount(
381381
newChildShadowView.layoutMetrics) {
382382
cppUpdateLayoutMountItems.push_back(
383383
CppMountItem::UpdateLayoutMountItem(
384-
mutation.newChildShadowView));
384+
mutation.newChildShadowView, parentShadowView));
385385
}
386386

387387
// OverflowInset: This is the values indicating boundaries including
@@ -441,7 +441,8 @@ void FabricMountingManager::executeMount(
441441

442442
// Layout
443443
cppUpdateLayoutMountItems.push_back(
444-
CppMountItem::UpdateLayoutMountItem(newChildShadowView));
444+
CppMountItem::UpdateLayoutMountItem(
445+
newChildShadowView, parentShadowView));
445446

446447
// OverflowInset: This is the values indicating boundaries including
447448
// children of the current view. The layout of current view may not
@@ -548,7 +549,7 @@ void FabricMountingManager::executeMount(
548549
int intBufferPosition = 0;
549550
int objBufferPosition = 0;
550551
int prevMountItemType = -1;
551-
jint temp[6];
552+
jint temp[7];
552553
for (int i = 0; i < cppCommonMountItems.size(); i++) {
553554
const auto &mountItem = cppCommonMountItems[i];
554555
const auto &mountItemType = mountItem.type;
@@ -723,13 +724,14 @@ void FabricMountingManager::executeMount(
723724
toInt(mountItem.newChildShadowView.layoutMetrics.displayType);
724725

725726
temp[0] = mountItem.newChildShadowView.tag;
726-
temp[1] = x;
727-
temp[2] = y;
728-
temp[3] = w;
729-
temp[4] = h;
730-
temp[5] = displayType;
731-
env->SetIntArrayRegion(intBufferArray, intBufferPosition, 6, temp);
732-
intBufferPosition += 6;
727+
temp[1] = mountItem.parentShadowView.tag;
728+
temp[2] = x;
729+
temp[3] = y;
730+
temp[4] = w;
731+
temp[5] = h;
732+
temp[6] = displayType;
733+
env->SetIntArrayRegion(intBufferArray, intBufferPosition, 7, temp);
734+
intBufferPosition += 7;
733735
}
734736
}
735737
if (!cppUpdateOverflowInsetMountItems.empty()) {

‎ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -967,7 +967,8 @@ public void sendAccessibilityEvent(int reactTag, int eventType) {
967967
}
968968

969969
@UiThread
970-
public void updateLayout(int reactTag, int x, int y, int width, int height, int displayType) {
970+
public void updateLayout(
971+
int reactTag, int parentTag, int x, int y, int width, int height, int displayType) {
971972
if (isStopped()) {
972973
return;
973974
}
@@ -992,9 +993,14 @@ public void updateLayout(int reactTag, int x, int y, int width, int height, int
992993
parent.requestLayout();
993994
}
994995

995-
// TODO: T31905686 Check if the parent of the view has to layout the view, or the child has
996-
// to lay itself out. see NativeViewHierarchyManager.updateLayout
997-
viewToUpdate.layout(x, y, x + width, y + height);
996+
ViewState parentViewState = getViewState(parentTag);
997+
ViewGroupManager<?> parentViewManager = null;
998+
if (parentViewState.mViewManager != null) {
999+
parentViewManager = parentViewState.mViewManager.getViewGroupManager();
1000+
}
1001+
if (parentViewManager == null || !parentViewManager.needsCustomLayoutForChildren()) {
1002+
viewToUpdate.layout(x, y, x + width, y + height);
1003+
}
9981004

9991005
// displayType: 0 represents display: 'none'
10001006
int visibility = displayType == 0 ? View.INVISIBLE : View.VISIBLE;

‎ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,13 +149,15 @@ public void execute(@NonNull MountingManager mountingManager) {
149149
surfaceMountingManager.updateState(mIntBuffer[i++], castToState(mObjBuffer[j++]));
150150
} else if (type == INSTRUCTION_UPDATE_LAYOUT) {
151151
int reactTag = mIntBuffer[i++];
152+
int parentTag = mIntBuffer[i++];
152153
int x = mIntBuffer[i++];
153154
int y = mIntBuffer[i++];
154155
int width = mIntBuffer[i++];
155156
int height = mIntBuffer[i++];
156157
int displayType = mIntBuffer[i++];
157158

158-
surfaceMountingManager.updateLayout(reactTag, x, y, width, height, displayType);
159+
surfaceMountingManager.updateLayout(
160+
reactTag, parentTag, x, y, width, height, displayType);
159161

160162
} else if (type == INSTRUCTION_UPDATE_PADDING) {
161163
surfaceMountingManager.updatePadding(

‎ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ void LayoutAnimationDriver::animationMutationsForFrame(
5757

5858
// Create the mutation instruction
5959
mutationsList.emplace_back(ShadowViewMutation::UpdateMutation(
60-
keyframe.viewPrev, mutatedShadowView));
60+
keyframe.viewPrev, mutatedShadowView, keyframe.parentView));
6161

6262
PrintMutationInstruction("Animation Progress:", updateMutation);
6363

‎ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1241,7 +1241,9 @@ void LayoutAnimationKeyFrameManager::queueFinalMutationsForCompletedKeyFrame(
12411241
break;
12421242
case ShadowViewMutation::Type::Update:
12431243
mutationsList.push_back(ShadowViewMutation::UpdateMutation(
1244-
prev, finalMutation.newChildShadowView));
1244+
prev,
1245+
finalMutation.newChildShadowView,
1246+
finalMutation.parentShadowView));
12451247
break;
12461248
}
12471249
if (finalMutation.newChildShadowView.tag > 0) {
@@ -1266,7 +1268,7 @@ void LayoutAnimationKeyFrameManager::queueFinalMutationsForCompletedKeyFrame(
12661268
auto mutatedShadowView =
12671269
createInterpolatedShadowView(1, keyframe.viewStart, keyframe.viewEnd);
12681270
auto generatedPenultimateMutation = ShadowViewMutation::UpdateMutation(
1269-
keyframe.viewPrev, mutatedShadowView);
1271+
keyframe.viewPrev, mutatedShadowView, keyframe.parentView);
12701272
react_native_assert(
12711273
generatedPenultimateMutation.oldChildShadowView.tag > 0);
12721274
react_native_assert(
@@ -1277,7 +1279,7 @@ void LayoutAnimationKeyFrameManager::queueFinalMutationsForCompletedKeyFrame(
12771279
mutationsList.push_back(generatedPenultimateMutation);
12781280

12791281
auto generatedMutation = ShadowViewMutation::UpdateMutation(
1280-
mutatedShadowView, keyframe.viewEnd);
1282+
mutatedShadowView, keyframe.viewEnd, keyframe.parentView);
12811283
react_native_assert(generatedMutation.oldChildShadowView.tag > 0);
12821284
react_native_assert(generatedMutation.newChildShadowView.tag > 0);
12831285
PrintMutationInstruction(
@@ -1286,7 +1288,7 @@ void LayoutAnimationKeyFrameManager::queueFinalMutationsForCompletedKeyFrame(
12861288
mutationsList.push_back(generatedMutation);
12871289
} else {
12881290
auto mutation = ShadowViewMutation::UpdateMutation(
1289-
keyframe.viewPrev, keyframe.viewEnd);
1291+
keyframe.viewPrev, keyframe.viewEnd, keyframe.parentView);
12901292
PrintMutationInstruction(
12911293
logPrefix +
12921294
"Animation Complete: Queuing up Final Synthetic Mutation:",

‎ReactCommon/react/renderer/mounting/Differentiator.cpp‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,7 @@ static void updateMatchedPair(
574574
if (oldPair.shadowView != newPair.shadowView) {
575575
mutationContainer.updateMutations.push_back(
576576
ShadowViewMutation::UpdateMutation(
577-
oldPair.shadowView, newPair.shadowView));
577+
oldPair.shadowView, newPair.shadowView, parentShadowView));
578578
}
579579
}
580580
}
@@ -835,7 +835,9 @@ static void calculateShadowViewMutationsFlattener(
835835
newTreeNodePair.isConcreteView && oldTreeNodePair.isConcreteView) {
836836
mutationContainer.updateMutations.push_back(
837837
ShadowViewMutation::UpdateMutation(
838-
oldTreeNodePair.shadowView, newTreeNodePair.shadowView));
838+
oldTreeNodePair.shadowView,
839+
newTreeNodePair.shadowView,
840+
node.shadowView));
839841
}
840842

841843
// Update children if appropriate.
@@ -1161,7 +1163,9 @@ static void calculateShadowViewMutationsV2(
11611163
oldChildPair.shadowView != newChildPair.shadowView) {
11621164
mutationContainer.updateMutations.push_back(
11631165
ShadowViewMutation::UpdateMutation(
1164-
oldChildPair.shadowView, newChildPair.shadowView));
1166+
oldChildPair.shadowView,
1167+
newChildPair.shadowView,
1168+
parentShadowView));
11651169
}
11661170

11671171
// Recursively update tree if ShadowNode pointers are not equal
@@ -1699,7 +1703,7 @@ ShadowViewMutation::List calculateShadowViewMutations(
16991703

17001704
if (oldRootShadowView != newRootShadowView) {
17011705
mutations.push_back(ShadowViewMutation::UpdateMutation(
1702-
oldRootShadowView, newRootShadowView));
1706+
oldRootShadowView, newRootShadowView, {}));
17031707
}
17041708

17051709
calculateShadowViewMutationsV2(

‎ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,11 @@ ShadowViewMutation ShadowViewMutation::RemoveDeleteTreeMutation(
8484

8585
ShadowViewMutation ShadowViewMutation::UpdateMutation(
8686
ShadowView oldChildShadowView,
87-
ShadowView newChildShadowView) {
87+
ShadowView newChildShadowView,
88+
ShadowView parentShadowView) {
8889
return {
8990
/* .type = */ Update,
90-
/* .parentShadowView = */ {},
91+
/* .parentShadowView = */ std::move(parentShadowView),
9192
/* .oldChildShadowView = */ std::move(oldChildShadowView),
9293
/* .newChildShadowView = */ std::move(newChildShadowView),
9394
/* .index = */ -1,

‎ReactCommon/react/renderer/mounting/ShadowViewMutation.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ struct ShadowViewMutation final {
7676
*/
7777
static ShadowViewMutation UpdateMutation(
7878
ShadowView oldChildShadowView,
79-
ShadowView newChildShadowView);
79+
ShadowView newChildShadowView,
80+
ShadowView parentShadowView);
8081

8182
#pragma mark - Type
8283

0 commit comments

Comments
 (0)