Fix view indices with Android LayoutAnimation

[lnikkila]

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
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.

Test Plan

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:

Current master (iOS vs Android)

With this patch (iOS vs Android, fixed)

Related PRs

Previously addressed in #11962, which wasn’t merged.

Tangentially related to my other LayoutAnimation PR #18651.

No documentation changes needed.

Release Notes

[ANDROID] [BUGFIX] [LayoutAnimation] - Removal LayoutAnimations no longer remove adjacent views as well in certain cases.

[janicduplessis]

Really nice, I didn't know about that method! Could you double check if it works well with this gist https://gist.github.com/janicduplessis/211d78c5408ab17dee7ad2e381648a48, it's what I was using when testing this issue. You can make the animation speed pretty slow and trigger multiple animations at the same time and make sure the proper number of views are there.

[janicduplessis]

It seems similar to the technique I wanted to use in #16091 and that iOS uses but using a platform implementation which is way better (since my implementation had a bug left that I didn't have time to investigate 😆).

[lnikkila]

Thanks for taking a look! I’ll try it out with that gist first thing tomorrow.

[facebook-github-bot]

@mdvacca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[lnikkila]

Tried it out, works fine!

I managed to get master to crash due to a wrong index by removing lots of squares rapidly and then adding some, but this branch was stable. Looks like #17118. I can see how this would happen, the views are added to a certain index and ViewGroup throws if the index is greater than the number of views in the group due to extra views having been removed.

There’s still a bug with view opacities not updating when interrupting an ongoing animation (compared to iOS where they get the final value) but it looks like a separate issue to me. You can see this with the tester by smashing the “remove view” button, they have random opacities on Android once the animation settles.

[mdvacca]

@lnikkila I'm reverting this PR, as we found a java.lang.IndexOutOfBoundsException when removing views:

Exception in native call java.lang.IndexOutOfBoundsException
at android.view.ViewGroup.removeViewsInternal(ViewGroup.java:4795)
at android.view.ViewGroup.removeViewsInLayout(ViewGroup.java:4652)
at com.facebook.react.views.view.ReactViewGroup.removeViewWithSubviewClippingEnabled(ReactViewGroup.java:530)
at com.facebook.react.views.view.ReactViewManager.removeViewAt(ReactViewManager.java:285)
at com.facebook.react.views.view.ReactViewManager.removeViewAt(ReactViewManager.java:36)
at com.facebook.react.uimanager.NativeViewHierarchyManager.manageChildren(NativeViewHierarchyManager.java:381)

[lnikkila]

Hmm could be due to removeClippedSubviews, I’ll take a closer look with that enabled and see if I can figure out why that’s happening.