From 1f88a7111d1a64910a4bfab322820406250081dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leo=20Nikkil=C3=A4?= Date: Thu, 2 Aug 2018 06:49:14 -0700 Subject: [PATCH] Fix view indices with Android LayoutAnimation (attempt 2) (#19775) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: /cc janicduplessis mdvacca This addresses the same issue as #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](https://github.com/facebook/react-native/pull/18830#issuecomment-382798628) by enabling clipping in [this test app](https://github.com/facebook/react-native/pull/18830#pullrequestreview-111758886) and adding a “clear views” button that resets the state to an empty items array with an animation. - #18830 [ANDROID] [BUGFIX] [LayoutAnimation] - Removal LayoutAnimations no longer remove adjacent views as well in certain cases. Pull Request resolved: https://github.com/facebook/react-native/pull/19775 Differential Revision: D9105838 Pulled By: hramos fbshipit-source-id: 5ccb0957d1f46c36add960c0e4ef2a545cb03cbe --- .../uimanager/NativeViewHierarchyManager.java | 13 ++-- .../react/uimanager/ViewGroupManager.java | 8 +++ .../react/views/view/ReactViewGroup.java | 69 +++++++++++++------ .../react/views/view/ReactViewManager.java | 20 ++++++ 4 files changed, 85 insertions(+), 25 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java index 3e7d56a5e..7d694e52c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -409,12 +409,13 @@ public class NativeViewHierarchyManager { 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; } } @@ -459,7 +460,9 @@ public class NativeViewHierarchyManager { mLayoutAnimator.deleteView(viewToDestroy, new LayoutAnimationListener() { @Override public void onAnimationEnd() { - 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); } }); diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java index 017fb5764..c4d5eed42 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java @@ -93,6 +93,14 @@ public abstract class ViewGroupManager } } + 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. diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java index ce2fa9528..6cd9462ce 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java @@ -39,6 +39,8 @@ import com.facebook.react.uimanager.RootViewUtil; import com.facebook.react.uimanager.ViewGroupDrawingOrderHelper; import com.facebook.react.uimanager.ViewProps; import com.facebook.yoga.YogaConstants; +import java.util.ArrayList; +import java.util.List; import javax.annotation.Nullable; /** @@ -106,6 +108,7 @@ public class ReactViewGroup extends ViewGroup implements private @Nullable ChildrenLayoutChangeListener mChildrenLayoutChangeListener; private @Nullable ReactViewBackgroundDrawable mReactBackgroundDrawable; private @Nullable OnInterceptTouchEventListener mOnInterceptTouchEventListener; + private @Nullable List mTransitioningViews; private boolean mNeedsOffscreenAlphaCompositing = false; private final ViewGroupDrawingOrderHelper mDrawingOrderHelper; private @Nullable Path mPath; @@ -334,16 +337,16 @@ public class ReactViewGroup extends ViewGroup implements 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) { View child = Assertions.assertNotNull(mAllChildren)[idx]; sHelperRect.set(child.getLeft(), child.getTop(), child.getRight(), child.getBottom()); boolean intersects = clippingRect @@ -360,10 +363,10 @@ public class ReactViewGroup extends ViewGroup implements 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 +402,25 @@ public class ReactViewGroup extends ViewGroup implements 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 protected void onSizeChanged(int w, int h, int oldw, int oldh) { super.onSizeChanged(w, h, oldw, oldh); @@ -509,13 +518,13 @@ public class ReactViewGroup extends ViewGroup implements 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); } @@ -525,14 +534,14 @@ public class ReactViewGroup extends ViewGroup implements 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); } @@ -547,6 +556,26 @@ public class ReactViewGroup extends ViewGroup implements 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); diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java index b4eb58704..31ff9d075 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java @@ -297,4 +297,24 @@ public class ReactViewManager extends ViewGroupManager { parent.removeAllViews(); } } + + @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); + } + } }