From df9abf798351c43253c449fe2c83c2cca0479d80 Mon Sep 17 00:00:00 2001 From: David Biedenbach Date: Mon, 7 Oct 2019 22:21:47 -0700 Subject: [PATCH] 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-native/issues/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()](https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/ViewGroup.java#6176), 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' Pull Request resolved: https://github.com/facebook/react-native/pull/26334 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 https://github.com/facebook/react-native/issues/23870, originally by d4vidi. The example project can be found here: - [Configured to build against RN Master](https://github.com/davidbiedenbach/RNClipVisibilityBugDemo/tree/rn-master) - [Configured to build against PR branch](https://github.com/davidbiedenbach/RNClipVisibilityBugDemo/tree/fix-23870) (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 ![BugScreen1](https://user-images.githubusercontent.com/1563532/64999573-314b6300-d89d-11e9-985e-294bd51a0ba9.jpg) #### Views inside a ReactScrollView do not report as clipped ![BugScreen2](https://user-images.githubusercontent.com/1563532/64999580-38727100-d89d-11e9-8186-96b25c937edc.jpg) #### Views inside a ReactHorizontalScrollView do not report clipping properly ![BugScreen4](https://user-images.githubusercontent.com/1563532/64999588-3f00e880-d89d-11e9-9477-7b79e44c5e46.jpg) ### Bug fixed When built against the PR branch, fully-clipped views no longer report visible. #### Views inside a ReactViewGroup report clipping properly ![FixScreen1](https://user-images.githubusercontent.com/1563532/64999634-6b1c6980-d89d-11e9-8534-b26b638bf4d8.jpg) #### Views inside a ReactScrollView report clipping properly ![FixScreen2](https://user-images.githubusercontent.com/1563532/64999641-7079b400-d89d-11e9-8f95-4d6e28bcf833.jpg) #### Views inside a ReactHorizontalScrollView report clipping properly ![FixScreen4](https://user-images.githubusercontent.com/1563532/64999645-74a5d180-d89d-11e9-9754-170bb3b620a2.jpg) Reviewed By: mdvacca Differential Revision: D17782658 Pulled By: yungsters fbshipit-source-id: 0cd0d385898579a7a8a3e453f6ba681679ebe496 --- .../ReactClippingViewGroupHelper.java | 48 +++++++++++++++++++ .../scroll/ReactHorizontalScrollView.java | 6 +++ .../react/views/scroll/ReactScrollView.java | 6 +++ .../react/views/view/ReactViewGroup.java | 6 +++ 4 files changed, 66 insertions(+) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactClippingViewGroupHelper.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactClippingViewGroupHelper.java index 78e51eaa21b08c..019369a102bacb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactClippingViewGroupHelper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactClippingViewGroupHelper.java @@ -7,6 +7,7 @@ package com.facebook.react.uimanager; import android.graphics.Rect; +import android.graphics.RectF; import android.view.View; import android.view.ViewParent; import javax.annotation.concurrent.NotThreadSafe; @@ -55,4 +56,51 @@ public static void calculateClippingRect(View view, Rect outputRect) { } view.getDrawingRect(outputRect); } + + public static boolean getChildVisibleRectHelper( + View child, Rect r, android.graphics.Point offset, View parent, String overflow) { + // This is based on the Android ViewGroup implementation, modified to clip child rects + // if overflow is set to ViewProps.HIDDEN. This effectively solves Issue #23870 which + // appears to have been introduced by FLAG_CLIP_CHILDREN being forced false + // regardless of whether clipping is desired. + final RectF rect = new RectF(); + rect.set(r); + + child.getMatrix().mapRect(rect); + + final int dx = child.getLeft() - parent.getScrollX(); + final int dy = child.getTop() - parent.getScrollY(); + + rect.offset(dx, dy); + + if (offset != null) { + float[] position = new float[2]; + position[0] = offset.x; + position[1] = offset.y; + child.getMatrix().mapPoints(position); + offset.x = Math.round(position[0]) + dx; + offset.y = Math.round(position[1]) + dy; + } + + final int width = parent.getRight() - parent.getLeft(); + final int height = parent.getBottom() - parent.getTop(); + + boolean rectIsVisible = true; + + ViewParent grandparent = parent.getParent(); + if (grandparent == null || ViewProps.HIDDEN.equals(overflow)) { + rectIsVisible = rect.intersect(0, 0, width, height); + } + + r.set( + (int) Math.floor(rect.left), + (int) Math.floor(rect.top), + (int) Math.ceil(rect.right), + (int) Math.ceil(rect.bottom)); + + if (rectIsVisible && grandparent != null) { + rectIsVisible = grandparent.getChildVisibleRect(parent, r, offset); + } + return rectIsVisible; + } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java index b0c12831a01476..85c86303e801c4 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java @@ -491,6 +491,12 @@ public void getClippingRect(Rect outClippingRect) { outClippingRect.set(Assertions.assertNotNull(mClippingRect)); } + @Override + public boolean getChildVisibleRect(View child, Rect r, android.graphics.Point offset) { + return ReactClippingViewGroupHelper.getChildVisibleRectHelper( + child, r, offset, this, mOverflow); + } + private int getSnapInterval() { if (mSnapInterval != 0) { return mSnapInterval; diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java index db071fbb49ded7..083922e655fe4a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java @@ -342,6 +342,12 @@ public void getClippingRect(Rect outClippingRect) { outClippingRect.set(Assertions.assertNotNull(mClippingRect)); } + @Override + public boolean getChildVisibleRect(View child, Rect r, android.graphics.Point offset) { + return ReactClippingViewGroupHelper.getChildVisibleRectHelper( + child, r, offset, this, mOverflow); + } + @Override public void fling(int velocityY) { // Workaround. diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java index e6b8dcd9bdc1d8..a3af32bd7f7b12 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java @@ -455,6 +455,12 @@ private void updateSubviewClipStatus(View subview) { } } + @Override + public boolean getChildVisibleRect(View child, Rect r, android.graphics.Point offset) { + return ReactClippingViewGroupHelper.getChildVisibleRectHelper( + child, r, offset, this, mOverflow); + } + @Override protected void onSizeChanged(int w, int h, int oldw, int oldh) { super.onSizeChanged(w, h, oldw, oldh);