Fix Issue 23870: Android API - View.getGlobalVisibleRect() does not properly clip result rect for ReactClippingViewGroups

[davidbiedenbach]

Summary

This PR addresses issue #23870 (View.getGlobalVisibleRect() is broken in some use cases)

The issue affects the following Android APIs:

• ViewGroup.getChildVisibleRect()
• View.getGlobalVisibleRect() (Which calls into ViewGroup.getChildVisibleRect)
• View.getLocalVisibleRect() (Which calls into View.getGlobalVisibleRect())

According to Android documentation, View.getGlobalVisibleRect() should provide a rect for a given view that has been clipped by the bounds of all of its parent views up the view hierarchy. It does so through the use of the recursive function ViewGroup.getChildVisibleRect().

Since React Native has a separate clipping mechanism that does not rely on Android UI's clipping implementation, ViewGroup.getChildVisibleRect() is unable to determine that a rect should be clipped if the clipping view is a ReactClippingViewGroup. This resultantly breaks some important use cases for things like testing with Detox, which relies on this functionality to tell when a component is on-screen, as explained in the above referenced issue.

The rationale of the fix is essentially to implement logic analogous to ViewGroup.getChildVisibleRect(), discarding irrelevant Android clipping modes, and instead testing against the 'overflow' property, restoring the originally intended functionality. This is implemented as an override to ViewGroup.getChildVisibleRect() in the following classes:

• ReactViewGroup
• ReactScrollView
• ReactHorizontalScrollView

Unfortunately, since the public ViewGroup.getChildVisibleRect() API recurses into a @hide annotated API which cannot be overridden, it was necessary to provide this override in each of the above React Native classes to ensure the superclass implementation would not be called, which would break the recursion.

Changelog

[Android] [Fixed] - View.getGlobalVisibleRect() clips result rect properly when overflow is 'hidden'

Test Plan

The functionality in question is neither used internally nor exposed by React Native, and thus only affects Android native modules that use the above referenced APIs.

As such, I have primarily performed testing with a forked example project that had been submitted with issue #23870, originally by @d4vidi.

The example project can be found here:

• Configured to build against RN Master
• Configured to build against PR branch

(Original project here: https://github.com/d4vidi/RNClipVisibilityBugDemo)

Bug in effect:

When built against RN master, it can be observed that fully clipped views are reported as visible, as in the below screenshots.

Views inside a ReactViewGroup do not report as clipped

Views inside a ReactScrollView do not report as clipped

Views inside a ReactHorizontalScrollView do not report clipping properly

Bug fixed

When built against the PR branch, fully-clipped views no longer report visible.

Views inside a ReactViewGroup report clipping properly

Views inside a ReactScrollView report clipping properly

Views inside a ReactHorizontalScrollView report clipping properly

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[shergin]

@davidbiedenbach Could you please change the title to be more descriptive?

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[davidbiedenbach]

@davidbiedenbach Could you please change the title to be more descriptive?

@shergin Absolutely. Is this better?

[d4vidi]

@davidbiedenbach why give up on android's optimizations, such as:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/ViewGroup.java#6192
and:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/ViewGroup.java#6195

[davidbiedenbach]

@d4vidi I tried to do it that way at first, but unfortunately this class doesn't have access to the superclass members required to use those optimizations. Especially without some getters that weren't introduced until Android API 18 (don't want to break 16 for RN users unnecessarily)

[d4vidi]

🎉

[davidbiedenbach]

The magic line ;-)

[d4vidi]

final this too? :)

[davidbiedenbach]

Oops. Must have been a copy-paste error (I was comparing against different versions of the Android implementations) ;-).

[d4vidi]

What's about the FLAG_CLIP_TO_PADDING - should you not keep that test as well?
ref: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/ViewGroup.java#6229
Perhaps this is a question for @yungsters, as I'm not sure whether its usage is legit anywhere in RN

[davidbiedenbach]

@d4vidi, yeah I considered that too, but again due to access level, it didn't appear to be possible.

[d4vidi]

I think you ought to override the method flavor that has the forceParentCheck param and take it into account. It's pretty easy to add, isn't it?

[davidbiedenbach]

@d4vidi
I wanted to, but since the original for that flavor has @hide in the comment, it's not available to override. I tried implementing it anyway, but when the ViewGroup implementation tries to call it (via a cast) it never reaches the subclass implementation anyway :(

