View.getGlobalVisibleRect() is broken in some use cases

[d4vidi]

🐛 Bug Report

With respect to the list of these commits - all of which involve child-views clipping on Android (AKA the overflow functionality): b81c8b, 6110a4c, bbdc12e, 9c71952;

While in essence the approach makes sense for introducing overflow toggling support, having view-group components' clip-children flag hard-codedly set to false creates undesired side-effects. One of which is the breaking of the logic of the native View.getGlobalVisibleRect() method. In some cases -- as illustrated by the screenshot from a demo app I've worked out for this, the native method will return true for views that were effectively properly clipped by ReactViewGroup, and are in fact not visible on the screen.

In that specific use case, while the view-group holding the upper part of the screen (gray background) is limited in height, the scroll view stretches all the way down to the bottom of the screen. This way, items 14..17 are drawn under the lower part of the screen (blue), and - as far as Android is concerned, are NOT clipped by the parent view-group -- as it's clip-children flag is off.

Note: the bug persists even if ScrollView is removed from the hierarchy.

Among other potential functionalities, this stability of the View.getGlobalVisibleRect() method is paramount for ui-testing projects to work -- such as https://github.com/wix/Detox (the one I'm working on right now) and that is already integrated into React Native itself (for iOS).

To Reproduce

The demo app is available on github

Expected Behavior

View.getGlobalVisibleRect() should return false for views that were clipped off by one of their view-group parents.

Code Example

From the demo app:

• A screen layout where this bug occurs
• The native logic that runs 'under the hood' in order to show the bug is there.

Environment

info 
  React Native Environment Info:
    System:
      OS: macOS 10.14.3
      CPU: (8) x64 Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz
      Memory: 409.30 MB / 16.00 GB
      Shell: 3.2.57 - /bin/bash
    Binaries:
      Node: 9.11.2 - ~/.nvm/versions/node/v9.11.2/bin/node
      Yarn: 1.3.2 - /usr/local/bin/yarn
      npm: 6.8.0 - ~/.nvm/versions/node/v9.11.2/bin/npm
      Watchman: 4.9.0 - /usr/local/bin/watchman
    SDKs:
      iOS SDK:
        Platforms: iOS 12.1, macOS 10.14, tvOS 12.1, watchOS 5.1
      Android SDK:
        API Levels: 19, 21, 22, 23, 24, 25, 26, 27, 28
        Build Tools: 19.1.0, 20.0.0, 21.1.2, 22.0.1, 23.0.1, 23.0.2, 23.0.3, 24.0.1, 25.0.0, 25.0.1, 25.0.2, 25.0.3, 26.0.0, 26.0.1, 26.0.2, 26.0.3, 27.0.1, 27.0.3, 28.0.0, 28.0.2, 28.0.3
        System Images: android-16 | Google APIs Intel x86 Atom, android-19 | Google APIs Intel x86 Atom, android-22 | Google APIs Intel x86 Atom, android-22 | Google APIs Intel x86 Atom_64, android-23 | Google APIs Intel x86 Atom_64, android-25 | Google APIs Intel x86 Atom_64, android-26 | Google APIs Intel x86 Atom, android-26 | Google Play Intel x86 Atom, android-28 | Google Play Intel x86 Atom
    IDEs:
      Android Studio: 3.3 AI-182.5107.16.33.5199772
      Xcode: 10.1/10B61 - /usr/bin/xcodebuild
    npmPackages:
      react: 16.8.3 => 16.8.3 
      react-native: 0.59.0 => 0.59.0 
    npmGlobalPackages:
      react-native-cli: 2.0.1

[yungsters]

There is a blog post titled, Android View Measurement. It provides a very good explanation of getGlobalVisibleRect() with its caveats:

getGlobalVisibleRect() is quite misleading, it actually pertains to clipping, not visibility.

If we take a look at the implementation of getGlobalVisibleRect(), we see:

