react-native: `View.getGlobalVisibleRect()` is broken in some use cases

šŸ› Bug Report

With respect to the list of these commits - all of which involve child-views clipping on Android (AKA the overflow functionality): https://github.com/facebook/react-native/commit/b81c8b, https://github.com/facebook/react-native/commit/6110a4c, https://github.com/facebook/react-native/commit/bbdc12e, https://github.com/facebook/react-native/commit/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:

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

About this issue

  • Original URL
  • State: open
  • Created 5 years ago
  • Reactions: 91
  • Comments: 37 (25 by maintainers)

Commits related to this issue

Most upvoted comments

Merry Christmas stale bot! Stale not.

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

@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?

Arise, zombie bug. Arise!

Hey stale-bot šŸ˜„ I know itā€™s been a while, but we will have to get this fixed, eventually.

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)

@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?

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.

I will implement https://github.com/facebook/react-native/pull/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: https://github.com/wix/Detox/issues/1208

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.

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ā€¦