Let onVisibilityChanged() be called on children (mainly fixes Image issues)

[danielgindi]

Does any other open PR do the same thing?

Nope.

What issue is this PR fixing?

This is related to the following issues:
#1870 #2105 #2048

The REAL problem with displaying images inside the markers in Android, is not a bug in Fresco as claimed in some issues. It's that Fresco depends on the onVisibilityChanged(...) event to be called. And that's a protected method. And is propagated by dispatchVisibilityChanged(...) to the whole subview hierarchy. dispatchVisibilityChanged is protected too. And it depends on mAttachInfo etc.
So fiddling with Reflection is not an option as it counts on too many private stuff to not change in Android's core.

The solution I propose here is having a semi-invisible ViewGroup and add the markers' views to it, so it is properly "visible".
I'm calling it semi-invisible because setting visibility to INVISIBLE/GONE will again prevent the subviews from getting visibility events. What I did is set it to zero size, offscreen, clipping, and 0 opacity. So basically doing everything to avoid it trying to draw, but letting the events pass through.

For me, this fixes the issue - together with #2477

How did you test this PR?

Real life project with animatable Markers.

[rborn]

LGTM @alvelig 🐽

[alvelig]

Nice catch. I'll need to analyze that thoroughly, but overall LGTM.

[rborn]

@danielgindi @alvelig can you investigate #2478 (comment) please?

[danielgindi]

Yes, it’s an addition for RN 0.57, I just noticed that it’s a new feature. The other clipping code should do it’s best to avoid unnecessary drawing.

Is there a way to enclose that specific line for > 0.57 only?

[rborn]

@danielgindi this? https://github.com/facebook/react-native/blob/5c2720b089dd0fb277771b0ac8b0e7788c493e5d/ReactAndroid/src/main/java/com/facebook/react/modules/systeminfo/AndroidInfoModule.java#L74

[danielgindi]

@rborn I mean putting attacherGroup.setOverflow(ViewProps.HIDDEN); in a block like if (ReactNativeVersion.VERSION >= 0.57) { that Android Studio will respect. It works with SDK versions obviously because it's something supported out of the box for Android SDKs, but I'm not sure how the line you referred to will assist in this

In the worst case we could just omit that line. I took it as a precaution in case RN takes over the clipping stuff, so worst case it will be slower when rendering to the bitmap.

[rborn]

my java-fu sucks, @alvelig might be able to help maybe

[danielgindi]

So we should wait for his feedback before doing a release...

In anyway - the current situation is that 0.57 is the first to come with overflow: visible support in Android. It's implementation consists of setClipChildren(false); and a custom canvas.clipRect(new RectF(... based on the background of a view.
overflow: visible is on by default.
setClipChildren should be okay for now as the default is set in the constructor, but clipRect isn't.
The bigger issue might be in future versions - where I'm assuming they will change that implementation significantly, as current implementation does not support touches outside the view when overflow: visible.
My sources: facebook/react-native@b81c8b5

[danielgindi]

Okay so I just made the same mistake as the guys over there:
facebook/react-native@f090840

Android's clipping affects children's overflow, not the parent's.
So this makes the setClipBounds call unnecessary, and setOverflow even more necessary performance wise - especially when they will have a more complete implementation of it.

[alvelig]

@danielgindi just make it a string until we move definitely to 0.57

    attacherGroup.setOverflow("hidden");

See this change: facebook/react-native@cfce6ee#diff-b26c3186930f561d80acdc8f0dde5dec

[danielgindi]

Oh yeah. I misread their errors here, thought that setOverflow is the one that's missing.
Could you do it as an additional commit or should I PR?

[alvelig]

Fixed in 710dca4