Skip to content

Commit

Permalink
Fix Issue 23870: Android API - View.getGlobalVisibleRect() does not p…
Browse files Browse the repository at this point in the history
…roperly clip result rect for ReactClippingViewGroups (#26334)

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()](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: #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 #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
  • Loading branch information
davidbiedenbach authored and facebook-github-bot committed Oct 8, 2019
1 parent 3b51499 commit df9abf7
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit df9abf7

Please sign in to comment.