Add zIndex support

[tuckerconnelly]

Summary:

Adds zIndex support :)

Test Plan

Tested the following components by adding two of each, overlapping them, and setting a high zIndex on the first of the two:

ActivityIndicator
Image
MapView
Picker
ScrollView
Slider
Switch
Text
TextInput
View
WebView

Tested on Android 4.1 and iOS 8.4. Also tested updating zIndexes on Views in my own app.

iOS seems to have a bug where zPosition is ignored when calling renderInContext (http://lists.apple.com/archives/cocoa-dev/2013/Jan/msg00084.html), so the test snapshots won't be z-indexed. If it's important enough I can look into another solution besides using zPosition.

---

iOS WebView was tough. Apparently RCTWebView is a subview? I set the zPosition on the superview in webViewDidFinishLoad, which seems a little hacky to me, so maybe someone can suggest a better solution :)

[ghost]

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

[ghost]

@tuckerconnelly updated the pull request.

[nicklockwood]

Maybe I'm missing something, but shouldn't a view with zIndex = -1 appear underneath a view view with zIndex = 0? In [email protected], it's on top.

[nicklockwood]

This looks promising, but I think the implementation on iOS needs to work by sorting the actual view order instead of adjusting the layer.zPosition.

(I also think you may have got the sign wrong on the zPosition anyway, as it appears that views with higher zIndex are being drawn behind views with lower zIndex, whereas according to the CSS spec "An element with greater stack order is always in front of an element with a lower stack order.")

[tuckerconnelly]

Nick, in the comment above:

iOS seems to have a bug where zPosition is ignored when calling renderInContext (http://lists.apple.com/archives/cocoa-dev/2013/Jan/msg00084.html), so the test snapshots won't be z-indexed.

Doesn't matter though 'cause zPosition won't work :P. Would bringSubviewToFront work?

Also, for Android, will bringToFront work with touch events?

[nicklockwood]

iOS seems to have a bug where zPosition is ignored when calling renderInContext

Ah, that makes sense.

Doesn't matter though 'cause zPosition won't work :P. Would bringSubviewToFront work?

Yes, but we need to be very careful when messing with the view order so that subsequent updates don't end up removing the wrong view.

I think we need to figure out the best place to do the view sorting. Options are:

1. In the view itself, inside the setReactSubviews method. That will mean storing the reactSubviews in an array and then sorting the views when they are added to the underlying UIView.subviews array.

2. In the UIManager, so that views are sorted prior to being inserted into the view/shadowView

3. On the JS side, so that they are already sorted by zIndex before they are sent over the bridge.

I'm not sure which of these is the best option, although 3) has the nice benefit that we could share the code between iOS and Android and avoid platform-specific hacks.

Also, for Android, will bringToFront work with touch events?

Sorry, I don't know anything about Android ¯_(ツ)_/¯

[nicklockwood]

Now I think about it, 3) probably isn't viable since it will affect the flexbox layout if we change the order prior to sending the views to native.

That means the order of shadowViews can't be changed based on zIndex, we can only change the order for UIViews.

[tuckerconnelly]

UIManager seems like the simplest to me--if it was done in the setReactSubviews method I think it'd have to be done in all the components' setReactSubviews methods. I'll give it a shot :)

[nicklockwood]

@tuckerconnelly I've got an idea how to approach this using setReactSubviews actually. I'll let you know if I can make it work.

[nicklockwood]

@tuckerconnelly I have a solution working for iOS. I'm just checking that it doesn't negatively affect performance.

[nicklockwood]

@tuckerconnelly OK, I've landed zIndex support for iOS: d64368b

You should be able to rebase your PR on top to add Android support.

[nicklockwood]

Cc: @dmmiller Dave, can you review the Android implementation here? I don't know the platform well enough to judge if this is an appropriate solution, or if we need to implement something like what I've done on iOS.

[tuckerconnelly]

DUDE YEAH! I can tomorrow

[syzer]

👍

[dmmiller]

Is this on every single view or are some exempted for some reason? In general, the core seems correct. I'm a little sad about doing this with an interface and 10 copy/pasted lines in every single view. This also won't scale if we have views that are purely native and not wrapped by react native. Is there a way to maybe do this with an annotation on the view (that addresses the first part, but not the second)?

