Fixed Android bug causing to remove wrong view after reordering by zIndex

[asgvard]

Commit 3d3b067 implemented zIndex support for Android. However after zIndex resorting of the views, the JS doesn't know about the new order. When dynamically removing the view from an array of views, JS sends the command to remove the view at certain index, which is not the same in Java anymore since the order of items changed.
This issue is also happens when adding view after reordering - it's added in the wrong position instead, but since it gets reordered again, it doesn't have visible effect, that's why it's only spotted when removing views.

Test Plan:
Related issue and the simple empty react-native 0.37 app where it was tested: #8968

Test app code:

import React, { Component } from 'react';
import {
  AppRegistry,
  StyleSheet,
  Text,
  View
} from 'react-native';

export default class zindex extends Component {
  constructor(props) {
    super(props);
    this.state = {
      showGreen: true
    };
  }
  render() {
    return (
      <View style={{flex: 1}}>
        <View style={[styles.item, {zIndex: 3, backgroundColor: 'red'}]}>
          <Text>zIndex: 3</Text>
        </View>
        {this.state.showGreen ?
          <View style={[styles.item, {zIndex: 2, backgroundColor: 'green', height: 90}]}>
            <Text>zIndex: 2</Text>
          </View> : null
        }
        <View style={[styles.item, {zIndex: 1, backgroundColor: 'blue', height: 120}]}>
          <Text>zIndex: 1</Text>
        </View>
        <View style={styles.button}>
          <Text onPress={() => this.setState({ showGreen: !this.state.showGreen })}>
            Toggle green
          </Text>
        </View>
      </View>
    );
  }
}

const styles = StyleSheet.create({
  item: {
    position: 'absolute',
    left: 0,
    right: 0,
    top: 0,
    height: 60
  },
  button: {
    position: 'absolute',
    left: 0,
    right: 0,
    backgroundColor: 'gray',
    top: 150,
    height: 30
  }
});

AppRegistry.registerComponent('zindex', () => zindex);

Starting screen:

Before fix, when trying to remove green, it removes blue instead:

After fix it removed green correctly:

When clicking toggle again, it adds green back, the blue is still ordered correctly by zIndex:

Initially the order of operations in manageChildren in NativeViewHierarchyManager.java was:

1. Loop by indicesToRemove, call viewManager.removeViewAt(viewToManage, indexToRemove). However if the view is animated and it's ID in the array of tagsToDelete, do nothing and delegate this job of removing item to the step 3).
2. Loop by viewsToAdd, add the views.
3. Loop by tagsToDelete, perform animations on the views that are using animations, then call viewManager.removeView(viewToManage, viewToDestroy) and dropView(viewToDestroy). But if the view is not animated, only dropView(viewToDestroy) is called because we assume that correct view was previously removed in step 1)

New order of operations:

1. Loop by tagsToDelete, perform animations if necessary, then call viewManager.removeView(viewToManage, viewToDestroy) and dropView(viewToDestroy). If no animations, just call viewManager.removeView(viewToManage, viewToDestroy) and dropView(viewToDestroy) right away.
2. Loop by viewsToAdd, add the views.

The intent is to perform clean-up by only looping by tagsToDelete first, instead of splitting the clean-up into separate steps of removing by index, then dropping by tagToDelete. And then when all the views are properly removed, we add the new views. Since indicesToRemove it's not reliable anymore and we don't use it for removeViewAt, we can get rid of the whole 1) step and replace it by 3) step.
Also by having the old operations order we having the case that the wrong view is removed in step 1), but then, correct view is dropped in step 3). In result we have inconsistency when removing one view but dropping another. It's described in more details in mentioned above related issue comments.

Because the diff doesn't look very clear because of replacing the blocks of code, here is what's actually changed:

1. Removed the loop by indicesToRemove as it's not reliable anymore and causing wrong view to be removed.
2. Moved loop by tagsToDelete before adding new views
3. Added a call for viewManager.removeView(viewToManage, viewToDestroy) in else statement.

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @janicduplessis and @dmmiller to be potential reviewers.

[asgvard]

Hi,

Ok, my first PR not the perfect one :) Will double-check all the CI tests locally and come with another solution.