[davidbiedenbach]

See https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/ViewGroup.java#6185

[davidbiedenbach]

Apparently, the ViewGroup implementation of getChildVisibleRect() is the only one that calls into an auxiliary function to force recursion when not necessary. It seems it was written for some kind of internal testing, and then marked @hide to prevent it from being used by the public.

[d4vidi]

Last but not least -- lot's of code duplication here... how about exporting all of this to an external util of some sort?

[davidbiedenbach]

I agree entirely. Was torn on this as well but decided to go with this way to get feedback first. I suppose I can always pass the value of mOverflow or any other dependencies.

ReactClippingViewGroupHelper.java seems like a good candidate for such a function. I'll take a look.

[d4vidi]

@davidbiedenbach well, 'added some comments, would love it if you could go over them! In any case the solution seems great so far.

[davidbiedenbach]

@d4vidi Thanks for your feedback! I've addressed what I could. I'd have liked to have kept a little closer to the Android implementation, but it just doesn't seem to be possible (see my replies to your individual comments). It is validating that we largely had the same instincts however ;-)

[matthargett]

Thanks for taking the time to make a narrow reproduction app. This change looks good to me, and people with production apps have verified the fix with no other ill effects.

[facebook-github-bot]

@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @davidbiedenbach in df9abf7.

^{When will my fix make it into a release? | Upcoming Releases}

[mdvacca]

Heads up @davidbiedenbach that we just created a ReactFeatureFlag to control the custom implementation of the getChildVisibleRect() method introduced by this PR.

The reason is that we are experiencing an increase of ANRs reported in RN Android apps because of the usage of the new method getChildVisibleRectHelper().

For reference ANR stacktraces are showing:

  at java.lang.Math.floor!(Native method)
  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:97)
  at com.facebook.react.views.view.ReactViewGroup.getChildVisibleRect(ReactViewGroup.java:433)
  at android.view.ViewGroup.getChildVisibleRect(ViewGroup.java:5141)
  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:103)
  at com.facebook.react.views.scroll.ReactScrollView.getChildVisibleRect(ReactScrollView.java:348)
  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:103)
  at com.facebook.react.views.view.ReactViewGroup.getChildVisibleRect(ReactViewGroup.java:433)
  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:103)
  at com.facebook.react.views.view.ReactViewGroup.getChildVisibleRect(ReactViewGroup.java:433)
  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:103)
  at com.facebook.react.views.view.ReactViewGroup.getChildVisibleRect(ReactViewGroup.java:433)
  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:103)
  at com.facebook.react.views.view.ReactViewGroup.getChildVisibleRect(ReactViewGroup.java:433)
  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:103)
  at com.facebook.react.views.scroll.ReactScrollView.getChildVisibleRect(ReactScrollView.java:348)
  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:103)
  at com.facebook.react.views.view.ReactViewGroup.getChildVisibleRect(ReactViewGroup.java:433)
  at android.view.ViewGroup.getChildVisibleRect(ViewGroup.java:5141)
  at android.view.ViewGroup.getChildVisibleRect(ViewGroup.java:5141)
  at android.view.ViewGroup.getChildVisibleRect(ViewGroup.java:5141)
  at android.view.View.getGlobalVisibleRect(View.java:11542)
  at android.view.View.getGlobalVisibleRect(View.java:11548)
  at ...
  at android.os.Handler.handleCallback(Handler.java:739)
  at android.os.Handler.dispatchMessage(Handler.java:95)
  at android.os.Looper.loop(Looper.java:135)
  at android.app.ActivityThread.main(ActivityThread.java:5536)
  at java.lang.reflect.Method.invoke!(Native method)
  at java.lang.reflect.Method.invoke(Method.java:372)
  at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1397)
  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1192)

The new FeatureFlag is called: "ReactFeatureFlag.clipChildRectsIfOverflowIsHidden" and it is disabled by default (you can re-enable this code just toggling the value of this variable).
See 9c61693

@davidbiedenbach can you help analyzing why the original PR is increasing ANRs?

Thanks!

CC @yungsters

[davidbiedenbach]

@mdvacca Thanks for the heads up, I'll investigate. I'm guessing something unexpected is happening with the view->parent traversal. I don't suppose there's a known repro case? Anyway, I'll follow up here with what I find out.

[mdvacca]

Unfortunately we don't have a known repro case.
Thank you

