Skip to content

Commit

Permalink
Do not destroy views when there is a touch going on for New Architect…
Browse files Browse the repository at this point in the history
…ure (#45865)

Summary:
Pull Request resolved: #45865

This diff introduces the logic to defer the destruction of ViewState (and EventEmitter) for views that are currently touched on by the user. The idea is to let the UIManager know which view is currently active from the `JSTouchDispatcher` and eventually defer the view deletion till the view is not interacted anymore.

The JSTouchDispatcher already retains the information on which tag was the touch originally fired.
We'll pass over that information to the UIManager/SurfaceMountingManager so that it can be accounted for when the view has to be deleted.

This is causing a couple of bad bugs on Android:

Fixes #45126
Fixes #44610
Closes #45675

Changelog:
[Android] [Fixed] - Do not destroy views when there is a touch going on for New Architecture

Reviewed By: mdvacca

Differential Revision: D60594878

fbshipit-source-id: c3334d16cf305e0178f50772576050ebfbba85ec
  • Loading branch information
cortinico authored and facebook-github-bot committed Aug 8, 2024
1 parent 03c0e5e commit 6b7f682
Show file tree
Hide file tree
Showing 10 changed files with 165 additions and 37 deletions.
9 changes: 9 additions & 0 deletions packages/react-native/ReactAndroid/api/ReactAndroid.api
Original file line number Diff line number Diff line change
Expand Up @@ -1569,6 +1569,7 @@ public abstract interface class com/facebook/react/bridge/UIManager : com/facebo
public abstract fun getEventDispatcher ()Ljava/lang/Object;
public abstract fun initialize ()V
public abstract fun invalidate ()V
public abstract fun markActiveTouchForTag (II)V
public abstract fun receiveEvent (IILjava/lang/String;Lcom/facebook/react/bridge/WritableMap;)V
public abstract fun receiveEvent (ILjava/lang/String;Lcom/facebook/react/bridge/WritableMap;)V
public abstract fun removeUIManagerEventListener (Lcom/facebook/react/bridge/UIManagerListener;)V
Expand All @@ -1577,6 +1578,7 @@ public abstract interface class com/facebook/react/bridge/UIManager : com/facebo
public abstract fun sendAccessibilityEvent (II)V
public abstract fun startSurface (Landroid/view/View;Ljava/lang/String;Lcom/facebook/react/bridge/WritableMap;II)I
public abstract fun stopSurface (I)V
public abstract fun sweepActiveTouchForTag (II)V
public abstract fun synchronouslyUpdateViewOnUIThread (ILcom/facebook/react/bridge/ReadableMap;)V
public abstract fun updateRootLayoutSpecs (IIIII)V
}
Expand Down Expand Up @@ -2710,6 +2712,7 @@ public class com/facebook/react/fabric/FabricUIManager : com/facebook/react/brid
public fun getThemeData (I[F)Z
public fun initialize ()V
public fun invalidate ()V
public fun markActiveTouchForTag (II)V
public fun onAllAnimationsComplete ()V
public fun onAnimationStarted ()V
public fun onHostDestroy ()V
Expand All @@ -2733,6 +2736,7 @@ public class com/facebook/react/fabric/FabricUIManager : com/facebook/react/brid
public fun startSurface (Lcom/facebook/react/interfaces/fabric/SurfaceHandler;Landroid/content/Context;Landroid/view/View;)V
public fun stopSurface (I)V
public fun stopSurface (Lcom/facebook/react/interfaces/fabric/SurfaceHandler;)V
public fun sweepActiveTouchForTag (II)V
public fun synchronouslyUpdateViewOnUIThread (ILcom/facebook/react/bridge/ReadableMap;)V
public fun updateRootLayoutSpecs (IIIII)V
}
Expand Down Expand Up @@ -2888,6 +2892,7 @@ public class com/facebook/react/fabric/mounting/SurfaceMountingManager {
public fun getViewExists (I)Z
public fun isRootViewAttached ()Z
public fun isStopped ()Z
public fun markActiveTouchForTag (I)V
public fun preallocateView (Ljava/lang/String;ILcom/facebook/react/bridge/ReadableMap;Lcom/facebook/react/uimanager/StateWrapper;Z)V
public fun printSurfaceState ()V
public fun receiveCommand (IILcom/facebook/react/bridge/ReadableArray;)V
Expand All @@ -2897,6 +2902,7 @@ public class com/facebook/react/fabric/mounting/SurfaceMountingManager {
public fun sendAccessibilityEvent (II)V
public fun setJSResponder (IIZ)V
public fun stopSurface ()V
public fun sweepActiveTouchForTag (I)V
public fun updateEventEmitter (ILcom/facebook/react/fabric/events/EventEmitterWrapper;)V
public fun updateLayout (IIIIIIII)V
public fun updateOverflowInset (IIIII)V
Expand Down Expand Up @@ -4302,6 +4308,7 @@ public class com/facebook/react/uimanager/JSPointerDispatcher {
public class com/facebook/react/uimanager/JSTouchDispatcher {
public fun <init> (Landroid/view/ViewGroup;)V
public fun handleTouchEvent (Landroid/view/MotionEvent;Lcom/facebook/react/uimanager/events/EventDispatcher;)V
public fun handleTouchEvent (Landroid/view/MotionEvent;Lcom/facebook/react/uimanager/events/EventDispatcher;Lcom/facebook/react/bridge/ReactContext;)V
public fun onChildEndedNativeGesture (Landroid/view/MotionEvent;Lcom/facebook/react/uimanager/events/EventDispatcher;)V
public fun onChildStartedNativeGesture (Landroid/view/MotionEvent;Lcom/facebook/react/uimanager/events/EventDispatcher;)V
}
Expand Down Expand Up @@ -5284,6 +5291,7 @@ public class com/facebook/react/uimanager/UIManagerModule : com/facebook/react/b
public fun invalidate ()V
public fun invalidateNodeLayout (I)V
public fun manageChildren (ILcom/facebook/react/bridge/ReadableArray;Lcom/facebook/react/bridge/ReadableArray;Lcom/facebook/react/bridge/ReadableArray;Lcom/facebook/react/bridge/ReadableArray;Lcom/facebook/react/bridge/ReadableArray;)V
public fun markActiveTouchForTag (II)V
public fun measure (ILcom/facebook/react/bridge/Callback;)V
public fun measureInWindow (ILcom/facebook/react/bridge/Callback;)V
public fun measureLayout (IILcom/facebook/react/bridge/Callback;Lcom/facebook/react/bridge/Callback;)V
Expand All @@ -5309,6 +5317,7 @@ public class com/facebook/react/uimanager/UIManagerModule : com/facebook/react/b
public fun setViewLocalData (ILjava/lang/Object;)V
public fun startSurface (Landroid/view/View;Ljava/lang/String;Lcom/facebook/react/bridge/WritableMap;II)I
public fun stopSurface (I)V
public fun sweepActiveTouchForTag (II)V
public fun synchronouslyUpdateViewOnUIThread (ILcom/facebook/react/bridge/ReadableMap;)V
public fun updateNodeSize (III)V
public fun updateRootLayoutSpecs (IIIII)V
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ protected void dispatchJSTouchEvent(MotionEvent event) {
EventDispatcher eventDispatcher =
UIManagerHelper.getEventDispatcher(getCurrentReactContext(), getUIManagerType());
if (eventDispatcher != null) {
mJSTouchDispatcher.handleTouchEvent(event, eventDispatcher);
mJSTouchDispatcher.handleTouchEvent(event, eventDispatcher, getCurrentReactContext());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,22 @@ public interface UIManager : PerformanceCounter {

/** Called before React Native instance is destroyed. */
public fun invalidate()

/**
* Mark a view as currently active for a touch event. This information could be used by the
* [UIManager] to decide if a view could be safely destroyed or not.
*
* @param surfaceId The surface ID where the view is rendered.
* @param reactTag The react tag for the specific view
*/
public fun markActiveTouchForTag(surfaceId: Int, reactTag: Int)

/**
* Sweep a view as currently not active for a touch event. This tells the [UIManager] that the
* view is not being interacted by the user and can safely be destroyed.
*
* @param surfaceId The surface ID where the view is rendered.
* @param reactTag The react tag for the specific view
*/
public fun sweepActiveTouchForTag(surfaceId: Int, reactTag: Int)
}
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,16 @@ public void invalidate() {
}
}

@Override
public void markActiveTouchForTag(int surfaceId, int reactTag) {
mMountingManager.getSurfaceManager(surfaceId).markActiveTouchForTag(reactTag);
}

@Override
public void sweepActiveTouchForTag(int surfaceId, int reactTag) {
mMountingManager.getSurfaceManager(surfaceId).sweepActiveTouchForTag(reactTag);
}

/**
* Method added to Fabric for backward compatibility reasons, as users on Paper could call
* [addUiBlock] and [prependUiBlock] on UIManagerModule.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,17 @@ public class SurfaceMountingManager {
@ThreadConfined(UI)
private final Set<Integer> mErroneouslyReaddedReactTags = new HashSet<>();

// This set is used to keep track of views that are currently being interacted with (i.e.
// views that saw a ACTION_DOWN but not a ACTION_UP event yet). This is used to prevent
// views from being removed while they are being interacted with as their event emitter will
// also be removed, and `Pressables` will look "stuck".
@ThreadConfined(UI)
private final Set<Integer> mViewsWithActiveTouches = new HashSet<>();

// This set contains the views that are scheduled to be removed after their touch finishes.
@ThreadConfined(UI)
private final Set<Integer> mViewsToDeleteAfterTouchFinishes = new HashSet<>();

// This is null *until* StopSurface is called.
private SparseArrayCompat<Object> mTagSetForStoppedSurface;

Expand Down Expand Up @@ -1031,12 +1042,21 @@ public void deleteView(int reactTag) {
return;
}

// To delete we simply remove the tag from the registry.
// We want to rely on the correct set of MountInstructions being sent to the platform,
// or StopSurface being called, so we do not handle deleting descendents of the View.
mTagToViewState.remove(reactTag);
if (mViewsWithActiveTouches.contains(reactTag)) {
// If the view that went offscreen is still being touched, we can't delete it yet.
// We have to delay the deletion till the touch is completed.
// This is causing bugs like those otherwise:
// - https://github.com/facebook/react-native/issues/44610
// - https://github.com/facebook/react-native/issues/45126
mViewsToDeleteAfterTouchFinishes.add(reactTag);
} else {
// To delete we simply remove the tag from the registry.
// We want to rely on the correct set of MountInstructions being sent to the platform,
// or StopSurface being called, so we do not handle deleting descendants of the View.
mTagToViewState.remove(reactTag);

onViewStateDeleted(viewState);
onViewStateDeleted(viewState);
}
}

@UiThread
Expand Down Expand Up @@ -1160,6 +1180,18 @@ public void run() {
});
}

public void markActiveTouchForTag(int reactTag) {
mViewsWithActiveTouches.add(reactTag);
}

public void sweepActiveTouchForTag(int reactTag) {
mViewsWithActiveTouches.remove(reactTag);
if (mViewsToDeleteAfterTouchFinishes.contains(reactTag)) {
mViewsToDeleteAfterTouchFinishes.remove(reactTag);
deleteView(reactTag);
}
}

/**
* This class holds view state for react tags. Objects of this class are stored into the {@link
* #mTagToViewState}, and they should be updated in the same thread.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ public class ReactSurfaceView(context: Context?, private val surface: ReactSurfa
override fun dispatchJSTouchEvent(event: MotionEvent) {
val eventDispatcher = surface.eventDispatcher
if (eventDispatcher != null) {
jsTouchDispatcher.handleTouchEvent(event, eventDispatcher)
jsTouchDispatcher.handleTouchEvent(
event, eventDispatcher, surface.reactHost.currentReactContext)
} else {
FLog.w(
TAG, "Unable to dispatch touch events to JS as the React instance has not been attached")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@

import android.view.MotionEvent;
import android.view.ViewGroup;
import androidx.annotation.Nullable;
import com.facebook.common.logging.FLog;
import com.facebook.infer.annotation.Assertions;
import com.facebook.infer.annotation.Nullsafe;
import com.facebook.react.bridge.ReactContext;
import com.facebook.react.bridge.UIManager;
import com.facebook.react.common.ReactConstants;
import com.facebook.react.uimanager.common.UIManagerType;
import com.facebook.react.uimanager.events.EventDispatcher;
import com.facebook.react.uimanager.events.TouchEvent;
import com.facebook.react.uimanager.events.TouchEventCoalescingKeyHelper;
Expand All @@ -30,12 +34,14 @@ public class JSTouchDispatcher {
private final float[] mTargetCoordinates = new float[2];
private boolean mChildIsHandlingNativeGesture = false;
private long mGestureStartTime = TouchEvent.UNSET;
private final ViewGroup mRootViewGroup;

private final ViewGroup mViewGroup;

private final TouchEventCoalescingKeyHelper mTouchEventCoalescingKeyHelper =
new TouchEventCoalescingKeyHelper();

public JSTouchDispatcher(ViewGroup viewGroup) {
mRootViewGroup = viewGroup;
mViewGroup = viewGroup;
}

public void onChildStartedNativeGesture(
Expand All @@ -57,6 +63,10 @@ public void onChildEndedNativeGesture(MotionEvent androidEvent, EventDispatcher
mChildIsHandlingNativeGesture = false;
}

public void handleTouchEvent(MotionEvent ev, EventDispatcher eventDispatcher) {
handleTouchEvent(ev, eventDispatcher, null);
}

/**
* Main catalyst view is responsible for collecting and sending touch events to JS. This method
* reacts for an incoming android native touch events ({@link MotionEvent}) and calls into {@link
Expand All @@ -65,7 +75,8 @@ public void onChildEndedNativeGesture(MotionEvent androidEvent, EventDispatcher
* method for figuring out a react view ID in the case of ACTION_DOWN event (when the gesture
* starts).
*/
public void handleTouchEvent(MotionEvent ev, EventDispatcher eventDispatcher) {
public void handleTouchEvent(
MotionEvent ev, EventDispatcher eventDispatcher, @Nullable ReactContext reactContext) {
int action = ev.getAction() & MotionEvent.ACTION_MASK;
if (action == MotionEvent.ACTION_DOWN) {
if (mTargetTag != -1) {
Expand All @@ -78,11 +89,13 @@ public void handleTouchEvent(MotionEvent ev, EventDispatcher eventDispatcher) {
// this gesture
mChildIsHandlingNativeGesture = false;
mGestureStartTime = ev.getEventTime();

mTargetTag = findTargetTagAndSetCoordinates(ev);
int surfaceId = UIManagerHelper.getSurfaceId(mViewGroup);
markActiveTouchForTag(surfaceId, mTargetTag, reactContext);

eventDispatcher.dispatchEvent(
TouchEvent.obtain(
UIManagerHelper.getSurfaceId(mRootViewGroup),
UIManagerHelper.getSurfaceId(mViewGroup),
mTargetTag,
TouchEventType.START,
ev,
Expand All @@ -105,24 +118,26 @@ public void handleTouchEvent(MotionEvent ev, EventDispatcher eventDispatcher) {
// End of the gesture. We reset target tag to -1 and expect no further event associated with
// this gesture.
findTargetTagAndSetCoordinates(ev);
int surfaceId = UIManagerHelper.getSurfaceId(mViewGroup);
eventDispatcher.dispatchEvent(
TouchEvent.obtain(
UIManagerHelper.getSurfaceId(mRootViewGroup),
surfaceId,
mTargetTag,
TouchEventType.END,
ev,
mGestureStartTime,
mTargetCoordinates[0],
mTargetCoordinates[1],
mTouchEventCoalescingKeyHelper));
sweepActiveTouchForTag(surfaceId, mTargetTag, reactContext);
mTargetTag = -1;
mGestureStartTime = TouchEvent.UNSET;
} else if (action == MotionEvent.ACTION_MOVE) {
// Update pointer position for current gesture
findTargetTagAndSetCoordinates(ev);
eventDispatcher.dispatchEvent(
TouchEvent.obtain(
UIManagerHelper.getSurfaceId(mRootViewGroup),
UIManagerHelper.getSurfaceId(mViewGroup),
mTargetTag,
TouchEventType.MOVE,
ev,
Expand All @@ -134,7 +149,7 @@ public void handleTouchEvent(MotionEvent ev, EventDispatcher eventDispatcher) {
// New pointer goes down, this can only happen after ACTION_DOWN is sent for the first pointer
eventDispatcher.dispatchEvent(
TouchEvent.obtain(
UIManagerHelper.getSurfaceId(mRootViewGroup),
UIManagerHelper.getSurfaceId(mViewGroup),
mTargetTag,
TouchEventType.START,
ev,
Expand All @@ -146,7 +161,7 @@ public void handleTouchEvent(MotionEvent ev, EventDispatcher eventDispatcher) {
// Exactly one of the pointers goes up
eventDispatcher.dispatchEvent(
TouchEvent.obtain(
UIManagerHelper.getSurfaceId(mRootViewGroup),
UIManagerHelper.getSurfaceId(mViewGroup),
mTargetTag,
TouchEventType.END,
ev,
Expand All @@ -162,6 +177,9 @@ public void handleTouchEvent(MotionEvent ev, EventDispatcher eventDispatcher) {
ReactConstants.TAG,
"Received an ACTION_CANCEL touch event for which we have no corresponding ACTION_DOWN");
}
int surfaceId = UIManagerHelper.getSurfaceId(mViewGroup);
sweepActiveTouchForTag(surfaceId, mTargetTag, reactContext);

mTargetTag = -1;
mGestureStartTime = TouchEvent.UNSET;
} else {
Expand All @@ -171,10 +189,32 @@ public void handleTouchEvent(MotionEvent ev, EventDispatcher eventDispatcher) {
}
}

private void markActiveTouchForTag(
int surfaceId, int reactTag, @Nullable ReactContext reactContext) {
if (reactContext == null) {
return;
}
UIManager uiManager = UIManagerHelper.getUIManager(reactContext, UIManagerType.FABRIC);
if (uiManager != null) {
uiManager.markActiveTouchForTag(surfaceId, reactTag);
}
}

private void sweepActiveTouchForTag(
int surfaceId, int reactTag, @Nullable ReactContext reactContext) {
if (reactContext == null) {
return;
}
UIManager uiManager = UIManagerHelper.getUIManager(reactContext, UIManagerType.FABRIC);
if (uiManager != null) {
uiManager.sweepActiveTouchForTag(surfaceId, reactTag);
}
}

private int findTargetTagAndSetCoordinates(MotionEvent ev) {
// This method updates `mTargetCoordinates` with coordinates for the motion event.
return TouchTargetHelper.findTargetTagAndCoordinatesForTouch(
ev.getX(), ev.getY(), mRootViewGroup, mTargetCoordinates, null);
ev.getX(), ev.getY(), mViewGroup, mTargetCoordinates, null);
}

private void dispatchCancelEvent(MotionEvent androidEvent, EventDispatcher eventDispatcher) {
Expand All @@ -195,7 +235,7 @@ private void dispatchCancelEvent(MotionEvent androidEvent, EventDispatcher event
Assertions.assertNotNull(eventDispatcher)
.dispatchEvent(
TouchEvent.obtain(
UIManagerHelper.getSurfaceId(mRootViewGroup),
UIManagerHelper.getSurfaceId(mViewGroup),
mTargetTag,
TouchEventType.CANCEL,
androidEvent,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,16 @@ public void invalidate() {
ViewManagerPropertyUpdater.clear();
}

@Override
public void markActiveTouchForTag(int surfaceId, int reactTag) {
// Not implemented for Paper.
}

@Override
public void sweepActiveTouchForTag(int surfaceId, int reactTag) {
// Not implemented for Paper.
}

/**
* This method is intended to reuse the {@link ViewManagerRegistry} with FabricUIManager. Do not
* use this method as this will be removed in the near future.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ public class ReactModalHostView(context: ThemedReactContext) :

override fun onInterceptTouchEvent(event: MotionEvent): Boolean {
eventDispatcher?.let { eventDispatcher ->
jSTouchDispatcher.handleTouchEvent(event, eventDispatcher)
jSTouchDispatcher.handleTouchEvent(event, eventDispatcher, reactContext)
jSPointerDispatcher?.handleMotionEvent(event, eventDispatcher, true)
}
return super.onInterceptTouchEvent(event)
Expand All @@ -492,7 +492,7 @@ public class ReactModalHostView(context: ThemedReactContext) :
@SuppressLint("ClickableViewAccessibility")
override fun onTouchEvent(event: MotionEvent): Boolean {
eventDispatcher?.let { eventDispatcher ->
jSTouchDispatcher.handleTouchEvent(event, eventDispatcher)
jSTouchDispatcher.handleTouchEvent(event, eventDispatcher, reactContext)
jSPointerDispatcher?.handleMotionEvent(event, eventDispatcher, false)
}
super.onTouchEvent(event)
Expand Down
Loading

0 comments on commit 6b7f682

Please sign in to comment.