From abd3fd9dc648b838e537ced5279a4649bdd3a66c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leo=20Nikkil=C3=A4?= Date: Thu, 12 Apr 2018 20:18:58 +0300 Subject: [PATCH 1/2] Fix view indices with Android LayoutAnimation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes issue #11828 that causes layout animations for removed views to remove some adjacent views as well. This happens because the animated views are still present in the ViewGroup, which throws off subsequent operations that rely on view indices having updated. This issue was addressed in #11962, which was closed in favour of a more reliable solution that addresses the issue globally since it’s difficult to account for animated views everywhere. @janicduplessis recommended[0] handling the issue through ViewManager. Since API 11, Android provides `ViewGroup#startViewTransition(View)` that can be used to keep child views visible even if they have been removed from the group. ViewGroup keeps an array of these views, which is only used for drawing. Methods such as `ViewGroup#getChildCount()` and `ViewGroup#getChildAt(int)` will ignore them. I believe relying on these framework methods within ViewManager is the most reliable way to solve this issue because it also works if callers ignore ViewManager and reach into the native view indices and counts directly. [0]: https://github.com/facebook/react-native/pull/11962#pullrequestreview-21862640 --- .../react/uimanager/NativeViewHierarchyManager.java | 13 ++++++++----- .../facebook/react/uimanager/ViewGroupManager.java | 8 ++++++++ 2 files changed, 16 insertions(+), 5 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 3e7d56a5e29f30..7d694e52c83a6c 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 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; } } @@ -459,7 +460,9 @@ public synchronized void manageChildren( 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 017fb5764e7237..c4d5eed429dde7 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 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. From 5245abec9da63a2e732b701aca243944b3cfaf95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leo=20Nikkil=C3=A4?= Date: Sun, 17 Jun 2018 18:56:14 +0300 Subject: [PATCH 2/2] Handle transitioning views when clipping --- .../react/views/view/ReactViewGroup.java | 69 +++++++++++++------ .../react/views/view/ReactViewManager.java | 20 ++++++ 2 files changed, 69 insertions(+), 20 deletions(-) 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 ce2fa952859249..6cd9462ce35d11 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.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 void onLayoutChange( 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 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) { View child = Assertions.assertNotNull(mAllChildren)[idx]; sHelperRect.set(child.getLeft(), child.getTop(), child.getRight(), child.getBottom()); boolean intersects = clippingRect @@ -360,10 +363,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 +402,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 protected void onSizeChanged(int w, int h, int oldw, int oldh) { super.onSizeChanged(w, h, oldw, oldh); @@ -509,13 +518,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); } @@ -525,14 +534,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); } @@ -547,6 +556,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); 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 b4eb587049bd9e..31ff9d0753dcb0 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 void removeAllViews(ReactViewGroup parent) { 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); + } + } }