Skip to content

Commit

Permalink
Fix RemoveDeleteTree and subview clipping compatibility (#42355)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #42355

`RemoveDeleteTreeUIFrameCallback` operated directly on the view to clean up its children, which does not correctly account for subviews which have been clipped because they're outside the visible frame.

Changelog: [Android][Added] Added `removeAllViews` to IViewGroupManager.

Reviewed By: jehartzog, sammy-SC

Differential Revision: D52834835

fbshipit-source-id: fb7f07a17d07467eecd3ce9721afc2f3abcc0caa
  • Loading branch information
javache authored and facebook-github-bot committed Jan 18, 2024
1 parent ca2dde5 commit 3cd85dc
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 33 deletions.
4 changes: 2 additions & 2 deletions packages/react-native/ReactAndroid/api/ReactAndroid.api
Original file line number Diff line number Diff line change
Expand Up @@ -3987,6 +3987,7 @@ public abstract interface class com/facebook/react/uimanager/IViewGroupManager :
public abstract fun addView (Landroid/view/View;Landroid/view/View;I)V
public abstract fun getChildAt (Landroid/view/View;I)Landroid/view/View;
public abstract fun getChildCount (Landroid/view/View;)I
public fun removeAllViews (Landroid/view/View;)V
public abstract fun removeViewAt (Landroid/view/View;I)V
}

