Unbreak Android LayoutAnimation by making all surfaces go through Bin…

…ding (#46691) Summary: Pull Request resolved: #46691 In Bridgeless, ReactSurface would only call `registerSurface` when it started, which did not match the behaviour seen in startSurface(WithConstraints). Instead, make all calls to start the surface go through the FabricUIManager Binding so we can correctly configure `setMountingOverrideDelegate` which is required for layout animations. Changelog: [Android][FIxed] LayoutAnimations work on full new architecture Reviewed By: cortinico Differential Revision: D63533635 fbshipit-source-id: f6d3db020bb2d7245f7b14f2407271d76837d40c
facebook · Sep 27, 2024 · 43af902 · 43af902
1 parent 459fadc
commit 43af902
Show file tree

Hide file tree

Showing 11 changed files with 85 additions and 100 deletions.
diff --git a/packages/react-native/ReactAndroid/api/ReactAndroid.api b/packages/react-native/ReactAndroid/api/ReactAndroid.api
@@ -2583,15 +2583,15 @@ public abstract interface class com/facebook/react/fabric/Binding {
 	public abstract fun driveCxxAnimations ()V
 	public abstract fun getInspectorDataForInstance (Lcom/facebook/react/fabric/events/EventEmitterWrapper;)Lcom/facebook/react/bridge/ReadableNativeMap;
 	public abstract fun register (Lcom/facebook/react/bridge/RuntimeExecutor;Lcom/facebook/react/bridge/RuntimeScheduler;Lcom/facebook/react/fabric/FabricUIManager;Lcom/facebook/react/fabric/events/EventBeatManager;Lcom/facebook/react/fabric/ComponentFactory;Lcom/facebook/react/fabric/ReactNativeConfig;)V
-	public abstract fun registerSurface (Lcom/facebook/react/fabric/SurfaceHandlerBinding;)V
 	public abstract fun reportMount (I)V
 	public abstract fun setConstraints (IFFFFFFZZ)V
 	public abstract fun setPixelDensity (F)V
 	public abstract fun startSurface (ILjava/lang/String;Lcom/facebook/react/bridge/NativeMap;)V
 	public abstract fun startSurfaceWithConstraints (ILjava/lang/String;Lcom/facebook/react/bridge/NativeMap;FFFFFFZZ)V
+	public abstract fun startSurfaceWithSurfaceHandler (Lcom/facebook/react/fabric/SurfaceHandlerBinding;)V
 	public abstract fun stopSurface (I)V
+	public abstract fun stopSurfaceWithSurfaceHandler (Lcom/facebook/react/fabric/SurfaceHandlerBinding;)V
 	public abstract fun unregister ()V
-	public abstract fun unregisterSurface (Lcom/facebook/react/fabric/SurfaceHandlerBinding;)V
 }
 
 public final class com/facebook/react/fabric/BindingImpl : com/facebook/react/fabric/Binding {
@@ -2600,15 +2600,15 @@ public final class com/facebook/react/fabric/BindingImpl : com/facebook/react/fa
 	public fun driveCxxAnimations ()V
 	public fun getInspectorDataForInstance (Lcom/facebook/react/fabric/events/EventEmitterWrapper;)Lcom/facebook/react/bridge/ReadableNativeMap;
 	public fun register (Lcom/facebook/react/bridge/RuntimeExecutor;Lcom/facebook/react/bridge/RuntimeScheduler;Lcom/facebook/react/fabric/FabricUIManager;Lcom/facebook/react/fabric/events/EventBeatManager;Lcom/facebook/react/fabric/ComponentFactory;Lcom/facebook/react/fabric/ReactNativeConfig;)V
-	public fun registerSurface (Lcom/facebook/react/fabric/SurfaceHandlerBinding;)V
 	public fun reportMount (I)V
 	public fun setConstraints (IFFFFFFZZ)V
 	public fun setPixelDensity (F)V
 	public fun startSurface (ILjava/lang/String;Lcom/facebook/react/bridge/NativeMap;)V
 	public fun startSurfaceWithConstraints (ILjava/lang/String;Lcom/facebook/react/bridge/NativeMap;FFFFFFZZ)V
+	public fun startSurfaceWithSurfaceHandler (Lcom/facebook/react/fabric/SurfaceHandlerBinding;)V
 	public fun stopSurface (I)V
+	public fun stopSurfaceWithSurfaceHandler (Lcom/facebook/react/fabric/SurfaceHandlerBinding;)V
 	public fun unregister ()V
-	public fun unregisterSurface (Lcom/facebook/react/fabric/SurfaceHandlerBinding;)V
 }
 
 public final class com/facebook/react/fabric/ComponentFactory {
@@ -2771,8 +2771,6 @@ public class com/facebook/react/fabric/SurfaceHandlerBinding : com/facebook/reac
 	public fun setMountable (Z)V
 	public fun setProps (Lcom/facebook/react/bridge/NativeMap;)V
 	public fun setSurfaceId (I)V
-	public fun start ()V
-	public fun stop ()V
 }
 
 public abstract interface annotation class com/facebook/react/fabric/SurfaceHandlerBinding$DisplayModeTypeDef : java/lang/annotation/Annotation {
@@ -2966,8 +2964,6 @@ public abstract interface class com/facebook/react/interfaces/fabric/SurfaceHand
 	public abstract fun setMountable (Z)V
 	public abstract fun setProps (Lcom/facebook/react/bridge/NativeMap;)V
 	public abstract fun setSurfaceId (I)V
-	public abstract fun start ()V
-	public abstract fun stop ()V
 }
 
 public final class com/facebook/react/jscexecutor/JSCExecutor : com/facebook/react/bridge/JavaScriptExecutor {

diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/Binding.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/Binding.kt
@@ -31,8 +31,12 @@ public interface Binding {
       doLeftAndRightSwapInRTL: Boolean
   )
 
+  public fun startSurfaceWithSurfaceHandler(surfaceHandler: SurfaceHandlerBinding)
+
   public fun stopSurface(surfaceId: Int)
 
+  public fun stopSurfaceWithSurfaceHandler(surfaceHandler: SurfaceHandlerBinding)
+
   public fun setPixelDensity(pointScaleFactor: Float)
 
   public fun setConstraints(
@@ -67,8 +71,4 @@ public interface Binding {
   )
 
   public fun unregister()
-
-  public fun registerSurface(surfaceHandler: SurfaceHandlerBinding)
-
-  public fun unregisterSurface(surfaceHandler: SurfaceHandlerBinding)
 }
diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/BindingImpl.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/BindingImpl.kt
@@ -49,8 +49,12 @@ public class BindingImpl : Binding {
       doLeftAndRightSwapInRTL: Boolean
   )
 
+  external override fun startSurfaceWithSurfaceHandler(surfaceHandler: SurfaceHandlerBinding)
+
   external override fun stopSurface(surfaceId: Int)
 
+  external override fun stopSurfaceWithSurfaceHandler(surfaceHandler: SurfaceHandlerBinding)
+
   external override fun setPixelDensity(pointScaleFactor: Float)
 
   external override fun setConstraints(
@@ -100,10 +104,6 @@ public class BindingImpl : Binding {
     uninstallFabricUIManager()
   }
 
-  external override fun registerSurface(surfaceHandler: SurfaceHandlerBinding)
-
-  external override fun unregisterSurface(surfaceHandler: SurfaceHandlerBinding)
-
   private companion object {
     init {
       FabricSoLoader.staticInit()

diff --git a/...es/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/...es/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java
@@ -329,11 +329,11 @@ public void startSurface(
     mMountingManager.startSurface(rootTag, reactContext, rootView);
 
     surfaceHandler.setSurfaceId(rootTag);
-    if (surfaceHandler instanceof SurfaceHandlerBinding) {
-      mBinding.registerSurface((SurfaceHandlerBinding) surfaceHandler);
-    }
     surfaceHandler.setMountable(rootView != null);
-    surfaceHandler.start();
+    if (!(surfaceHandler instanceof SurfaceHandlerBinding)) {
+      throw new IllegalArgumentException("Invalid SurfaceHandler");
+    }
+    mBinding.startSurfaceWithSurfaceHandler((SurfaceHandlerBinding) surfaceHandler);
   }
 
   public void attachRootView(final SurfaceHandler surfaceHandler, final View rootView) {
@@ -357,12 +357,10 @@ public void stopSurface(final SurfaceHandler surfaceHandler) {
     }
 
     mMountingManager.stopSurface(surfaceHandler.getSurfaceId());
-
-    surfaceHandler.stop();
-
-    if (surfaceHandler instanceof SurfaceHandlerBinding) {
-      mBinding.unregisterSurface((SurfaceHandlerBinding) surfaceHandler);
+    if (!(surfaceHandler instanceof SurfaceHandlerBinding)) {
+      throw new IllegalArgumentException("Invalid SurfaceHandler");
     }
+    mBinding.stopSurfaceWithSurfaceHandler((SurfaceHandlerBinding) surfaceHandler);
   }
 
   /** Method called when an event has been dispatched on the C++ side. */

diff --git a/...ct-native/ReactAndroid/src/main/java/com/facebook/react/fabric/SurfaceHandlerBinding.java b/...ct-native/ReactAndroid/src/main/java/com/facebook/react/fabric/SurfaceHandlerBinding.java
@@ -65,20 +65,6 @@ public String getModuleName() {
 
   private native String getModuleNameNative();
 
-  @Override
-  public void start() {
-    startNative();
-  }
-
-  private native void startNative();
-
-  @Override
-  public void stop() {
-    stopNative();
-  }
-
-  private native void stopNative();
-
   @Override
   public boolean isRunning() {
     return isRunningNative();

diff --git a/...-native/ReactAndroid/src/main/java/com/facebook/react/interfaces/fabric/SurfaceHandler.kt b/...-native/ReactAndroid/src/main/java/com/facebook/react/interfaces/fabric/SurfaceHandler.kt
@@ -24,12 +24,6 @@ public interface SurfaceHandler {
 
   public val moduleName: String
 
-  /** Starts the surface if the surface is not running */
-  public fun start()
-
-  /** Stops the surface if it is currently running */
-  public fun stop()
-
   public fun setProps(props: NativeMap)
 
   /**

diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.cpp
@@ -140,12 +140,58 @@ void Binding::reportMount(SurfaceId surfaceId) {
 
 #pragma mark - Surface management
 
+// Used by bridgeless
+void Binding::startSurfaceWithSurfaceHandler(
+    jni::alias_ref<SurfaceHandlerBinding::jhybridobject>
+        surfaceHandlerBinding) {
+  SystraceSection s("FabricUIManagerBinding::startSurfaceWithSurfaceHandler");
+
+  const auto& surfaceHandler =
+      surfaceHandlerBinding->cthis()->getSurfaceHandler();
+  if (enableFabricLogs_) {
+    LOG(WARNING)
+        << "Binding::startSurfaceWithSurfaceHandler() was called (address: "
+        << this << ", surfaceId: " << surfaceHandler.getSurfaceId() << ").";
+  }
+
+  auto scheduler = getScheduler();
+  if (!scheduler) {
+    LOG(ERROR)
+        << "Binding::startSurfaceWithSurfaceHandler: scheduler disappeared";
+    return;
+  }
+  scheduler->registerSurface(surfaceHandler);
+
+  surfaceHandler.start();
+
+  surfaceHandler.getMountingCoordinator()->setMountingOverrideDelegate(
+      animationDriver_);
+
+  {
+    std::unique_lock lock(surfaceHandlerRegistryMutex_);
+    surfaceHandlerRegistry_.emplace(
+        surfaceHandler.getSurfaceId(), jni::make_weak(surfaceHandlerBinding));
+  }
+
+  auto mountingManager = getMountingManager("startSurfaceWithSurfaceHandler");
+  if (!mountingManager) {
+    return;
+  }
+  mountingManager->onSurfaceStart(surfaceHandler.getSurfaceId());
+}
+
+// Used by non-bridgeless+Fabric
 void Binding::startSurface(
     jint surfaceId,
     jni::alias_ref<jstring> moduleName,
     NativeMap* initialProps) {
   SystraceSection s("FabricUIManagerBinding::startSurface");
 
+  if (enableFabricLogs_) {
+    LOG(WARNING) << "Binding::startSurface() was called (address: " << this
+                 << ", surfaceId: " << surfaceId << ").";
+  }
+
   auto scheduler = getScheduler();
   if (!scheduler) {
     LOG(ERROR) << "Binding::startSurface: scheduler disappeared";
@@ -181,6 +227,7 @@ void Binding::startSurface(
   mountingManager->onSurfaceStart(surfaceId);
 }
 
+// Used by non-bridgeless+Fabric
 void Binding::startSurfaceWithConstraints(
     jint surfaceId,
     jni::alias_ref<jstring> moduleName,
@@ -251,9 +298,9 @@ void Binding::startSurfaceWithConstraints(
   mountingManager->onSurfaceStart(surfaceId);
 }
 
+// Used by non-bridgeless+Fabric
 void Binding::stopSurface(jint surfaceId) {
   SystraceSection s("FabricUIManagerBinding::stopSurface");
-
   if (enableFabricLogs_) {
     LOG(WARNING) << "Binding::stopSurface() was called (address: " << this
                  << ", surfaceId: " << surfaceId << ").";
@@ -267,7 +314,6 @@ void Binding::stopSurface(jint surfaceId) {
 
   {
     std::unique_lock lock(surfaceHandlerRegistryMutex_);
-
     auto iterator = surfaceHandlerRegistry_.find(surfaceId);
     if (iterator == surfaceHandlerRegistry_.end()) {
       LOG(ERROR) << "Binding::stopSurface: Surface with given id is not found";
@@ -291,37 +337,21 @@ void Binding::stopSurface(jint surfaceId) {
   mountingManager->onSurfaceStop(surfaceId);
 }
 
-void Binding::registerSurface(
+// Used by bridgeless
+void Binding::stopSurfaceWithSurfaceHandler(
     jni::alias_ref<SurfaceHandlerBinding::jhybridobject>
         surfaceHandlerBinding) {
+  SystraceSection s("FabricUIManagerBinding::stopSurfaceWithSurfaceHandler");
   const auto& surfaceHandler =
       surfaceHandlerBinding->cthis()->getSurfaceHandler();
-
-  auto scheduler = getScheduler();
-  if (!scheduler) {
-    LOG(ERROR) << "Binding::registerSurface: scheduler disappeared";
-    return;
-  }
-  scheduler->registerSurface(surfaceHandler);
-
-  {
-    std::unique_lock lock(surfaceHandlerRegistryMutex_);
-    surfaceHandlerRegistry_.emplace(
-        surfaceHandler.getSurfaceId(), jni::make_weak(surfaceHandlerBinding));
+  if (enableFabricLogs_) {
+    LOG(WARNING)
+        << "Binding::stopSurfaceWithSurfaceHandler() was called (address: "
+        << this << ", surfaceId: " << surfaceHandler.getSurfaceId() << ").";
   }
 
-  auto mountingManager = getMountingManager("registerSurface");
-  if (!mountingManager) {
-    return;
-  }
-  mountingManager->onSurfaceStart(surfaceHandler.getSurfaceId());
-}
+  surfaceHandler.stop();
 
-void Binding::unregisterSurface(
-    jni::alias_ref<SurfaceHandlerBinding::jhybridobject>
-        surfaceHandlerBinding) {
-  const auto& surfaceHandler =
-      surfaceHandlerBinding->cthis()->getSurfaceHandler();
   auto scheduler = getScheduler();
   if (!scheduler) {
     LOG(ERROR) << "Binding::unregisterSurface: scheduler disappeared";
@@ -377,7 +407,6 @@ void Binding::setConstraints(
 
   {
     std::shared_lock lock(surfaceHandlerRegistryMutex_);
-
     auto iterator = surfaceHandlerRegistry_.find(surfaceId);
     if (iterator == surfaceHandlerRegistry_.end()) {
       LOG(ERROR)
@@ -650,8 +679,12 @@ void Binding::registerNatives() {
       makeNativeMethod("reportMount", Binding::reportMount),
       makeNativeMethod(
           "uninstallFabricUIManager", Binding::uninstallFabricUIManager),
-      makeNativeMethod("registerSurface", Binding::registerSurface),
-      makeNativeMethod("unregisterSurface", Binding::unregisterSurface),
+      makeNativeMethod(
+          "startSurfaceWithSurfaceHandler",
+          Binding::startSurfaceWithSurfaceHandler),
+      makeNativeMethod(
+          "stopSurfaceWithSurfaceHandler",
+          Binding::stopSurfaceWithSurfaceHandler),
   });
 }
 

diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.h b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.h
@@ -95,10 +95,10 @@ class Binding : public jni::HybridClass<Binding, JBinding>,
 
   void stopSurface(jint surfaceId);
 
-  void registerSurface(
+  void startSurfaceWithSurfaceHandler(
       jni::alias_ref<SurfaceHandlerBinding::jhybridobject> surfaceHandler);
 
-  void unregisterSurface(
+  void stopSurfaceWithSurfaceHandler(
       jni::alias_ref<SurfaceHandlerBinding::jhybridobject> surfaceHandler);
 
   void schedulerDidFinishTransaction(

diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/SurfaceHandlerBinding.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/SurfaceHandlerBinding.cpp
@@ -19,22 +19,6 @@ void SurfaceHandlerBinding::setDisplayMode(jint mode) {
   surfaceHandler_.setDisplayMode(static_cast<DisplayMode>(mode));
 }
 
-void SurfaceHandlerBinding::start() {
-  std::unique_lock lock(lifecycleMutex_);
-
-  if (surfaceHandler_.getStatus() != SurfaceHandler::Status::Running) {
-    surfaceHandler_.start();
-  }
-}
-
-void SurfaceHandlerBinding::stop() {
-  std::unique_lock lock(lifecycleMutex_);
-
-  if (surfaceHandler_.getStatus() == SurfaceHandler::Status::Running) {
-    surfaceHandler_.stop();
-  }
-}
-
 jint SurfaceHandlerBinding::getSurfaceId() {
   return surfaceHandler_.getSurfaceId();
 }
@@ -101,8 +85,6 @@ void SurfaceHandlerBinding::registerNatives() {
       makeNativeMethod("isRunningNative", SurfaceHandlerBinding::isRunning),
       makeNativeMethod(
           "getModuleNameNative", SurfaceHandlerBinding::getModuleName),
-      makeNativeMethod("startNative", SurfaceHandlerBinding::start),
-      makeNativeMethod("stopNative", SurfaceHandlerBinding::stop),
       makeNativeMethod(
           "setLayoutConstraintsNative",
           SurfaceHandlerBinding::setLayoutConstraints),

diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/SurfaceHandlerBinding.h b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/SurfaceHandlerBinding.h
@@ -24,9 +24,6 @@ class SurfaceHandlerBinding : public jni::HybridClass<SurfaceHandlerBinding> {
 
   SurfaceHandlerBinding(SurfaceId surfaceId, const std::string& moduleName);
 
-  void start();
-  void stop();
-
   void setDisplayMode(jint mode);
 
   jint getSurfaceId();
@@ -51,7 +48,6 @@ class SurfaceHandlerBinding : public jni::HybridClass<SurfaceHandlerBinding> {
   const SurfaceHandler& getSurfaceHandler();
 
  private:
-  mutable std::shared_mutex lifecycleMutex_;
   const SurfaceHandler surfaceHandler_;
 
   jni::alias_ref<SurfaceHandlerBinding::jhybriddata> jhybridobject_;

diff --git a/...es/react-native/ReactAndroid/src/test/java/com/facebook/react/runtime/ReactSurfaceTest.kt b/...es/react-native/ReactAndroid/src/test/java/com/facebook/react/runtime/ReactSurfaceTest.kt
@@ -174,11 +174,11 @@ class ReactSurfaceTest {
     var heightMeasureSpec = 0
     var widthMeasureSpec = 0
 
-    override fun start() {
+    fun start() {
       isRunning = true
     }
 
-    override fun stop() {
+    fun stop() {
       isRunning = false
     }