• getGlobalVisibleRect() delegates to mParent.getChildVisibleRect().
• mParent.getChildVisibleRect() simply intersects its own dimensions with the child's.

I do not have particular opinions about this Android API. But I can understand why, from a performance perspective, the platform designers would not want to provide a simple method call that does an O(depth) traversal.

If you want a method that recursively traverses parent view groups to apply all clippings, it would appear that you can do this by repeatedly calling getChildVisibleRect() on getParent(). Would that work?

[d4vidi]

@yungsters I don't think that would be helpful tbh, as, given the existing implementation, getGlobalVisibleRect() doesn't work even if the direct parent itself is the one doing the clipping (O(1) check, no tree traversal).

I suppose the real question here is whether hard-setting native views' clipping to false while implementing custom clipping in a custom draw implementation is a fair strategy... In essence, it puts the Android-ish UI realm under the wrong notion of that (almost) nothing clips anything else, such that no matter what API I use, it would't be accurate.
As I understand it, the only way to introduce a query of that sort that would actually work is through react native's implementation itself, which has become the one real source of truth in this case...
wdyt?

[d4vidi]

@yungsters @hramos any leads regarding improving / fixing this?

[fungilation]

Ya, any heads up appreciated. As Detox is broken and blocked on RN 0.59+ on Android due to this.

[yungsters]

I do not have any good ideas for how to resolve this. I understand the dilemma that this places on Detox, but I do not know how else we could possibly have implemented support for overflow: visible without defaulting setClipChildren(false). Due to the viral nature of overflow, a default of overflow: hidden (and setClipChildren(true)) prevents any meaningful support for overflow: visible which, if you might recall, was one of the top requested features for React Native.

[davidbiedenbach]

@yungsters, @d4vidi, If I may chime in here, I'm facing the same issue trying to implement an Intersection Observer native component and did a little digging.

It seems the implementation in question is in ViewGroup, not ViewRootImpl and it does in fact recurse up the view hierarchy:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/ViewGroup.java#6211

As such, @yungsters would it not be possible to override this method in ReactViewGroup.java, and instead of testing against getClipChildren(), test the value of mOverflow, or is there some nuance I'm missing?

[yungsters]

Ahhh… good catch. That might indeed work, and I am not aware of any nuance. Checking getClipChildren() should be sufficient (and implementing a custom intersection otherwise).

I don’t know when I’ll personally be able to get to trying this, but would you be willing to send a pull request?

[davidbiedenbach]

I've got a proof of concept working with a static re-implementation in my local codebase. I'll see if I can get it working as a proper override in ReactViewGroup and issue a PR. (Might have to go through a few local approvals; this is for a work project, but there is precedent for us contributing to RN, so it shouldn't be a problem)

[d4vidi]

@davidbiedenbach if you manage to do so it would be the best news I got all year :-)

[davidbiedenbach]

My apologies for the delay, I've had a bit of parental leave.

@d4vidi Here's a screenshot for you to whet your appetite:

In addition to overriding getChildVisibleRect in a few places, I had to explicitly set overflow to hidden in the scrollview's parent view's styling.

@yungsters A couple of issues have come up:

1. The override for getChildVisibleRect needs to be in multiple places - ReactViewGroup, and every other view that doesn't derive from ReactViewGroup (for example, ReactScrollView and ReactHorizontalScrollView). I trust that shouldn't be a problem?

2. In ReactViewGroup, mOverflow appears to be null when "overflow" is not explicitly set in a view's styling. I thought it should always be one of enum('visible', 'hidden', 'scroll') and default to 'visible'? Since the android clipChildren flag is always forced false even when clipping is enabled in the current RN implementation, mOverflow seems to be the only way to determine if child views should be clipped.

[davidbiedenbach]

@yungsters or is it better to get the PR together and discuss there?

[d4vidi]

Hilarious 🤣
Thanks for the effort, 'at the edge of my seat now - waiting for the PR!

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

[fungilation]

Arise, zombie bug. Arise!

[d4vidi]

