Migrate needsCustomLayoutForChildren check to the new architecture (f…

…acebook#34254) Summary: Fixes facebook#34120 The new React Native architecture doesn't check `needsCustomLayoutForChildren` so it wrongly positions native views on Android. In facebook#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: facebook#34254 Test Plan: I checked the fix in the repro from facebook#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
Expensify · Aug 17, 2022 · 8182a72 · 8182a72
1 parent 840ff15
commit 8182a72
Show file tree

Hide file tree

Showing 10 changed files with 53 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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/.../src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java b/.../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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -76,7 +76,8 @@ struct ShadowViewMutation final {
    */
   static ShadowViewMutation UpdateMutation(
       ShadowView oldChildShadowView,
-      ShadowView newChildShadowView);
+      ShadowView newChildShadowView,
+      ShadowView parentShadowView);
 
 #pragma mark - Type