Skip to content

Commit

Permalink
Add overflowInset to RN Android ViewGroup as separate mount instruction
Browse files Browse the repository at this point in the history
Summary:
This diff adds `overflowInset` values in RN Android. These values are used to give  an enlarged boundary of a view group that also contains all its children layout. Here is [the post](https://fb.workplace.com/groups/yogalayout/permalink/2264980363573947/) that discuss more on why this is useful. I steal the pic in that post here as TLDR:

{F687030994}

In the above case, we will get overflowInset for ViewGroup A as something like `top: 0, right: -20, bottom: -20, left: 0`.

This has been added in the [Fabric core](https://fburl.com/code/f8c5tg7b) and [in IOS](https://fburl.com/code/vkh0hpt6). In Android, since we used to ignore all event coordinates outside of a ViewGroup boundary, this is not an issue. However, that caused unregistered touch area problem and got fixed in D30104853 (facebook@e35a963), which dropped the boundary check and made the hit test algorithm in [TouchTargetHelper.java](https://fburl.com/code/dj8jiz22) worse as we now need to explore all the child node under ReactRootNode.

This perf issue is getting obvious when a view loads too many items, which matches our experience with "Hover getting slow after scrolling", "Hover getting slow after going back from PDP view", and "The saved list view (in Explore) is very fast (because it has very few components)"

To fix this issue, I added the support to `overflowInset` to RN Android by
1. Sending the `overflowInset` values from Binding.cpp in RN Android as a separate mount instruction
2. Update `IntBufferBatchMountItem.java` to read the int buffer sent over JNI, and pass the `overflowInset` values to `SurfaceMountingManager.java`
3. Creating new interface `ReactOverflowViewWithInset.java` and extending the existing `ReactOverflowView.java` usages
4. Adding implementation of getter and setter for `overflowInset` in various views
5. Update `TouchTargetHelper.java` to read the values and check boundaries before exploring ViewGroup's children

Note that in #3 I didn't change `ReactOverflowView.java` interface directly. I am concerned about backward compatibility issues in case this interface is being used in OSS. I suggest we deprecate it as we are not using it anymore in our code.

Changelog:
[Internal][Android]

Reviewed By: JoshuaGross

Differential Revision: D33133977

fbshipit-source-id: 64e3e837fe7ca6e6dbdbc836ab0615182e10f28c
  • Loading branch information
ryancat authored and facebook-github-bot committed Dec 21, 2021
1 parent 821382b commit bc9168d
Show file tree
Hide file tree
Showing 10 changed files with 211 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ CppMountItem CppMountItem::UpdatePaddingMountItem(
ShadowView const &shadowView) {
return {CppMountItem::Type::UpdatePadding, {}, {}, shadowView, -1};
}
CppMountItem CppMountItem::UpdateOverflowInsetMountItem(
ShadowView const &shadowView) {
return {CppMountItem::Type::UpdateOverflowInset, {}, {}, shadowView, -1};
}

} // namespace react
} // namespace facebook
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ struct CppMountItem final {

static CppMountItem UpdatePaddingMountItem(ShadowView const &shadowView);

static CppMountItem UpdateOverflowInsetMountItem(
ShadowView const &shadowView);

#pragma mark - Type

enum Type {
Expand All @@ -58,7 +61,8 @@ struct CppMountItem final {
UpdateState = 64,
UpdateLayout = 128,
UpdateEventEmitter = 256,
UpdatePadding = 512
UpdatePadding = 512,
UpdateOverflowInset = 1024
};

#pragma mark - Fields
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ static inline int getIntBufferSizeForType(CppMountItem::Type mountItemType) {
return 5; // tag, top, left, bottom, right
case CppMountItem::Type::UpdateLayout:
return 6; // tag, x, y, w, h, DisplayType
case CppMountItem::Type::UpdateOverflowInset:
return 5; // tag, left, top, right, bottom
case CppMountItem::Undefined:
case CppMountItem::Multiple:
return -1;
Expand Down Expand Up @@ -84,6 +86,7 @@ static inline void computeBufferSizes(
std::vector<CppMountItem> &cppUpdateStateMountItems,
std::vector<CppMountItem> &cppUpdatePaddingMountItems,
std::vector<CppMountItem> &cppUpdateLayoutMountItems,
std::vector<CppMountItem> &cppUpdateOverflowInsetMountItems,
std::vector<CppMountItem> &cppUpdateEventEmitterMountItems) {
CppMountItem::Type lastType = CppMountItem::Type::Undefined;
int numSameType = 0;
Expand Down Expand Up @@ -128,6 +131,11 @@ static inline void computeBufferSizes(
cppUpdateLayoutMountItems.size(),
batchMountItemIntsSize,
batchMountItemObjectsSize);
updateBufferSizes(
CppMountItem::Type::UpdateOverflowInset,
cppUpdateOverflowInsetMountItems.size(),
batchMountItemIntsSize,
batchMountItemObjectsSize);
updateBufferSizes(
CppMountItem::Type::UpdateEventEmitter,
cppUpdateEventEmitterMountItems.size(),
Expand Down Expand Up @@ -232,6 +240,7 @@ void FabricMountingManager::executeMount(
std::vector<CppMountItem> cppUpdateStateMountItems;
std::vector<CppMountItem> cppUpdatePaddingMountItems;
std::vector<CppMountItem> cppUpdateLayoutMountItems;
std::vector<CppMountItem> cppUpdateOverflowInsetMountItems;
std::vector<CppMountItem> cppUpdateEventEmitterMountItems;

for (const auto &mutation : mutations) {
Expand Down Expand Up @@ -293,6 +302,16 @@ void FabricMountingManager::executeMount(
CppMountItem::UpdateLayoutMountItem(
mutation.newChildShadowView));
}

// OverflowInset: This is the values indicating boundaries including
// children of the current view. The layout of current view may not
// change, and we separate this part from layout mount items to not
// pack too much data there.
if (oldChildShadowView.layoutMetrics.overflowInset !=
newChildShadowView.layoutMetrics.overflowInset) {
cppUpdateOverflowInsetMountItems.push_back(
CppMountItem::UpdateOverflowInsetMountItem(newChildShadowView));
}
}

if (oldChildShadowView.eventEmitter !=
Expand Down Expand Up @@ -331,6 +350,13 @@ void FabricMountingManager::executeMount(
// Layout
cppUpdateLayoutMountItems.push_back(
CppMountItem::UpdateLayoutMountItem(mutation.newChildShadowView));

// OverflowInset: This is the values indicating boundaries including
// children of the current view. The layout of current view may not
// change, and we separate this part from layout mount items to not
// pack too much data there.
cppUpdateOverflowInsetMountItems.push_back(
CppMountItem::UpdateOverflowInsetMountItem(newChildShadowView));
}

// EventEmitter
Expand Down Expand Up @@ -359,6 +385,7 @@ void FabricMountingManager::executeMount(
cppUpdateStateMountItems,
cppUpdatePaddingMountItems,
cppUpdateLayoutMountItems,
cppUpdateOverflowInsetMountItems,
cppUpdateEventEmitterMountItems);

static auto createMountItemsIntBufferBatchContainer =
Expand Down Expand Up @@ -407,7 +434,7 @@ void FabricMountingManager::executeMount(
int intBufferPosition = 0;
int objBufferPosition = 0;
int prevMountItemType = -1;
jint temp[7];
jint temp[6];
for (int i = 0; i < cppCommonMountItems.size(); i++) {
const auto &mountItem = cppCommonMountItems[i];
const auto &mountItemType = mountItem.type;
Expand Down Expand Up @@ -594,6 +621,36 @@ void FabricMountingManager::executeMount(
intBufferPosition += 6;
}
}
if (!cppUpdateOverflowInsetMountItems.empty()) {
writeIntBufferTypePreamble(
CppMountItem::Type::UpdateOverflowInset,
cppUpdateOverflowInsetMountItems.size(),
env,
intBufferArray,
intBufferPosition);

for (const auto &mountItem : cppUpdateOverflowInsetMountItems) {
auto layoutMetrics = mountItem.newChildShadowView.layoutMetrics;
auto pointScaleFactor = layoutMetrics.pointScaleFactor;
auto overflowInset = layoutMetrics.overflowInset;

int overflowInsetLeft =
round(scale(overflowInset.left, pointScaleFactor));
int overflowInsetTop = round(scale(overflowInset.top, pointScaleFactor));
int overflowInsetRight =
round(scale(overflowInset.right, pointScaleFactor));
int overflowInsetBottom =
round(scale(overflowInset.bottom, pointScaleFactor));

temp[0] = mountItem.newChildShadowView.tag;
temp[1] = overflowInsetLeft;
temp[2] = overflowInsetTop;
temp[3] = overflowInsetRight;
temp[4] = overflowInsetBottom;
env->SetIntArrayRegion(intBufferArray, intBufferPosition, 5, temp);
intBufferPosition += 5;
}
}
if (!cppUpdateEventEmitterMountItems.empty()) {
writeIntBufferTypePreamble(
CppMountItem::Type::UpdateEventEmitter,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.facebook.react.fabric.mounting.mountitems.MountItem;
import com.facebook.react.touch.JSResponderHandler;
import com.facebook.react.uimanager.IllegalViewOperationException;
import com.facebook.react.uimanager.ReactOverflowViewWithInset;
import com.facebook.react.uimanager.ReactRoot;
import com.facebook.react.uimanager.ReactStylesDiffMap;
import com.facebook.react.uimanager.RootView;
Expand Down Expand Up @@ -758,6 +759,35 @@ public void updatePadding(int reactTag, int left, int top, int right, int bottom
viewManager.setPadding(viewToUpdate, left, top, right, bottom);
}

@UiThread
public void updateOverflowInset(
int reactTag,
int overflowInsetLeft,
int overflowInsetTop,
int overflowInsetRight,
int overflowInsetBottom) {
if (isStopped()) {
return;
}

ViewState viewState = getViewState(reactTag);
// Do not layout Root Views
if (viewState.mIsRoot) {
return;
}

View viewToUpdate = viewState.mView;
if (viewToUpdate == null) {
throw new IllegalStateException("Unable to find View for tag: " + reactTag);
}

if (viewToUpdate instanceof ReactOverflowViewWithInset) {
((ReactOverflowViewWithInset) viewToUpdate)
.setOverflowInset(
overflowInsetLeft, overflowInsetTop, overflowInsetRight, overflowInsetBottom);
}
}

@UiThread
public void updateState(final int reactTag, @Nullable StateWrapper stateWrapper) {
UiThreadUtil.assertOnUiThread();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public class IntBufferBatchMountItem implements MountItem {
static final int INSTRUCTION_UPDATE_LAYOUT = 128;
static final int INSTRUCTION_UPDATE_EVENT_EMITTER = 256;
static final int INSTRUCTION_UPDATE_PADDING = 512;
static final int INSTRUCTION_UPDATE_OVERFLOW_INSET = 1024;

private final int mSurfaceId;
private final int mCommitNumber;
Expand Down Expand Up @@ -163,11 +164,25 @@ public void execute(@NonNull MountingManager mountingManager) {
int width = mIntBuffer[i++];
int height = mIntBuffer[i++];
int displayType = mIntBuffer[i++];

surfaceMountingManager.updateLayout(reactTag, x, y, width, height, displayType);

} else if (type == INSTRUCTION_UPDATE_PADDING) {
surfaceMountingManager.updatePadding(
mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++]);
} else if (type == INSTRUCTION_UPDATE_OVERFLOW_INSET) {
int reactTag = mIntBuffer[i++];
int overflowInsetLeft = mIntBuffer[i++];
int overflowInsetTop = mIntBuffer[i++];
int overflowInsetRight = mIntBuffer[i++];
int overflowInsetBottom = mIntBuffer[i++];

surfaceMountingManager.updateOverflowInset(
reactTag,
overflowInsetLeft,
overflowInsetTop,
overflowInsetRight,
overflowInsetBottom);
} else if (type == INSTRUCTION_UPDATE_EVENT_EMITTER) {
surfaceMountingManager.updateEventEmitter(
mIntBuffer[i++], castToEventEmitter(mObjBuffer[j++]));
Expand Down Expand Up @@ -251,6 +266,15 @@ public String toString() {
mIntBuffer[i++],
mIntBuffer[i++],
mIntBuffer[i++]));
} else if (type == INSTRUCTION_UPDATE_OVERFLOW_INSET) {
s.append(
String.format(
"UPDATE OVERFLOWINSET [%d]: left:%d top:%d right:%d bottom:%d\n",
mIntBuffer[i++],
mIntBuffer[i++],
mIntBuffer[i++],
mIntBuffer[i++],
mIntBuffer[i++]));
} else if (type == INSTRUCTION_UPDATE_EVENT_EMITTER) {
j += 1;
s.append(String.format("UPDATE EVENTEMITTER [%d]\n", mIntBuffer[i++]));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

package com.facebook.react.uimanager;

import android.graphics.Rect;
import android.view.View;

/**
* Interface that should be implemented by {@link View} subclasses that support {@code overflow}
* style and want to use the overflowInset values. This allows the overflow information to be used
* by {@link TouchTargetHelper} to determine if a View is touchable.
*/
public interface ReactOverflowViewWithInset extends ReactOverflowView {
/**
* Get the overflow inset rect values which indicate the extensions to the boundaries of current
* view that wraps all of its children views
*
* @return Rect of integers indicating the left, top, right, bottom pixel extensions. The values
* are non-positive (indicating enlarged boundaries).
*/
Rect getOverflowInset();

/**
* Set the overflow inset rect values which indicate the extensions to the boundaries of current
* view that wraps all of its children views
*/
void setOverflowInset(int left, int top, int right, int bottom);
}
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,14 @@ private static View findTouchTargetView(
if (!isTouchPointInView(eventCoords[0], eventCoords[1], view)) {
// We don't allow touches on views that are outside the bounds of an `overflow: hidden` and
// `overflow: scroll` View.
if (view instanceof ReactOverflowView) {
@Nullable String overflow = ((ReactOverflowView) view).getOverflow();
if (view instanceof ReactOverflowViewWithInset) {
// If the touch point is outside of the overflowinset for the view, we can safely ignore
// it.
if (!isTouchPointInViewWithOverflowInset(eventCoords[0], eventCoords[1], view)) {
return null;
}

@Nullable String overflow = ((ReactOverflowViewWithInset) view).getOverflow();
if (ViewProps.HIDDEN.equals(overflow) || ViewProps.SCROLL.equals(overflow)) {
return null;
}
Expand Down Expand Up @@ -253,6 +259,16 @@ private static boolean isTouchPointInView(float x, float y, View view) {
}
}

private static boolean isTouchPointInViewWithOverflowInset(float x, float y, View view) {
if (!(view instanceof ReactOverflowViewWithInset)) {
return false;
}

final Rect overflowInset = ((ReactOverflowViewWithInset) view).getOverflowInset();
return (x >= -overflowInset.left && x < view.getWidth() - overflowInset.right)
&& (y >= -overflowInset.top && y < view.getHeight() - overflowInset.bottom);
}

/**
* Returns the coordinates of a touch in the child View. It is transform aware and will invert the
* transform Matrix to find the true local points This code is taken from {@link
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
import com.facebook.react.uimanager.MeasureSpecAssertions;
import com.facebook.react.uimanager.ReactClippingViewGroup;
import com.facebook.react.uimanager.ReactClippingViewGroupHelper;
import com.facebook.react.uimanager.ReactOverflowView;
import com.facebook.react.uimanager.ReactOverflowViewWithInset;
import com.facebook.react.uimanager.ViewProps;
import com.facebook.react.uimanager.events.NativeGestureUtil;
import com.facebook.react.views.scroll.ReactScrollViewHelper.HasFlingAnimator;
Expand All @@ -57,7 +57,7 @@
public class ReactHorizontalScrollView extends HorizontalScrollView
implements ReactClippingViewGroup,
FabricViewStateManager.HasFabricViewStateManager,
ReactOverflowView,
ReactOverflowViewWithInset,
HasScrollState,
HasFlingAnimator {

Expand All @@ -77,6 +77,7 @@ public class ReactHorizontalScrollView extends HorizontalScrollView
private final @Nullable OverScroller mScroller;
private final VelocityHelper mVelocityHelper = new VelocityHelper();
private final Rect mRect = new Rect();
private final Rect mOverflowInset = new Rect();

private boolean mActivelyScrolling;
private @Nullable Rect mClippingRect;
Expand Down Expand Up @@ -256,6 +257,16 @@ public void setOverflow(String overflow) {
return mOverflow;
}

@Override
public void setOverflowInset(int left, int top, int right, int bottom) {
mOverflowInset.set(left, top, right, bottom);
}

@Override
public Rect getOverflowInset() {
return mOverflowInset;
}

@Override
protected void onDraw(Canvas canvas) {
if (DEBUG_MODE) {
Expand Down
Loading

0 comments on commit bc9168d

Please sign in to comment.