Skip to content

Fix handling removal of transitioning views #47634

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

Closed
kkafar wants to merge 14 commits into from
Closed

Fix handling removal of transitioning views #47634

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0c310cb
Prevent removal from parent when transitioning
kkafar Nov 15, 2024
ca9a22d
Track children removed while transitioning
kkafar Nov 23, 2024
00e0e52
Merge main && refactor the code to comply with recent changes
kkafar Nov 23, 2024
150c15e
Cleanup
kkafar Nov 24, 2024
48b90ad
Cleanup 2
kkafar Nov 24, 2024
6e15352
Cleanup 3
kkafar Nov 24, 2024
4d6b796
Prevent bug when setting removeClippedSubviews to `false` after `endV…
kkafar Nov 24, 2024
93b0a35
Add missing import
kkafar Nov 24, 2024
c3da8d0
Allocate hashset only when necessary
kkafar Nov 26, 2024
09b7c62
Merge branch 'main' into @kkafar/support-for-view-transition
kkafar Nov 26, 2024
eb4e4fd
Get rid of redundant null check
kkafar Nov 27, 2024
c14e026
Review part one
kkafar Nov 27, 2024
a7c7af3
Just clear the hash set, do not release it
kkafar Nov 27, 2024
f54164d
Support cases where not all children are transitioning at once
kkafar Dec 2, 2024
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
4 changes: 1 addition & 3 deletions ...tive/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactClippingViewManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,12 @@ public abstract class ReactClippingViewManager<T : ReactViewGroup> : ViewGroupMa
if (removeClippedSubviews) {
val child = getChildAt(parent, index)
if (child != null) {
if (child.parent != null) {
parent.removeView(child)
}
Comment on lines -65 to -67
Copy link
Member

@javache javache Nov 26, 2024

Choose a reason for hiding this comment

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

Hmm, this is really interesting, and may explain a crash we're seeing. I wonder why we were previously removing this subview twice - potentially messing up some state in ReactViewGroup.

What's your reasoning for removing this?

Copy link
Contributor Author

@kkafar kkafar Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@javache thanks for review. My logic was as follows:

before 0b22b95 you relied on handleRemoveView being called to perform side effects regarding child drawing order. Implementation of ReactViewgroup.removeView took care of handling these sideeffects and detaching child view from parent.

On the other hand ReactViewGroup.removeViewWithSubviewClippingEnabled had also two responsibilities:

  1. detach child from parent if it has not been detached yet (notice the guard here!)
  2. remove child from mAllChildren array, which also retained children already detached due to clipping.

Notice, that it had not performed handleRemoveView.

My theory is that in clipping view manager you had call to parent.removeView(child) to ensure sideeffects of handleRemoveView were performed and then you called parent.removeViewWithSubviewClippingEnabled(child) to remove child from mAllChildren array, relying on the guard from (1), which prevented Android crash potentially caused by removing twice the same child view.

After 0b22b95 side effects of handleRemoveView have been moved to onViewRemoved which is called on any attempt of child removal by the Android framework. Therefore we can remove the call to parent.removeView as all required side effects will be performed by the call to parent.removeViewWithSubviewClippingEnabled(child) - drawing order code, removal from mAllChildren and child detachment. I've left the check for nullish child, as I couldn't determine what was the logic behind it.

parent.removeViewWithSubviewClippingEnabled(child)
}
} else {
parent.removeViewAt(index)
}

}

override fun removeAllViews(parent: T) {
Expand Down
35 changes: 33 additions & 2 deletions ...react-native/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 @@ -133,6 +133,9 @@ public void shutdown() {
private @Nullable ViewGroupDrawingOrderHelper mDrawingOrderHelper;
private float mBackfaceOpacity;
private String mBackfaceVisibility;
private boolean mIsTransitioning = false;
private @Nullable Set<Integer> mChildrenRemovedWhileTransitioning = null;
Copy link
Contributor Author

@kkafar kkafar Nov 24, 2024

Choose a reason for hiding this comment

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

I'm using Set<Integer> (holding only the view tag) instead of Set<View> to avoid any retain issues.

Copy link
Member

@javache javache Nov 26, 2024

Choose a reason for hiding this comment

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

Consider using androidx.collection.MutableIntSet instead to avoid Integer boxing.

Copy link
Contributor Author

@kkafar kkafar Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I was not aware of this API! Hover it seems that it has been added few months ago in androidx.collection.MutableIntSet 1.4.0, while my development project (using recent react-native 0.76) uses version 1.1.0. I'm not sure where would I need to bump / add this dependency in RN to make use of it.

Copy link
Member

@javache javache Nov 26, 2024

Choose a reason for hiding this comment

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

Looks like this would need to be a new dependency in https://github.com/facebook/react-native/blob/main/packages/react-native/gradle/libs.versions.toml#L45 - let's add a comment to change this in the future.



/**
* Creates a new `ReactViewGroup` instance.
Expand Down Expand Up @@ -360,12 +363,14 @@ public void setRemoveClippedSubviews(boolean removeClippedSubviews) {
mAllChildren[i] = child;
child.addOnLayoutChangeListener(mChildrenLayoutChangeListener);
}
mChildrenRemovedWhileTransitioning = new HashSet<>();
updateClippingRect();
} else {
// Add all clipped views back, deallocate additional arrays, remove layoutChangeListener
Assertions.assertNotNull(mClippingRect);
Assertions.assertNotNull(mAllChildren);
Assertions.assertNotNull(mChildrenLayoutChangeListener);
Assertions.assertNotNull(mChildrenRemovedWhileTransitioning);
for (int i = 0; i < mAllChildrenCount; i++) {
mAllChildren[i].removeOnLayoutChangeListener(mChildrenLayoutChangeListener);
}
Expand All @@ -375,6 +380,7 @@ public void setRemoveClippedSubviews(boolean removeClippedSubviews) {
mClippingRect = null;
mAllChildrenCount = 0;
mChildrenLayoutChangeListener = null;
mChildrenRemovedWhileTransitioning = null;
}
}

Expand All @@ -401,6 +407,23 @@ public void updateClippingRect() {
updateClippingToRect(mClippingRect);
}

@Override
public void startViewTransition(View view) {
super.startViewTransition(view);
mIsTransitioning = true;
}

@Override
public void endViewTransition(View view) {
super.endViewTransition(view);
mIsTransitioning = false;
if (mChildrenRemovedWhileTransitioning != null) {
mChildrenRemovedWhileTransitioning.clear();
mChildrenRemovedWhileTransitioning = null;
}
}


private void updateClippingToRect(Rect clippingRect) {
Assertions.assertNotNull(mAllChildren);
int clippedSoFar = 0;
Expand Down Expand Up @@ -547,6 +570,11 @@ public void onViewRemoved(View child) {
} else {
setChildrenDrawingOrderEnabled(false);
}

if (mIsTransitioning && mChildrenRemovedWhileTransitioning != null) {
mChildrenRemovedWhileTransitioning.add(child.getId());
}

super.onViewRemoved(child);
}

Expand Down Expand Up @@ -661,11 +689,13 @@ public void run() {

/*package*/ void removeViewWithSubviewClippingEnabled(View view) {
UiThreadUtil.assertOnUiThread();

Assertions.assertCondition(mRemoveClippedSubviews);
Assertions.assertNotNull(mClippingRect);
Assertions.assertNotNull(mChildrenRemovedWhileTransitioning);

View[] childArray = Assertions.assertNotNull(mAllChildren);
view.removeOnLayoutChangeListener(mChildrenLayoutChangeListener);

int index = indexOfChildInAllChildren(view);
if (!isViewClipped(childArray[index])) {
int clippedSoFar = 0;
Expand All @@ -677,6 +707,7 @@ public void run() {
removeViewsInLayout(index - clippedSoFar, 1);
}
removeFromArray(index);

}

/*package*/ void removeAllViewsWithSubviewClippingEnabled() {
Expand All @@ -693,7 +724,7 @@ public void run() {
* @return {@code true} if the view has been removed from the ViewGroup.
*/
private boolean isViewClipped(View view) {
return view.getParent() == null;
return view.getParent() == null || (mChildrenRemovedWhileTransitioning != null && mChildrenRemovedWhileTransitioning.contains(view.getId()));
}

private int indexOfChildInAllChildren(View child) {
Expand Down