Skip to content

Commit 572040a

Browse files
lnikkilamacdoum1
authored andcommitted
Fix view indices with Android LayoutAnimation
Summary: Fixes issue facebook#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 facebook#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]: facebook#11962 (review) I wrote a minimal test app that you can find here: <https://gist.github.com/lnikkila/87f3825442a5773f17ead433a810d53f> The expected result is that the red and green squares disappear, a blue one appears, and the black one stays in place. iOS has this behaviour, but Android removes the black square as well. We can see the bug with some breakpoint logging. Without LayoutAnimation: ``` NativeViewHierarchyOptimizer: Removing node from parent with tag 2 at index 0 NativeViewHierarchyOptimizer: Removing node from parent with tag 4 at index 1 NativeViewHierarchyManager: Removing indices [0] with tags [2] RootViewManager: Removing child view at index 0 with tag 2 NativeViewHierarchyManager: Removing indices [1] with tags [4] RootViewManager: Removing child view at index 1 with tag 4 ``` With LayoutAnimation tag 3 gets removed when it shouldn’t be: ``` NativeViewHierarchyOptimizer: Removing node from parent with tag 2 at index 0 NativeViewHierarchyOptimizer: Removing node from parent with tag 4 at index 1 NativeViewHierarchyManager: Removing indices [0] with tags [2] NativeViewHierarchyManager: Removing indices [1] with tags [4] -> RootViewManager: Removing child view at index 1 with tag 3 RootViewManager: Removing child view at index 2 with tag 4 (Animation listener kicks in here) RootViewManager: Removing child view at index 1 with tag 2 ``` Here are some GIFs to compare, click to expand: <details> <summary><b>Current master (iOS vs Android)</b></summary> <p></p> <img src="https://user-images.githubusercontent.com/1291143/38695083-fbc29cd4-3e93-11e8-9150-9b8ea75b87aa.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38695108-06eb73a6-3e94-11e8-867a-b95d7f926ccd.gif" height="400" /> </details><p></p> <details> <summary><b>With this patch (iOS vs Android, fixed)</b></summary> <p></p> <img src="https://user-images.githubusercontent.com/1291143/38695083-fbc29cd4-3e93-11e8-9150-9b8ea75b87aa.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38695137-1090f782-3e94-11e8-94c8-ce33a5d7ebdb.gif" height="400" /> </details><p></p> Previously addressed in facebook#11962, which wasn’t merged. Tangentially related to my other LayoutAnimation PR facebook#18651. No documentation changes needed. [ANDROID] [BUGFIX] [LayoutAnimation] - Removal LayoutAnimations no longer remove adjacent views as well in certain cases. Closes facebook#18830 Reviewed By: achen1 Differential Revision: D7612904 Pulled By: mdvacca fbshipit-source-id: a04cf47ab80e0e813fa043125b1f907e212b1ad4
1 parent a2b8837 commit 572040a

File tree

2 files changed

+16
-5
lines changed

2 files changed

+16
-5
lines changed

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

+8-5
Original file line numberDiff line numberDiff line change
@@ -373,12 +373,13 @@ public synchronized void manageChildren(
373373
if (mLayoutAnimationEnabled &&
374374
mLayoutAnimator.shouldAnimateLayout(viewToRemove) &&
375375
arrayContains(tagsToDelete, viewToRemove.getId())) {
376-
// The view will be removed and dropped by the 'delete' layout animation
377-
// instead, so do nothing
378-
} else {
379-
viewManager.removeViewAt(viewToManage, indexToRemove);
376+
// Display the view in the parent after removal for the duration of the layout animation,
377+
// but pretend that it doesn't exist when calling other ViewGroup methods.
378+
viewManager.startViewTransition(viewToManage, viewToRemove);
380379
}
381380

381+
viewManager.removeViewAt(viewToManage, indexToRemove);
382+
382383
lastIndexToRemove = indexToRemove;
383384
}
384385
}
@@ -423,7 +424,9 @@ public synchronized void manageChildren(
423424
mLayoutAnimator.deleteView(viewToDestroy, new LayoutAnimationListener() {
424425
@Override
425426
public void onAnimationEnd() {
426-
viewManager.removeView(viewToManage, viewToDestroy);
427+
// Already removed from the ViewGroup, we can just end the transition here to
428+
// release the child.
429+
viewManager.endViewTransition(viewToManage, viewToDestroy);
427430
dropView(viewToDestroy);
428431
}
429432
});

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

+8
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,14 @@ public void removeAllViews(T parent) {
9393
}
9494
}
9595

96+
public void startViewTransition(T parent, View view) {
97+
parent.startViewTransition(view);
98+
}
99+
100+
public void endViewTransition(T parent, View view) {
101+
parent.endViewTransition(view);
102+
}
103+
96104
/**
97105
* Returns whether this View type needs to handle laying out its own children instead of
98106
* deferring to the standard css-layout algorithm.

0 commit comments

Comments
 (0)