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

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 6 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
49 changes: 49 additions & 0 deletions ...oid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollContainerView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,15 @@
package com.facebook.react.views.scroll;

import android.content.Context;
import android.graphics.Rect;
import android.graphics.RectF;
import android.view.View;
import android.view.ViewGroup;
import android.view.ViewParent;
import android.widget.HorizontalScrollView;
import androidx.core.view.ViewCompat;
import com.facebook.react.modules.i18nmanager.I18nUtil;
import com.facebook.react.uimanager.ViewProps;

/** Container of Horizontal scrollViews that supports RTL scrolling. */
public class ReactHorizontalScrollContainerView extends ViewGroup {
Expand Down Expand Up @@ -45,4 +50,48 @@ protected void onLayout(boolean changed, int left, int top, int right, int botto
}
mCurrentWidth = getWidth();
}

@Override
public boolean getChildVisibleRect(View child, Rect r, android.graphics.Point offset) {
// 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() - getScrollX();
int dy = child.getTop() - 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 = getRight() - getLeft();
final int height = getBottom() - getTop();

boolean rectIsVisible = true;

ViewParent parent = getParent();
if (parent == null) {
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 && parent != null) {
rectIsVisible = parent.getChildVisibleRect(this, r, offset);
}
return rectIsVisible;
}
}
46 changes: 46 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 @@ -10,13 +10,15 @@
import android.graphics.Canvas;
import android.graphics.Color;
import android.graphics.Rect;
import android.graphics.RectF;
import android.graphics.drawable.ColorDrawable;
import android.graphics.drawable.Drawable;
import android.util.Log;
import android.view.FocusFinder;
import android.view.KeyEvent;
import android.view.MotionEvent;
import android.view.View;
import android.view.ViewParent;
import android.widget.HorizontalScrollView;
import android.widget.OverScroller;
import androidx.annotation.Nullable;
Expand Down Expand Up @@ -491,6 +493,50 @@ public void getClippingRect(Rect outClippingRect) {
outClippingRect.set(Assertions.assertNotNull(mClippingRect));
}

@Override
public boolean getChildVisibleRect(View child, Rect r, android.graphics.Point offset) {
// 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() - getScrollX();
int dy = child.getTop() - 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 = getRight() - getLeft();
final int height = getBottom() - getTop();

boolean rectIsVisible = true;

ViewParent parent = getParent();
if (parent == null || ViewProps.HIDDEN.equals(mOverflow)) {
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 && parent != null) {
rectIsVisible = parent.getChildVisibleRect(this, r, offset);
}
return rectIsVisible;
}

private int getSnapInterval() {
if (mSnapInterval != 0) {
return mSnapInterval;
Expand Down
46 changes: 46 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 @@ -9,13 +9,15 @@
import android.graphics.Canvas;
import android.graphics.Color;
import android.graphics.Rect;
import android.graphics.RectF;
import android.graphics.drawable.ColorDrawable;
import android.graphics.drawable.Drawable;
import android.util.Log;
import android.view.KeyEvent;
import android.view.MotionEvent;
import android.view.View;
import android.view.ViewGroup;
import android.view.ViewParent;
import android.widget.OverScroller;
import android.widget.ScrollView;
import androidx.annotation.Nullable;
Expand Down Expand Up @@ -342,6 +344,50 @@ public void getClippingRect(Rect outClippingRect) {
outClippingRect.set(Assertions.assertNotNull(mClippingRect));
}

@Override
public boolean getChildVisibleRect(View child, Rect r, android.graphics.Point offset) {
// 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() - getScrollX();
int dy = child.getTop() - 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 = getRight() - getLeft();
final int height = getBottom() - getTop();

boolean rectIsVisible = true;

ViewParent parent = getParent();
if (parent == null || ViewProps.HIDDEN.equals(mOverflow)) {
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 && parent != null) {
rectIsVisible = parent.getChildVisibleRect(this, r, offset);
}
return rectIsVisible;
}

@Override
public void fling(int velocityY) {
// Workaround.
Expand Down
45 changes: 45 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 @@ -24,6 +24,7 @@
import android.view.MotionEvent;
import android.view.View;
import android.view.ViewGroup;
import android.view.ViewParent;
import android.view.ViewStructure;
import android.view.animation.Animation;
import androidx.annotation.NonNull;
Expand Down Expand Up @@ -455,6 +456,50 @@ 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.

// 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();
Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

@davidbiedenbach davidbiedenbach Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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)

rect.set(r);

child.getMatrix().mapRect(rect);

final int dx = child.getLeft() - getScrollX();
int dy = child.getTop() - getScrollY();
Copy link
Contributor

Choose a reason for hiding this comment

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

final this too? :)

Copy link
Contributor Author

@davidbiedenbach davidbiedenbach Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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


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 = getRight() - getLeft();
final int height = getBottom() - getTop();

boolean rectIsVisible = true;

ViewParent parent = getParent();
if (parent == null || ViewProps.HIDDEN.equals(mOverflow)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The magic line ;-)

rectIsVisible = rect.intersect(0, 0, width, height);
}
Copy link
Contributor

@d4vidi d4vidi Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@davidbiedenbach davidbiedenbach Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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


r.set((int) Math.floor(rect.left), (int) Math.floor(rect.top),
(int) Math.ceil(rect.right), (int) Math.ceil(rect.bottom));

if (rectIsVisible && parent != null) {
rectIsVisible = parent.getChildVisibleRect(this, r, offset);
}
return rectIsVisible;
}

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