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
wants to merge 14 commits into from
Closed

Fix handling removal of transitioning views #47634

Show file tree
Hide file tree
Changes from all 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
3 changes: 0 additions & 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,9 +62,6 @@ 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

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 {
Expand Down
30 changes: 29 additions & 1 deletion ...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 @@ -137,6 +137,7 @@ public void shutdown() {
private @Nullable ViewGroupDrawingOrderHelper mDrawingOrderHelper;
private float mBackfaceOpacity;
private String mBackfaceVisibility;
private @Nullable Set<Integer> mChildrenRemovedWhileTransitioning;

/**
* Creates a new `ReactViewGroup` instance.
Expand Down Expand Up @@ -170,6 +171,7 @@ private void initView() {
mDrawingOrderHelper = null;
mBackfaceOpacity = 1.f;
mBackfaceVisibility = "visible";
mChildrenRemovedWhileTransitioning = null;
}

/* package */ void recycleView() {
Expand Down Expand Up @@ -405,6 +407,25 @@ public void updateClippingRect() {
updateClippingToRect(mClippingRect);
}

@Override
public void endViewTransition(View view) {
super.endViewTransition(view);
if (mChildrenRemovedWhileTransitioning != null) {
mChildrenRemovedWhileTransitioning.remove(view.getId());
}
}

private Set<Integer> ensureChildrenRemovedWhileTransitioning() {
if (mChildrenRemovedWhileTransitioning == null) {
mChildrenRemovedWhileTransitioning = new HashSet<>();
}
return mChildrenRemovedWhileTransitioning;
}

private boolean isChildRemovedWhileTransitioning(View child) {
return mChildrenRemovedWhileTransitioning != null && mChildrenRemovedWhileTransitioning.contains(child.getId());
}

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

// The parent might not be null in case the child is transitioning.
if (child.getParent() != null) {
ensureChildrenRemovedWhileTransitioning().add(child.getId());
}

super.onViewRemoved(child);
}

Expand Down Expand Up @@ -712,7 +739,8 @@ public void run() {
*/
private boolean isViewClipped(View view) {
ViewParent parent = view.getParent();
if (parent == null) {

if (parent == null || isChildRemovedWhileTransitioning(view)) {
return true;
} else {
Assertions.assertCondition(parent == this);
Expand Down
Loading