From 8182a7291a6583c36720dd83a745782df5a5fed5 Mon Sep 17 00:00:00 2001 From: Graham Mendick Date: Thu, 28 Jul 2022 09:57:36 -0700 Subject: [PATCH] Migrate needsCustomLayoutForChildren check to the new architecture (#34254) Summary: Fixes https://github.com/facebook/react-native/issues/34120 The new React Native architecture doesn't check `needsCustomLayoutForChildren` so it wrongly positions native views on Android. In https://github.com/facebook/react-native/issues/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: https://github.com/facebook/react-native/pull/34254 Test Plan: I checked the fix in the repro from https://github.com/facebook/react-native/issues/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 --- .../react/fabric/jni/FabricMountItem.cpp | 6 +++-- .../react/fabric/jni/FabricMountItem.h | 4 +++- .../fabric/jni/FabricMountingManager.cpp | 24 ++++++++++--------- .../mounting/SurfaceMountingManager.java | 14 +++++++---- .../mountitems/IntBufferBatchMountItem.java | 4 +++- .../animations/LayoutAnimationDriver.cpp | 2 +- .../LayoutAnimationKeyFrameManager.cpp | 10 ++++---- .../renderer/mounting/Differentiator.cpp | 12 ++++++---- .../renderer/mounting/ShadowViewMutation.cpp | 5 ++-- .../renderer/mounting/ShadowViewMutation.h | 3 ++- 10 files changed, 53 insertions(+), 31 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.cpp index 05979307245a06..d204f306a0b690 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.cpp @@ -44,8 +44,10 @@ CppMountItem CppMountItem::UpdatePropsMountItem( CppMountItem CppMountItem::UpdateStateMountItem(ShadowView const &shadowView) { return {CppMountItem::Type::UpdateState, {}, {}, shadowView, -1}; } -CppMountItem CppMountItem::UpdateLayoutMountItem(ShadowView const &shadowView) { - return {CppMountItem::Type::UpdateLayout, {}, {}, shadowView, -1}; +CppMountItem CppMountItem::UpdateLayoutMountItem( + ShadowView const &shadowView, + ShadowView const &parentView) { + return {CppMountItem::Type::UpdateLayout, parentView, {}, shadowView, -1}; } CppMountItem CppMountItem::UpdateEventEmitterMountItem( ShadowView const &shadowView) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.h index 1de6263467e70a..f9cb4c623e2d52 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.h @@ -46,7 +46,9 @@ struct CppMountItem final { static CppMountItem UpdateStateMountItem(ShadowView const &shadowView); - static CppMountItem UpdateLayoutMountItem(ShadowView const &shadowView); + static CppMountItem UpdateLayoutMountItem( + ShadowView const &shadowView, + ShadowView const &parentView); static CppMountItem UpdateEventEmitterMountItem(ShadowView const &shadowView); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp index c67032f9b9a7de..69609f862a578f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp @@ -79,7 +79,7 @@ static inline int getIntBufferSizeForType(CppMountItem::Type mountItemType) { case CppMountItem::Type::UpdatePadding: return 5; // tag, top, left, bottom, right case CppMountItem::Type::UpdateLayout: - return 6; // tag, x, y, w, h, DisplayType + return 7; // tag, parentTag, x, y, w, h, DisplayType case CppMountItem::Type::UpdateOverflowInset: return 5; // tag, left, top, right, bottom case CppMountItem::Undefined: @@ -381,7 +381,7 @@ void FabricMountingManager::executeMount( newChildShadowView.layoutMetrics) { cppUpdateLayoutMountItems.push_back( CppMountItem::UpdateLayoutMountItem( - mutation.newChildShadowView)); + mutation.newChildShadowView, parentShadowView)); } // OverflowInset: This is the values indicating boundaries including @@ -441,7 +441,8 @@ void FabricMountingManager::executeMount( // Layout cppUpdateLayoutMountItems.push_back( - CppMountItem::UpdateLayoutMountItem(newChildShadowView)); + CppMountItem::UpdateLayoutMountItem( + newChildShadowView, parentShadowView)); // OverflowInset: This is the values indicating boundaries including // children of the current view. The layout of current view may not @@ -548,7 +549,7 @@ void FabricMountingManager::executeMount( int intBufferPosition = 0; int objBufferPosition = 0; int prevMountItemType = -1; - jint temp[6]; + jint temp[7]; for (int i = 0; i < cppCommonMountItems.size(); i++) { const auto &mountItem = cppCommonMountItems[i]; const auto &mountItemType = mountItem.type; @@ -723,13 +724,14 @@ void FabricMountingManager::executeMount( toInt(mountItem.newChildShadowView.layoutMetrics.displayType); temp[0] = mountItem.newChildShadowView.tag; - temp[1] = x; - temp[2] = y; - temp[3] = w; - temp[4] = h; - temp[5] = displayType; - env->SetIntArrayRegion(intBufferArray, intBufferPosition, 6, temp); - intBufferPosition += 6; + temp[1] = mountItem.parentShadowView.tag; + temp[2] = x; + temp[3] = y; + temp[4] = w; + temp[5] = h; + temp[6] = displayType; + env->SetIntArrayRegion(intBufferArray, intBufferPosition, 7, temp); + intBufferPosition += 7; } } if (!cppUpdateOverflowInsetMountItems.empty()) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java index 3d8f8efdd478d8..90096f4ada13b2 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java @@ -967,7 +967,8 @@ public void sendAccessibilityEvent(int reactTag, int eventType) { } @UiThread - public void updateLayout(int reactTag, int x, int y, int width, int height, int displayType) { + public void updateLayout( + int reactTag, int parentTag, int x, int y, int width, int height, int displayType) { if (isStopped()) { return; } @@ -992,9 +993,14 @@ public void updateLayout(int reactTag, int x, int y, int width, int height, int parent.requestLayout(); } - // TODO: T31905686 Check if the parent of the view has to layout the view, or the child has - // to lay itself out. see NativeViewHierarchyManager.updateLayout - viewToUpdate.layout(x, y, x + width, y + height); + ViewState parentViewState = getViewState(parentTag); + ViewGroupManager parentViewManager = null; + if (parentViewState.mViewManager != null) { + parentViewManager = parentViewState.mViewManager.getViewGroupManager(); + } + if (parentViewManager == null || !parentViewManager.needsCustomLayoutForChildren()) { + viewToUpdate.layout(x, y, x + width, y + height); + } // displayType: 0 represents display: 'none' int visibility = displayType == 0 ? View.INVISIBLE : View.VISIBLE; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java index 693f889651eb24..5f1a4366b3e8c0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java @@ -149,13 +149,15 @@ public void execute(@NonNull MountingManager mountingManager) { surfaceMountingManager.updateState(mIntBuffer[i++], castToState(mObjBuffer[j++])); } else if (type == INSTRUCTION_UPDATE_LAYOUT) { int reactTag = mIntBuffer[i++]; + int parentTag = mIntBuffer[i++]; int x = mIntBuffer[i++]; int y = mIntBuffer[i++]; int width = mIntBuffer[i++]; int height = mIntBuffer[i++]; int displayType = mIntBuffer[i++]; - surfaceMountingManager.updateLayout(reactTag, x, y, width, height, displayType); + surfaceMountingManager.updateLayout( + reactTag, parentTag, x, y, width, height, displayType); } else if (type == INSTRUCTION_UPDATE_PADDING) { surfaceMountingManager.updatePadding( diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp b/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp index 8ff9569a845e9d..a24d7c4a2cf0cc 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp +++ b/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp @@ -57,7 +57,7 @@ void LayoutAnimationDriver::animationMutationsForFrame( // Create the mutation instruction mutationsList.emplace_back(ShadowViewMutation::UpdateMutation( - keyframe.viewPrev, mutatedShadowView)); + keyframe.viewPrev, mutatedShadowView, keyframe.parentView)); PrintMutationInstruction("Animation Progress:", updateMutation); diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp index 332790447e8c28..2160e541efcb00 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp +++ b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp @@ -1241,7 +1241,9 @@ void LayoutAnimationKeyFrameManager::queueFinalMutationsForCompletedKeyFrame( break; case ShadowViewMutation::Type::Update: mutationsList.push_back(ShadowViewMutation::UpdateMutation( - prev, finalMutation.newChildShadowView)); + prev, + finalMutation.newChildShadowView, + finalMutation.parentShadowView)); break; } if (finalMutation.newChildShadowView.tag > 0) { @@ -1266,7 +1268,7 @@ void LayoutAnimationKeyFrameManager::queueFinalMutationsForCompletedKeyFrame( auto mutatedShadowView = createInterpolatedShadowView(1, keyframe.viewStart, keyframe.viewEnd); auto generatedPenultimateMutation = ShadowViewMutation::UpdateMutation( - keyframe.viewPrev, mutatedShadowView); + keyframe.viewPrev, mutatedShadowView, keyframe.parentView); react_native_assert( generatedPenultimateMutation.oldChildShadowView.tag > 0); react_native_assert( @@ -1277,7 +1279,7 @@ void LayoutAnimationKeyFrameManager::queueFinalMutationsForCompletedKeyFrame( mutationsList.push_back(generatedPenultimateMutation); auto generatedMutation = ShadowViewMutation::UpdateMutation( - mutatedShadowView, keyframe.viewEnd); + mutatedShadowView, keyframe.viewEnd, keyframe.parentView); react_native_assert(generatedMutation.oldChildShadowView.tag > 0); react_native_assert(generatedMutation.newChildShadowView.tag > 0); PrintMutationInstruction( @@ -1286,7 +1288,7 @@ void LayoutAnimationKeyFrameManager::queueFinalMutationsForCompletedKeyFrame( mutationsList.push_back(generatedMutation); } else { auto mutation = ShadowViewMutation::UpdateMutation( - keyframe.viewPrev, keyframe.viewEnd); + keyframe.viewPrev, keyframe.viewEnd, keyframe.parentView); PrintMutationInstruction( logPrefix + "Animation Complete: Queuing up Final Synthetic Mutation:", diff --git a/ReactCommon/react/renderer/mounting/Differentiator.cpp b/ReactCommon/react/renderer/mounting/Differentiator.cpp index 41d4de1e8e5368..372aa7cb6f0ca3 100644 --- a/ReactCommon/react/renderer/mounting/Differentiator.cpp +++ b/ReactCommon/react/renderer/mounting/Differentiator.cpp @@ -574,7 +574,7 @@ static void updateMatchedPair( if (oldPair.shadowView != newPair.shadowView) { mutationContainer.updateMutations.push_back( ShadowViewMutation::UpdateMutation( - oldPair.shadowView, newPair.shadowView)); + oldPair.shadowView, newPair.shadowView, parentShadowView)); } } } @@ -835,7 +835,9 @@ static void calculateShadowViewMutationsFlattener( newTreeNodePair.isConcreteView && oldTreeNodePair.isConcreteView) { mutationContainer.updateMutations.push_back( ShadowViewMutation::UpdateMutation( - oldTreeNodePair.shadowView, newTreeNodePair.shadowView)); + oldTreeNodePair.shadowView, + newTreeNodePair.shadowView, + node.shadowView)); } // Update children if appropriate. @@ -1161,7 +1163,9 @@ static void calculateShadowViewMutationsV2( oldChildPair.shadowView != newChildPair.shadowView) { mutationContainer.updateMutations.push_back( ShadowViewMutation::UpdateMutation( - oldChildPair.shadowView, newChildPair.shadowView)); + oldChildPair.shadowView, + newChildPair.shadowView, + parentShadowView)); } // Recursively update tree if ShadowNode pointers are not equal @@ -1699,7 +1703,7 @@ ShadowViewMutation::List calculateShadowViewMutations( if (oldRootShadowView != newRootShadowView) { mutations.push_back(ShadowViewMutation::UpdateMutation( - oldRootShadowView, newRootShadowView)); + oldRootShadowView, newRootShadowView, {})); } calculateShadowViewMutationsV2( diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp index 2873f06e4da2f2..53006ff347b015 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp @@ -84,10 +84,11 @@ ShadowViewMutation ShadowViewMutation::RemoveDeleteTreeMutation( ShadowViewMutation ShadowViewMutation::UpdateMutation( ShadowView oldChildShadowView, - ShadowView newChildShadowView) { + ShadowView newChildShadowView, + ShadowView parentShadowView) { return { /* .type = */ Update, - /* .parentShadowView = */ {}, + /* .parentShadowView = */ std::move(parentShadowView), /* .oldChildShadowView = */ std::move(oldChildShadowView), /* .newChildShadowView = */ std::move(newChildShadowView), /* .index = */ -1, diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h index 7beba76b25eb2a..4b571bcc7753ea 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h @@ -76,7 +76,8 @@ struct ShadowViewMutation final { */ static ShadowViewMutation UpdateMutation( ShadowView oldChildShadowView, - ShadowView newChildShadowView); + ShadowView newChildShadowView, + ShadowView parentShadowView); #pragma mark - Type