Fix view indices with Android LayoutAnimation (attempt 2) (facebook#1…

…9775)

Summary:
/cc janicduplessis mdvacca

This addresses the same issue as facebook#18830 which was reverted since it didn’t handle `removeClippedSubviews` properly.

When subview clipping is on, ReactViewGroup keeps track of its own children directly and accounts for the offset introduced by clipped views when calling ViewGroup methods that modify children.

Instead of accounting for just clipped children (views with no parent), it should account for any children that aren’t in the ViewGroup which also includes children that are being transitioned. If you look at the ViewGroup source code, [it explicitly retains the view parent until the transition finishes](https://github.com/aosp-mirror/platform_frameworks_base/blob/oreo-release/core/java/android/view/ViewGroup.java#L5034-L5036) which caused the `getParent()` checks to pass, even though those views should be ignored. I added a new `isChildInViewGroup` method that handles both clipped and transitioning views to fix this.

I reproduced the [earlier crash](facebook#18830 (comment)) by enabling clipping in [this test app](facebook#18830 (review)) and adding a “clear views” button that resets the state to an empty items array with an animation.

- facebook#18830

[ANDROID] [BUGFIX] [LayoutAnimation] - Removal LayoutAnimations no longer remove adjacent views as well in certain cases.
Pull Request resolved: facebook#19775

Differential Revision: D9105838

Pulled By: hramos

fbshipit-source-id: 5ccb0957d1f46c36add960c0e4ef2a545cb03cbe

ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java

@@ -461,12 +461,13 @@ public synchronized void manageChildren(
         if (mLayoutAnimationEnabled
             && mLayoutAnimator.shouldAnimateLayout(viewToRemove)
             && arrayContains(tagsToDelete, viewToRemove.getId())) {
-          // The view will be removed and dropped by the 'delete' layout animation
-          // instead, so do nothing
-        } else {
-          viewManager.removeViewAt(viewToManage, indexToRemove);
+          // Display the view in the parent after removal for the duration of the layout animation,
+          // but pretend that it doesn't exist when calling other ViewGroup methods.
+          viewManager.startViewTransition(viewToManage, viewToRemove);
         }
 
+        viewManager.removeViewAt(viewToManage, indexToRemove);
+
         lastIndexToRemove = indexToRemove;
       }
     }
@@ -495,7 +496,9 @@ public void onAnimationEnd() {
                   // onAnimationEnd is called (indirectly) by Android View Animation.
                   UiThreadUtil.assertOnUiThread();
 
-                  viewManager.removeView(viewToManage, viewToDestroy);
+                  // Already removed from the ViewGroup, we can just end the transition here to
+                  // release the child.
+                  viewManager.endViewTransition(viewToManage, viewToDestroy);
                   dropView(viewToDestroy);
                   pendingDeletionTags.remove(viewToDestroy.getId());
                   if (pendingDeletionTags.isEmpty()) {

ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java

@@ -93,6 +93,14 @@ public void removeAllViews(T parent) {
     }
   }
 
+  public void startViewTransition(T parent, View view) {
+    parent.startViewTransition(view);
+  }
+
+  public void endViewTransition(T parent, View view) {
+    parent.endViewTransition(view);
+  }
+
   /**
    * Returns whether this View type needs to handle laying out its own children instead of deferring
    * to the standard css-layout algorithm. Returns true for the layout to *not* be automatically

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java

@@ -51,6 +51,8 @@
 import com.facebook.react.uimanager.common.UIManagerType;
 import com.facebook.react.uimanager.common.ViewUtil;
 import com.facebook.yoga.YogaConstants;
+import java.util.ArrayList;
+import java.util.List;
 
 /**
  * Backing for a React View. Has support for borders, but since borders aren't common, lazy
@@ -121,6 +123,7 @@ public void onLayoutChange(
   private @Nullable ChildrenLayoutChangeListener mChildrenLayoutChangeListener;
   private @Nullable ReactViewBackgroundDrawable mReactBackgroundDrawable;
   private @Nullable OnInterceptTouchEventListener mOnInterceptTouchEventListener;
+  private @Nullable List<View> mTransitioningViews;
   private boolean mNeedsOffscreenAlphaCompositing = false;
   private final ViewGroupDrawingOrderHelper mDrawingOrderHelper;
   private @Nullable Path mPath;
@@ -330,18 +333,17 @@ public void updateClippingRect() {
 
   private void updateClippingToRect(Rect clippingRect) {
     Assertions.assertNotNull(mAllChildren);
-    int clippedSoFar = 0;
+    int childIndexOffset = 0;
     for (int i = 0; i < mAllChildrenCount; i++) {
-      updateSubviewClipStatus(clippingRect, i, clippedSoFar);
-      if (mAllChildren[i].getParent() == null) {
-        clippedSoFar++;
+      updateSubviewClipStatus(clippingRect, i, childIndexOffset);
+      if (!isChildInViewGroup(mAllChildren[i])) {
+        childIndexOffset++;
       }
     }
   }
 
-  private void updateSubviewClipStatus(Rect clippingRect, int idx, int clippedSoFar) {
+  private void updateSubviewClipStatus(Rect clippingRect, int idx, int childIndexOffset) {
     UiThreadUtil.assertOnUiThread();
-
     View child = Assertions.assertNotNull(mAllChildren)[idx];
     sHelperRect.set(child.getLeft(), child.getTop(), child.getRight(), child.getBottom());
     boolean intersects =
@@ -359,10 +361,10 @@ private void updateSubviewClipStatus(Rect clippingRect, int idx, int clippedSoFa
     if (!intersects && child.getParent() != null && !isAnimating) {
       // We can try saving on invalidate call here as the view that we remove is out of visible area
       // therefore invalidation is not necessary.
-      super.removeViewsInLayout(idx - clippedSoFar, 1);
+      super.removeViewsInLayout(idx - childIndexOffset, 1);
       needUpdateClippingRecursive = true;
     } else if (intersects && child.getParent() == null) {
-      super.addViewInLayout(child, idx - clippedSoFar, sDefaultLayoutParam, true);
+      super.addViewInLayout(child, idx - childIndexOffset, sDefaultLayoutParam, true);
       invalidate();
       needUpdateClippingRecursive = true;
     } else if (intersects) {
@@ -399,19 +401,25 @@ private void updateSubviewClipStatus(View subview) {
     boolean oldIntersects = (subview.getParent() != null);
 
     if (intersects != oldIntersects) {
-      int clippedSoFar = 0;
+      int childIndexOffset = 0;
       for (int i = 0; i < mAllChildrenCount; i++) {
         if (mAllChildren[i] == subview) {
-          updateSubviewClipStatus(mClippingRect, i, clippedSoFar);
+          updateSubviewClipStatus(mClippingRect, i, childIndexOffset);
           break;
         }
-        if (mAllChildren[i].getParent() == null) {
-          clippedSoFar++;
+        if (!isChildInViewGroup(mAllChildren[i])) {
+          childIndexOffset++;
         }
       }
     }
   }
 
+  private boolean isChildInViewGroup(View view) {
+    // A child is in the group if it's not clipped and it's not transitioning.
+    return view.getParent() != null
+      && (mTransitioningViews == null || !mTransitioningViews.contains(view));
+  }
+
   @Override
   public boolean getChildVisibleRect(View child, Rect r, android.graphics.Point offset) {
     return ReactFeatureFlags.clipChildRectsIfOverflowIsHidden
@@ -549,13 +557,13 @@ protected void dispatchSetPressed(boolean pressed) {
     addInArray(child, index);
     // we add view as "clipped" and then run {@link #updateSubviewClipStatus} to conditionally
     // attach it
-    int clippedSoFar = 0;
+    int childIndexOffset = 0;
     for (int i = 0; i < index; i++) {
-      if (mAllChildren[i].getParent() == null) {
-        clippedSoFar++;
+      if (!isChildInViewGroup(mAllChildren[i])) {
+        childIndexOffset++;
       }
     }
-    updateSubviewClipStatus(mClippingRect, index, clippedSoFar);
+    updateSubviewClipStatus(mClippingRect, index, childIndexOffset);
     child.addOnLayoutChangeListener(mChildrenLayoutChangeListener);
   }
 
@@ -567,14 +575,14 @@ protected void dispatchSetPressed(boolean pressed) {
     Assertions.assertNotNull(mAllChildren);
     view.removeOnLayoutChangeListener(mChildrenLayoutChangeListener);
     int index = indexOfChildInAllChildren(view);
-    if (mAllChildren[index].getParent() != null) {
-      int clippedSoFar = 0;
+    if (isChildInViewGroup(mAllChildren[index])) {
+      int childIndexOffset = 0;
       for (int i = 0; i < index; i++) {
-        if (mAllChildren[i].getParent() == null) {
-          clippedSoFar++;
+        if (!isChildInViewGroup(mAllChildren[i])) {
+          childIndexOffset++;
         }
       }
-      super.removeViewsInLayout(index - clippedSoFar, 1);
+      super.removeViewsInLayout(index - childIndexOffset, 1);
     }
     removeFromArray(index);
   }
@@ -589,6 +597,26 @@ protected void dispatchSetPressed(boolean pressed) {
     mAllChildrenCount = 0;
   }
 
+  /*package*/ void startViewTransitionWithSubviewClippingEnabled(View view) {
+    // We're mirroring ViewGroup's mTransitioningViews since when a transitioning child is removed,
+    // its parent is not set to null unlike a regular child. Normally this wouldn't be an issue as
+    // ViewGroup pretends the transitioning child doesn't exist when calling any methods that expose
+    // child views, but we keep track of our children directly when subview clipping is enabled and
+    // need to be aware of these.
+    if (mTransitioningViews == null) {
+      mTransitioningViews = new ArrayList<>();
+    }
+    mTransitioningViews.add(view);
+    startViewTransition(view);
+  }
+
+  /*package*/ void endViewTransitionWithSubviewClippingEnabled(View view) {
+    if (mTransitioningViews != null) {
+      mTransitioningViews.remove(view);
+    }
+    endViewTransition(view);
+  }
+
   private int indexOfChildInAllChildren(View child) {
     final int count = mAllChildrenCount;
     final View[] children = Assertions.assertNotNull(mAllChildren);

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java

@@ -340,4 +340,24 @@ private void handleHotspotUpdate(ReactViewGroup root, @Nullable ReadableArray ar
     float y = PixelUtil.toPixelFromDIP(args.getDouble(1));
     root.drawableHotspotChanged(x, y);
   }
+
+  @Override
+  public void startViewTransition(ReactViewGroup parent, View view) {
+    boolean removeClippedSubviews = parent.getRemoveClippedSubviews();
+    if (removeClippedSubviews) {
+      parent.startViewTransitionWithSubviewClippingEnabled(view);
+    } else {
+      parent.startViewTransition(view);
+    }
+  }
+
+  @Override
+  public void endViewTransition(ReactViewGroup parent, View view) {
+    boolean removeClippedSubviews = parent.getRemoveClippedSubviews();
+    if (removeClippedSubviews) {
+      parent.endViewTransitionWithSubviewClippingEnabled(view);
+    } else {
+      parent.endViewTransition(view);
+    }
+  }
 }