Expand Down Expand Up @@ -5061,7 +5062,6 @@ public abstract class com/facebook/react/uimanager/ViewGroupManager : com/facebo
public fun getShadowNodeClass ()Ljava/lang/Class;
public static fun getViewZIndex (Landroid/view/View;)Ljava/lang/Integer;
public fun needsCustomLayoutForChildren ()Z
public fun removeAllViews (Landroid/view/ViewGroup;)V
public fun removeView (Landroid/view/ViewGroup;Landroid/view/View;)V
public synthetic fun removeViewAt (Landroid/view/View;I)V
public fun removeViewAt (Landroid/view/ViewGroup;I)V
Expand Down Expand Up @@ -7519,7 +7519,7 @@ public abstract class com/facebook/react/views/view/ReactClippingViewManager : c
public synthetic fun getChildCount (Landroid/view/View;)I
public synthetic fun getChildCount (Landroid/view/ViewGroup;)I
public fun getChildCount (Lcom/facebook/react/views/view/ReactViewGroup;)I
public synthetic fun removeAllViews (Landroid/view/ViewGroup;)V
public synthetic fun removeAllViews (Landroid/view/View;)V
public fun removeAllViews (Lcom/facebook/react/views/view/ReactViewGroup;)V
public synthetic fun removeViewAt (Landroid/view/View;I)V
public synthetic fun removeViewAt (Landroid/view/ViewGroup;I)V
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public class SurfaceMountingManager {
private final Set<Integer> mErroneouslyReaddedReactTags = new HashSet<>();

@ThreadConfined(UI)
private RemoveDeleteTreeUIFrameCallback mRemoveDeleteTreeUIFrameCallback;
private @Nullable RemoveDeleteTreeUIFrameCallback mRemoveDeleteTreeUIFrameCallback;

// This is null *until* StopSurface is called.
private Set<Integer> mTagSetForStoppedSurfaceLegacy;
Expand Down Expand Up @@ -773,17 +773,9 @@ public void run() {

// The View has been removed from the View hierarchy; now it
// and all of its children, if any, need to be deleted, recursively.
// We want to maintain the legacy ordering: delete (and call onViewStateDeleted)
// for leaf nodes, and then parents, recursively.
// Schedule the Runnable first, to detect if we need to schedule a Runnable at all.
// Since this current function and the Runnable both run on the UI thread, there is
// no race condition here.
runDeferredTagRemovalAndDeletion();
mReactTagsToRemove.push(tag);
}

@UiThread
private void runDeferredTagRemovalAndDeletion() {
if (mReactTagsToRemove.empty()) {
if (mRemoveDeleteTreeUIFrameCallback == null) {
mRemoveDeleteTreeUIFrameCallback = new RemoveDeleteTreeUIFrameCallback(mThemedReactContext);
Expand All @@ -792,6 +784,7 @@ private void runDeferredTagRemovalAndDeletion() {
.postFrameCallback(
ReactChoreographer.CallbackType.IDLE_EVENT, mRemoveDeleteTreeUIFrameCallback);
}
mReactTagsToRemove.push(tag);
}

@UiThread
Expand Down Expand Up @@ -1455,19 +1448,20 @@ public void doFrameGuarded(long frameTimeNanos) {
ViewState thisViewState = getNullableViewState(reactTag);
if (thisViewState != null) {
View thisView = thisViewState.mView;
int numChildren = 0;
if (thisView instanceof ViewGroup) {
IViewGroupManager viewManager = getViewGroupManager(thisViewState);

// Children are managed by React Native if both of the following are true:
// 1) There are 1 or more children of this View, which must be a ViewGroup
// 2) Those children are managed by RN (this is not the case for certain native
// components, like embedded Litho hierarchies)
boolean childrenAreManaged = false;
// Children are managed by React Native if both of the following are true:
// 1) There are 1 or more children of this View, which must be a ViewGroup
// 2) Those children are managed by RN (this is not the case for certain native
// components, like embedded Litho hierarchies)
boolean childrenAreManaged = false;

if (thisView instanceof ViewGroup) {
View nextChild = null;
// For reasons documented elsewhere in this class, getChildCount is not
// necessarily reliable, and so we rely instead on requesting children directly.
while ((nextChild = ((ViewGroup) thisView).getChildAt(numChildren)) != null) {
View nextChild = null;
int numChildren = 0;
while ((nextChild = viewManager.getChildAt(thisView, numChildren)) != null) {
int childId = nextChild.getId();
childrenAreManaged = childrenAreManaged || getNullableViewState(childId) != null;
localChildren.push(nextChild.getId());
Expand All @@ -1487,16 +1481,17 @@ public void doFrameGuarded(long frameTimeNanos) {
// In debug mode, the SoftException will cause a crash. In production it
// will not. This should give good visibility into whether or not this is
// a problem without causing user-facing errors.
((ViewGroup) thisView).removeAllViews();
viewManager.removeAllViews(thisView);
} catch (RuntimeException e) {
childrenAreManaged = false;
ReactSoftExceptionLogger.logSoftException(TAG, e);
}
}
}
if (childrenAreManaged) {
// Push tags onto the stack so we process all children
mReactTagsToRemove.addAll(localChildren);

if (childrenAreManaged) {
// Push tags onto the stack so we process all children
mReactTagsToRemove.addAll(localChildren);
}
}

// Immediately remove tag and notify listeners.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package com.facebook.react.uimanager;

import android.view.View;
import com.facebook.react.bridge.UiThreadUtil;

/** Interface providing children management API for view managers of classes extending ViewGroup. */
public interface IViewGroupManager<T extends View> extends IViewManagerWithChildren {
Expand All @@ -21,6 +22,15 @@ public interface IViewGroupManager<T extends View> extends IViewManagerWithChild
/** Removes View from the parent View at the index specified as a parameter. */
void removeViewAt(T parent, int index);

/** Remove all child views from the parent View. */
default void removeAllViews(T parent) {
UiThreadUtil.assertOnUiThread();

for (int i = getChildCount(parent) - 1; i >= 0; i--) {
removeViewAt(parent, i);
}
}

/** Return the amount of children contained by the view specified as a parameter. */
int getChildCount(T parent);
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,6 @@ public void removeView(T parent, View view) {
}
}

public void removeAllViews(T parent) {
UiThreadUtil.assertOnUiThread();

for (int i = getChildCount(parent) - 1; i >= 0; i--) {
removeViewAt(parent, i);
}
}

/**
* Returns whether this View type needs to handle laying out its own children instead of deferring
* to the standard css-layout algorithm. Returns true for the layout to *not* be automatically
Expand Down

0 comments on commit 3cd85dc

Please sign in to comment.