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:
- 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
About this issue
- Original URL
- State: open
- Created 5 years ago
- Reactions: 91
- Comments: 37 (25 by maintainers)
Commits related to this issue
- Initial attempt at fixing issue #23870 by overriding getChildVisibleRect() in ReactViewGroup — committed to davidbiedenbach/react-native by davidbiedenbach 5 years ago
- Initial attempt at fixing issue #23870 by overriding getChildVisibleRect() in ReactViewGroup — committed to davidbiedenbach/react-native by davidbiedenbach 5 years ago
- Initial attempt at fixing issue #23870 by overriding getChildVisibleRect() in ReactViewGroup — committed to davidbiedenbach/react-native by davidbiedenbach 5 years ago
- Fix Issue 23870: Android API - View.getGlobalVisibleRect() does not properly clip result rect for ReactClippingViewGroups (#26334) Summary: This PR addresses issue https://github.com/facebook/react-n... — committed to facebook/react-native by davidbiedenbach 5 years ago
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ā¦