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 since the order of items changed.

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:

Description:
In manageChildren in NativeViewHierarchyManager.java there was a loop by indicesToRemove and a 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 loop by tagsToDelete later.

Now we propagate tagsToRemove from UIImplementation to NativeViewHierarchyOptimizer to NativeViewHierarchyManager and using it instead of indicesToRemove, because the tag is reliable way of identifying the node, and the index is not reliable anymore because of internal reordering of the views.

Also by having the old indicesToRemove loop we having the case that the wrong view is removed, but then, correct view is dropped in tagsToDelete loop. In result we have inconsistency when removing one view but dropping another. It's described in more details in mentioned above related issue comments.

[facebook-github-bot]

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

[asgvard]

Also related to #9704

[mkonicek]

Why have both indicesToRemove and tagsToRemove? Isn't tagsToRemove sufficient?

[asgvard]

Hi,
I'm pretty sure tagsToRemove is sufficient, just wasn't sure enough if indices should be completely removed in all this chain. Probably yes, just kept them for any possible backward compatibility.

[asgvard]

Should I remove indicesToRemove in the end, or we should leave it for any backward compat case?
Also is there any news/plans on reviewing this one? :) Would be awesome to have it merged

[goshacmd]

+1. I know it's a pretty darn amazing experience to have Android support for RN apps practically for free and I appreciate all the work that went into making it happen! And it would be very cool to have zIndex support fixed on Android.

[drewvolz]

+1. I also appreciate the work that has gone into this PR. I would be excited to see if someone could be found to review this.

[mkonicek]

Thanks for the detailed description and fix! I'm trying to find someone to review this :)

[lacker]

Can you add a test for this? It seems very likely to me that this bugfix would be subsequently broken in one way or another.

[astreet]

Agreed we should have a test.

However, the original patch implementing zIndex ordering is doing it in a pretty hacky way: we really shouldn't be changing the order of children in their parent. Someone correct me if there's actually a good reason for it, but this is the exact use case for ViewGroup#isChildrenDrawingOrderEnabled and ViewGroup#getChildDrawingOrder. I'd much rather we move to those instead of having to move to the assumption that JS children indices aren't respected.

[astreet]

(The alternative in my mind would be to enforce we never use the get/removeChildAt methods when dealing with the native view hierachy and instead use *get/removeChildByTag, but I think that'll be much harder to maintain and makes it harder to reason about things like adding children)

[hramos]

Any progress here?

[asgvard]

@hramos Not sure, since as I mentioned before, I'm not a pro in Java and in Java testing, and also @astreet suggested to maybe use children drawing order instead, but it's going too deep into Android part so I'm not sure I can handle it properly :) If someone from Android team could have a look onto this PR and add some unit tests for adding/removing children views to check if the correct view was removed after sorting by zIndex, would be great.

[lacker]

I have a certain amount of magical power to make people write tests for other peoples' pull requests but I seem to have used it all up at the moment. @asgvard I think you should either dig in and figure out how to do this the right way and add tests and so on, or let this feature request drift on the winds of fate and let this pull request be closed. Depends what you are in the mood for ;-)

[asgvard]

Does it makes sense to go with this PR assuming the rendering method on Android going to change to Nodes soon? Does it mean that the whole zIndex implementation would be different?

[nimatrueway]

23 days ago @janicduplessis fixed zIndex mechanism in commit 9a51fa8 which as gone into master branch already.

[asgvard]

I guess that should be fixed by this commit. So I'll close this PR.