[tuckerconnelly]

Yeah I didn't like the interface either. I can't really imagine a way to do it with annotations without stepping out of ViewManager-manages-View pattern, but I'm not a pro with Java :)

What if we created a generic View and have every subclass of View contain a reference to the android component. So something like:

public class ReactGenericView<T extends android.view.View> {
  private @Nullable float mZIndex;

  public void setZIndex(float zIndex) {
    mZIndex = zIndex;
  }

  public float getZIndex() {
    return mZIndex;
  }
  // Other future props
  // Could also put borderRadius, etc. in here
  public abstract T getComponent()
}

class ReactImageView extends ReactGenericView<GenericDraweeView> {
  // Stuff specific to Images
  @Override
  public GenericDraweeView getComponent() {
    // return lazily-initialized GenericDraweeView
  }
}

class ReactImageManager {
  @ReactProp(name = "tintColor", customType = "Color")
  public void setTintColor(ReactImageView view, @Nullable Integer tintColor) {
    if (tintColor == null) {
      view.getComponent().clearColorFilter();
    } else {
      view.getComponent().setColorFilter(tintColor, Mode.SRC_IN);
    }
  }
}

That's a pretty big change though.

Re: purely native views, I could put a default in to treat them as zIndex: 0, and if they wanted to set their own zIndex, they could implement the interface (or extend GenericView) and React would pick up on it.

Edit: Actually that wouldn't work with the interface because it wouldn't re-order the siblings when set. It would work with GenericView though, if the siblings were re-ordered on setZIndex.

[dmmiller]

@lexs do you know a good way to do this with annotations?

[lexs]

Why is this special?

[tuckerconnelly]

Whoops, that's old. I'll delete it.

[lexs]

We talked about the Android side internally and have a few wishes.

Instead of forcing each view to implement an interface can we have a look-aside map like: WeakHashMap<View, Integer>. Note that this will also require changing both impls to use int instead of float. The reason to use Integer here is that we can avoid boxing (and instead relying on the internal Java cache for small Integers), there is no such cache for floats. Using integers is also the css spec: https://developer.mozilla.org/en/docs/Web/CSS/z-index

We'd also like to move to a model where addChild() doesn't require sorting children if none of them have a z-index because we're afraid this might hurt perf. As I'd really like to see this land I don't think we need to do it in this PR but we (you or us internally) should look into this in the future depending on how inefficient the current implementation is. We have a few ideas on how to do this by for example keeping track of which ViewGroups contain z-indexed children and skipping sorting if we know they contain no z-children.

[nicklockwood]

We'd also like to move to a model where addChild() doesn't require sorting children if none of them have a z-index because we're afraid this might hurt perf

FWIW, the iOS implementation has this optimisation. I currently do it in a fairly naive way – instead of keeping track as the views are added, I just do a single pass through the views to see if any has a non-zero zIndex before I sort them – but that means the common case is O(n) instead of O(n log n) so it still seems worth doing.

[mkonicek]

@tuckerconnelly,@lexs What's the status? Does this need review or changes?

[dmmiller]

It needs to be rebased and updated.

[tuckerconnelly]

Alright it's up. I also added the optimization to check if there are any non-zero z-indexes before sorting 😃

[lexs]

@tuckerconnelly Thanks, I'll import this now and test it internally for perf regressions. I'll merge it on Monday if everything goes well :)

[lexs]

@facebook-github-bot import

[ghost]

Thanks for importing. If you are an FB employee go to Phabricator to review.

[lexs]

@facebook-github-bot shipit

[lexs]

@facebook-github-bot shipit

[ghost]

Thanks for importing. If you are an FB employee go to Phabricator to review.

[bestander]

I am a bit confused about the changed images for the iOS snapshot tests.
How are they related to the java change?

[bestander]

@lexs, the test was failing on the CI for the PR https://travis-ci.org/facebook/react-native/builds/137840201, be careful :)

[tuckerconnelly]

Picked the wrong yours/theirs in my revert/rebase. Had a hard time getting the Snapshot Test Cases to run again so just ignored it :P I can try to get them fixed up tomorrow morning

[bestander]

No worries, fixd it now bb0fda7