From dae2f606c76905de74e76db7b0a20052a5caea46 Mon Sep 17 00:00:00 2001 From: Zeya Peng Date: Fri, 5 Sep 2025 09:18:50 -0700 Subject: [PATCH] Course correct props at SurfaceMountingManager.updateProps() (#53589) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/53589 ## Changelog: [Android] [Changed] - [c++ animated] Course correct props at SurfaceMountingManager.updateProps() Sometimes a React update will try to commit to the same view that native animated modified before via direct manipulation, and after the update host view will use the prop value currently in Fabric. In `AnimatedMountingOverrideDelegate` there's logic to course correct at ShadowTree mount, but if this update is from JS thread, it takes some time to reach mounting layer, at the same time UI thread can still be doing more direct animation updates, and once the corrected change gets there it's already stale. In this diff I added mechanism to keep track of direct manipulation props (or "synchronous mount props" to match the naming of java function `synchronouslyUpdateView...`) and use it to correct what reaches host view. `SurfaceMountingManager.updateProps()` is called by both regular mount and direct manipulation and it's always called on UI thread, so it could be a good candidate to synchronize these 2 scenarios Reviewed By: sammy-SC Differential Revision: D81611823 fbshipit-source-id: 638a59bcd94b3d7e8bab68defd472b2b482dc92f --- .../ReactAndroid/api/ReactAndroid.api | 2 + .../react/fabric/FabricUIManager.java | 3 +- .../react/fabric/mounting/MountingManager.kt | 14 ++- .../mounting/SurfaceMountingManager.java | 116 +++++++++++++++++- .../animated/NativeAnimatedNodesManager.cpp | 20 +-- 5 files changed, 144 insertions(+), 11 deletions(-) diff --git a/packages/react-native/ReactAndroid/api/ReactAndroid.api b/packages/react-native/ReactAndroid/api/ReactAndroid.api index 8436cf85407..fb71ef2fee4 100644 --- a/packages/react-native/ReactAndroid/api/ReactAndroid.api +++ b/packages/react-native/ReactAndroid/api/ReactAndroid.api @@ -2354,12 +2354,14 @@ public class com/facebook/react/fabric/mounting/SurfaceMountingManager { public fun sendAccessibilityEvent (II)V public fun setJSResponder (IIZ)V public fun stopSurface ()V + public fun storeSynchronousMountPropsOverride (ILcom/facebook/react/bridge/ReadableMap;)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 public fun updatePadding (IIIII)V public fun updateProps (ILcom/facebook/react/bridge/ReadableMap;)V + public fun updatePropsSynchronously (ILcom/facebook/react/bridge/ReadableMap;)V public fun updateState (ILcom/facebook/react/uimanager/StateWrapper;)V } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index e08055776a3..f601b0757df 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -791,7 +791,8 @@ public class FabricUIManager @Override public void execute(MountingManager mountingManager) { try { - mountingManager.updateProps(reactTag, props); + mountingManager.storeSynchronousMountPropsOverride(reactTag, props); + mountingManager.updatePropsSynchronously(reactTag, props); } catch (Exception ex) { // TODO T42943890: Fix animations in Fabric and remove this try/catch? // There might always be race conditions between surface teardown and diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.kt index ad8d5132433..be94133ddd5 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.kt @@ -265,13 +265,23 @@ internal class MountingManager( } @UiThread - fun updateProps(reactTag: Int, props: ReadableMap?) { + fun storeSynchronousMountPropsOverride(reactTag: Int, props: ReadableMap?) { assertOnUiThread() if (props == null) { return } - getSurfaceManagerForViewEnforced(reactTag).updateProps(reactTag, props) + getSurfaceManagerForViewEnforced(reactTag).storeSynchronousMountPropsOverride(reactTag, props) + } + + @UiThread + fun updatePropsSynchronously(reactTag: Int, props: ReadableMap?) { + assertOnUiThread() + if (props == null) { + return + } + + getSurfaceManagerForViewEnforced(reactTag).updatePropsSynchronously(reactTag, props) } /** diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java index 705ad0411a0..a1a8a0753c5 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java @@ -27,10 +27,14 @@ import com.facebook.react.bridge.ReactNoCrashSoftException; import com.facebook.react.bridge.ReactSoftExceptionLogger; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; +import com.facebook.react.bridge.ReadableType; import com.facebook.react.bridge.RetryableMountingLayerException; import com.facebook.react.bridge.SoftAssertions; import com.facebook.react.bridge.UiThreadUtil; +import com.facebook.react.bridge.WritableArray; import com.facebook.react.bridge.WritableMap; +import com.facebook.react.bridge.WritableNativeArray; +import com.facebook.react.bridge.WritableNativeMap; import com.facebook.react.common.annotations.UnstableReactNativeAPI; import com.facebook.react.common.build.ReactBuildConfig; import com.facebook.react.common.mapbuffer.MapBuffer; @@ -53,7 +57,10 @@ import com.facebook.react.uimanager.ViewManagerRegistry; import com.facebook.react.uimanager.events.EventCategoryDef; import com.facebook.systrace.Systrace; import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.LinkedList; import java.util.Map; import java.util.Queue; @@ -96,6 +103,11 @@ public class SurfaceMountingManager { // This is null *until* StopSurface is called. private SparseArrayCompat mTagSetForStoppedSurface; + // This is to make sure direct manipulation result will not be overridden by React update. + @ThreadConfined(UI) + private final SparseArrayCompat> mTagToSynchronousMountProps = + new SparseArrayCompat<>(); + private final int mSurfaceId; public SurfaceMountingManager( @@ -682,13 +694,110 @@ public class SurfaceMountingManager { } } + private static void overridePropsReadableMap( + Map patchMap, WritableMap outputReadableMap) { + for (Map.Entry entry : patchMap.entrySet()) { + String propKey = entry.getKey(); + if (outputReadableMap.hasKey(propKey)) { + Object propValue = entry.getValue(); + if (propKey.equals("transform")) { + assert (outputReadableMap.getType(propKey) == ReadableType.Array + && propValue instanceof ArrayList); + WritableArray array = new WritableNativeArray(); + for (Object item : (ArrayList) propValue) { + if (item instanceof HashMap) { + WritableNativeMap itemMap = new WritableNativeMap(); + for (Map.Entry itemEntry : + ((HashMap) item).entrySet()) { + if (itemEntry.getValue() instanceof String) { + itemMap.putString(itemEntry.getKey(), (String) itemEntry.getValue()); + } else if (itemEntry.getValue() instanceof Number) { + itemMap.putDouble( + itemEntry.getKey(), ((Number) itemEntry.getValue()).doubleValue()); + } + } + array.pushMap(itemMap); + } + } + outputReadableMap.putArray(propKey, array); + } else if (propKey.equals("opacity")) { + assert (outputReadableMap.getType(propKey) == ReadableType.Number + && propValue instanceof Number); + outputReadableMap.putDouble(propKey, ((Number) propValue).doubleValue()); + } + } + } + } + + private static Map getHashMapFromPropsReadableMap(ReadableMap readableMap) { + HashMap outputMap = new HashMap<>(); + + Iterator> iter = readableMap.getEntryIterator(); + while (iter.hasNext()) { + Map.Entry entry = iter.next(); + String propKey = entry.getKey(); + Object propValue = entry.getValue(); + if (propKey.equals("transform") && propValue instanceof ReadableArray) { + ArrayList> arrayList = new ArrayList<>(); + for (int i = 0; i < ((ReadableArray) propValue).size(); i++) { + ReadableMap map = ((ReadableArray) propValue).getMap(i); + if (map != null) { + arrayList.add(map.toHashMap()); + } + } + outputMap.put(propKey, arrayList); + } else if (propKey.equals("opacity") && propValue instanceof Number) { + outputMap.put(propKey, ((Number) propValue).doubleValue()); + } + } + + return outputMap; + } + + public void storeSynchronousMountPropsOverride(int reactTag, ReadableMap props) { + if (ReactNativeFeatureFlags.overrideBySynchronousMountPropsAtMountingAndroid()) { + Map propsMap = getHashMapFromPropsReadableMap(props); + if (mTagToSynchronousMountProps.containsKey(reactTag)) { + Map mergedPropsMap = + Assertions.assertNotNull(mTagToSynchronousMountProps.get(reactTag)); + mergedPropsMap.putAll(propsMap); + mTagToSynchronousMountProps.put(reactTag, mergedPropsMap); + } else { + mTagToSynchronousMountProps.put(reactTag, propsMap); + } + } + } + + public void updatePropsSynchronously(int reactTag, ReadableMap props) { + updateProps(reactTag, props, true); + } + public void updateProps(int reactTag, ReadableMap props) { + updateProps(reactTag, props, false); + } + + @UiThread + private void updateProps( + int reactTag, ReadableMap props, Boolean shouldSkipSynchronousMountPropsOverride) { if (isStopped()) { return; } ViewState viewState = getViewState(reactTag); - viewState.mCurrentProps = new ReactStylesDiffMap(props); + + if (ReactNativeFeatureFlags.overrideBySynchronousMountPropsAtMountingAndroid() + && !shouldSkipSynchronousMountPropsOverride + && mTagToSynchronousMountProps.containsKey(reactTag)) { + WritableMap modifiedProps = new WritableNativeMap(); + modifiedProps.merge(props); + Map directPropsMap = + Assertions.assertNotNull(mTagToSynchronousMountProps.get(reactTag)); + overridePropsReadableMap(directPropsMap, modifiedProps); + viewState.mCurrentProps = new ReactStylesDiffMap(modifiedProps); + } else { + viewState.mCurrentProps = new ReactStylesDiffMap(props); + } + View view = viewState.mView; if (view == null) { @@ -1057,6 +1166,11 @@ public class SurfaceMountingManager { return; } + if (ReactNativeFeatureFlags.overrideBySynchronousMountPropsAtMountingAndroid() + && mTagToSynchronousMountProps.containsKey(reactTag)) { + mTagToSynchronousMountProps.remove(reactTag); + } + ViewState viewState = getNullableViewState(reactTag); if (viewState == null) { diff --git a/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp b/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp index b2fecda32d6..c4960f1fc19 100644 --- a/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp +++ b/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp @@ -742,7 +742,8 @@ folly::dynamic NativeAnimatedNodesManager::managedProps( if (const auto node = getAnimatedNode(iter->second)) { return node->props(); } - } else { + } else if (!ReactNativeFeatureFlags:: + overrideBySynchronousMountPropsAtMountingAndroid()) { std::lock_guard lockUnsyncedDirectViewProps( unsyncedDirectViewPropsMutex_); if (auto it = unsyncedDirectViewProps_.find(tag); @@ -761,7 +762,8 @@ bool NativeAnimatedNodesManager::hasManagedProps() const noexcept { return true; } } - { + if (!ReactNativeFeatureFlags:: + overrideBySynchronousMountPropsAtMountingAndroid()) { std::lock_guard lock(unsyncedDirectViewPropsMutex_); if (!unsyncedDirectViewProps_.empty()) { return true; @@ -771,10 +773,13 @@ bool NativeAnimatedNodesManager::hasManagedProps() const noexcept { } void NativeAnimatedNodesManager::onManagedPropsRemoved(Tag tag) noexcept { - std::lock_guard lock(unsyncedDirectViewPropsMutex_); - if (auto iter = unsyncedDirectViewProps_.find(tag); - iter != unsyncedDirectViewProps_.end()) { - unsyncedDirectViewProps_.erase(iter); + if (!ReactNativeFeatureFlags:: + overrideBySynchronousMountPropsAtMountingAndroid()) { + std::lock_guard lock(unsyncedDirectViewPropsMutex_); + if (auto iter = unsyncedDirectViewProps_.find(tag); + iter != unsyncedDirectViewProps_.end()) { + unsyncedDirectViewProps_.erase(iter); + } } } @@ -828,7 +833,8 @@ void NativeAnimatedNodesManager::schedulePropsCommit( mergeObjects(updateViewPropsDirect_[viewTag], props); } else if (!layoutStyleUpdated && directManipulationCallback_ != nullptr) { mergeObjects(updateViewPropsDirect_[viewTag], props); - { + if (!ReactNativeFeatureFlags:: + overrideBySynchronousMountPropsAtMountingAndroid()) { std::lock_guard lock(unsyncedDirectViewPropsMutex_); mergeObjects(unsyncedDirectViewProps_[viewTag], props); }