@yungsters @mdvacca could we get another shot at this? Last time we tried it, @davidbiedenbach's PR was reverted because of an alleged increase of ANR's (see #23870). I think it's important enough so as to try again. Wdyt?

[davidbiedenbach]

Hey @d4vidi , finally circling back on this. I see the code hasn't been reverted per se, it's just gated by a feature flag.
ReactFeatureFlags.clipChildRectsIfOverflowIsHidden I guess if we set that to true, it'll enable the use of the code I submitted. I mentioned a while back that I had a static re-implementation of this method in an Intersection Observer module that I wrote for another app - that bit of code is running fine on Android with no ANRs. For the heck of it maybe I can try to enable this feature flag in that app and see what happens...

[davidbiedenbach]

(Also, I think you have the wrong link in your comment... ;-) Here's the merged PR: #26334)

[d4vidi]

Yikes, thanks for pointing that out 😄

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

[fungilation]

Merry Christmas stale bot! Stale not.

[github-actions]

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[d4vidi]

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

Still broken (we have automated tests the check that regularly)

[github-actions]

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[d4vidi]

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

This is still in effect, unfortunately. Could someone please revisit this, maybe retry the solution? We see Detox users getting "hit" by this every now and then.

[devoren]

Same issue ANR from google play

"main" tid=1 Runnable
  at android.view.ViewGroup.getChildVisibleRect (ViewGroup.java:6443)
  at android.view.ViewGroup.getChildVisibleRect (ViewGroup.java:6497)
  at android.view.ViewGroup.getChildVisibleRect (ViewGroup.java:6497)
  at android.view.ViewGroup.getChildVisibleRect (ViewGroup.java:6429)
  at com.facebook.react.views.view.ReactViewGroup.getChildVisibleRect (ReactViewGroup.java:478)
  at android.view.View.getGlobalVisibleRect (View.java:18929)
  at android.view.View.getGlobalVisibleRect (View.java:18949)
  at com.th3rdwave.safeareacontext.SafeAreaUtilsKt.getSafeAreaInsets (SafeAreaUtils.kt:73)
  at com.th3rdwave.safeareacontext.SafeAreaProvider.maybeUpdateInsets (SafeAreaProvider.kt:18)
  at com.th3rdwave.safeareacontext.SafeAreaProvider.onPreDraw (SafeAreaProvider.kt:39)
  at android.view.ViewTreeObserver.dispatchOnPreDraw (ViewTreeObserver.java:1093)
  at android.view.ViewRootImpl.performTraversals (ViewRootImpl.java:3708)
  at android.view.ViewRootImpl.doTraversal (ViewRootImpl.java:2442)
  at android.view.ViewRootImpl$TraversalRunnable.run (ViewRootImpl.java:9399)
  at android.view.Choreographer$CallbackRecord.run (Choreographer.java:1388)
  at android.view.Choreographer$CallbackRecord.run (Choreographer.java:1396)
  at android.view.Choreographer.doCallbacks (Choreographer.java:1033)
  at android.view.ChoreographerExtImpl.checkScrollOptSceneEnable (ChoreographerExtImpl.java:420)
  at android.view.Choreographer.doFrame (Choreographer.java:900)
  at android.view.Choreographer$FrameDisplayEventReceiver.run (Choreographer.java:1371)
  at android.os.Handler.handleCallback (Handler.java:942)
  at android.os.Handler.dispatchMessage (Handler.java:99)
  at android.os.Looper.loopOnce (Looper.java:240)
  at android.os.Looper.loop (Looper.java:351)
  at android.app.ActivityThread.main (ActivityThread.java:8381)
  at java.lang.reflect.Method.invoke (Native method)
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:584)
  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1013)

[fabOnReact]

I will implement #26334 and fix this issue.
Is anybody interested in having this added as a feature of react-native? you can enable it for your use cases

Relevant: wix/Detox#1208

[react-native-bot]

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[devoren]

Still broken?