Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6cf12aa
Initial attempt at fixing issue #23870 by overriding getChildVisibleR…
davidbiedenbach Sep 4, 2019
4a8359c
Ensure non-ReactViewGroup-derived ViewGroups will use the correct imp…
davidbiedenbach Sep 4, 2019
d5d8096
Merge remote-tracking branch 'origin/master' into issue_23870_fix
davidbiedenbach Sep 12, 2019
c33899c
Remove uncalled override of @hide marked API, add getChildVisibleRect…
davidbiedenbach Sep 12, 2019
85febce
remove unnecessary log statements
davidbiedenbach Sep 16, 2019
57383f6
One more getChildVisibleRect override was necessary to maintain recur…
davidbiedenbach Sep 16, 2019
149ef3e
Move replicated code into ReactClippingViewGroupHelper.java, missing …
davidbiedenbach Sep 18, 2019
ee4f12c
Clean up unused imports
davidbiedenbach Sep 18, 2019
587f26c
Merge branch 'master' into issue_23870_fix
davidbiedenbach Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactClippingViewGroupHelper.java
View file
Open in desktop
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,47 @@ 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;
}
}
5 changes: 5 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,11 @@ 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
5 changes: 5 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,11 @@ 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
5 changes: 5 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,11 @@ private void updateSubviewClipStatus(View subview) {
}
}

@Override
public boolean getChildVisibleRect(View child, Rect r, android.graphics.Point offset) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return ReactClippingViewGroupHelper.getChildVisibleRectHelper(child, r, offset, this, mOverflow);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Override
protected void onSizeChanged(int w, int h, int oldw, int oldh) {
super.onSizeChanged(w, h, oldw, oldh);
Expand Down