[d4vidi]

This is quite unfortunate. @davidbiedenbach I'd be more than willing to help in solving it. On that note - @mdvacca you've kindly provided the stacktrace but not the error itself; Could you please share the error message (what can be wrong with Math.floor?...), I understand the reference is T57363204... Thanks!

[yungsters]

There is no error message because the increase is in Android ANRs, which stands for “Application Not Responding”.

For example, prolonged execution on the UI thread as a result of this original pull request may cause an increase in ANRs.

For whatever reason, the stack trace that @mdvacca shared is when the increase in ANRs is happening.

[davidbiedenbach]

@d4vidi I definitely appreciate any help you can provide... I'm a little tied up and haven't been able to dig in just yet. But, a couple things about the stack trace look odd. One being that Math.floor() is at the top, but I'm guessing that's just coincidental to the timing of when the OS decided to kill the app. The other being this:

  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:97)
  at com.facebook.react.views.view.ReactViewGroup.getChildVisibleRect(ReactViewGroup.java:433)
->  at android.view.ViewGroup.getChildVisibleRect(ViewGroup.java:5141)
  at com.facebook.react.uimanager.ReactClippingViewGroupHelper.getChildVisibleRectHelper(ReactClippingViewGroupHelper.java:103)

It strikes me as odd that one view near the top of the stack seems to have a non-RN parent view (or at least calls the ViewGroup implementation of getChildVisibleRect) and that one calls back into the RN override. I guess it's also a little odd that getChildVisibleRect() is even being called at all, since I hadn't found anything in the RN codebase that calls it, the last time I checked.

I suppose one completely unrelated possibility is that the optimizations I threw away because of lack of access to the necessary class members are more important than it seemed, in which case we might need to find a way to re-implement them...

[d4vidi]

@yungsters thanks for this elaborate explanation :)

@davidbiedenbach Just to be on the strict side here, the stacktrace is probably from the exact moment the ANR showed, as the app doesn't get killed offhand (user has to take a proactive action). Typically, when an ANR occurs, Android provides a complete thread-dump in trace files -- I suppose the stack trace is from there. On that note, @yungsters @mdvacca could you possibly provide the complete thread dump info on top of the ui-thread stacktrace snippet? I suppose we would find that very helpful.

In any case, I do agree that the fact that a 'plain' ViewGroup is among a react-native view-hierarchy's elements is weird. Also, I noticed two ReactScrollView elements in the stack; While this is a legitimate use case, all-in-all maybe it could shed light on what the edge-cases are where this situation could be introduced.

Nevertheless I'm far from convinced that the lack of optimizations is at fault here; ANR only shows after events await in queue for 5 whole seconds (!) Omitting some optimization should induce a janky app, at worst. We should therefore ought to follow a different direction (imo)...

As for calling getChildVisibleRect() - this is mostly likely rooted in the app's code, rather than RN's; For example, trying to asses the visibility of an ad while the user is scrolling, is a reasonable scenario.

[davidbiedenbach]

@mdvacca @yungsters

Hey guys, just wanted to circle back on this. I've made several attempts to repro this ANR but I haven't had any luck. To the best of your knowledge, is it ever possible for the view hierarchy on android to contain a loop? Because the only thing I can think of that could be causing a problem is some kind of infinite recursion, which really shouldn't be happening since I'm only traversing up the hierarchy via mParent.

I'm wondering if we can try turning this back on by default & see if the ANRs come back or not. Alternatively, I can try to add some kind of recursion depth limit or looping safeguard in another PR. What's the best path forward here? Is there any way to get in touch with someone that experienced these ANRs in their app?

cc @d4vidi

[d4vidi]

@davidbiedenbach I'm shooting in the dark here but I went about comparing again this implementation vs. android native's (SDK's 28, 29) and there are some differences - 2 major ones, in particular:

1. Some of the computation done in Android's SDK are restricted to whether the child has a transformation matrix (i.e. usage of !child.hasIdentityMatrix()). Can we really assume that in RN there's no matrix set (namely, because native animation are not typically used)? What about "brownfield" apps?
2. With respect to the initially formed rect, here we just use a new RectF() while on Android SDK's the view's attach-info is taken into account (namely, some transient property called mTmpTransformRect).

I know none of these should affect ANR's per se, but aren't they worth addressing, before anything else? ...or am I way off here? Wdyt?