Skip to content

Commit dae2f60

Browse files
zeyapfacebook-github-bot
zeyap
authored and
facebook-github-bot
committed
Course correct props at SurfaceMountingManager.updateProps() (#53589)
Summary: Pull Request resolved: #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
1 parent 5774bd1 commit dae2f60

5 files changed

Lines changed: 144 additions & 11 deletions

File tree

‎packages/react-native/ReactAndroid/api/ReactAndroid.api‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2354,12 +2354,14 @@ public class com/facebook/react/fabric/mounting/SurfaceMountingManager {
23542354
public fun sendAccessibilityEvent (II)V
23552355
public fun setJSResponder (IIZ)V
23562356
public fun stopSurface ()V
2357+
public fun storeSynchronousMountPropsOverride (ILcom/facebook/react/bridge/ReadableMap;)V
23572358
public fun sweepActiveTouchForTag (I)V
23582359
public fun updateEventEmitter (ILcom/facebook/react/fabric/events/EventEmitterWrapper;)V
23592360
public fun updateLayout (IIIIIIII)V
23602361
public fun updateOverflowInset (IIIII)V
23612362
public fun updatePadding (IIIII)V
23622363
public fun updateProps (ILcom/facebook/react/bridge/ReadableMap;)V
2364+
public fun updatePropsSynchronously (ILcom/facebook/react/bridge/ReadableMap;)V
23632365
public fun updateState (ILcom/facebook/react/uimanager/StateWrapper;)V
23642366
}
23652367

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -791,7 +791,8 @@ public void synchronouslyUpdateViewOnUIThread(final int reactTag, final Readable
791791
@Override
792792
public void execute(MountingManager mountingManager) {
793793
try {
794-
mountingManager.updateProps(reactTag, props);
794+
mountingManager.storeSynchronousMountPropsOverride(reactTag, props);
795+
mountingManager.updatePropsSynchronously(reactTag, props);
795796
} catch (Exception ex) {
796797
// TODO T42943890: Fix animations in Fabric and remove this try/catch?
797798
// There might always be race conditions between surface teardown and

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.kt‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -265,13 +265,23 @@ internal class MountingManager(
265265
}
266266

267267
@UiThread
268-
fun updateProps(reactTag: Int, props: ReadableMap?) {
268+
fun storeSynchronousMountPropsOverride(reactTag: Int, props: ReadableMap?) {
269269
assertOnUiThread()
270270
if (props == null) {
271271
return
272272
}
273273

274-
getSurfaceManagerForViewEnforced(reactTag).updateProps(reactTag, props)
274+
getSurfaceManagerForViewEnforced(reactTag).storeSynchronousMountPropsOverride(reactTag, props)
275+
}
276+
277+
@UiThread
278+
fun updatePropsSynchronously(reactTag: Int, props: ReadableMap?) {
279+
assertOnUiThread()
280+
if (props == null) {
281+
return
282+
}
283+
284+
getSurfaceManagerForViewEnforced(reactTag).updatePropsSynchronously(reactTag, props)
275285
}
276286

277287
/**

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java‎

Lines changed: 115 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,14 @@
2727
import com.facebook.react.bridge.ReactSoftExceptionLogger;
2828
import com.facebook.react.bridge.ReadableArray;
2929
import com.facebook.react.bridge.ReadableMap;
30+
import com.facebook.react.bridge.ReadableType;
3031
import com.facebook.react.bridge.RetryableMountingLayerException;
3132
import com.facebook.react.bridge.SoftAssertions;
3233
import com.facebook.react.bridge.UiThreadUtil;
34+
import com.facebook.react.bridge.WritableArray;
3335
import com.facebook.react.bridge.WritableMap;
36+
import com.facebook.react.bridge.WritableNativeArray;
37+
import com.facebook.react.bridge.WritableNativeMap;
3438
import com.facebook.react.common.annotations.UnstableReactNativeAPI;
3539
import com.facebook.react.common.build.ReactBuildConfig;
3640
import com.facebook.react.common.mapbuffer.MapBuffer;
@@ -53,7 +57,10 @@
5357
import com.facebook.react.uimanager.events.EventCategoryDef;
5458
import com.facebook.systrace.Systrace;
5559
import java.util.ArrayDeque;
60+
import java.util.ArrayList;
61+
import java.util.HashMap;
5662
import java.util.HashSet;
63+
import java.util.Iterator;
5764
import java.util.LinkedList;
5865
import java.util.Map;
5966
import java.util.Queue;
@@ -96,6 +103,11 @@ public class SurfaceMountingManager {
96103
// This is null *until* StopSurface is called.
97104
private SparseArrayCompat<Object> mTagSetForStoppedSurface;
98105

106+
// This is to make sure direct manipulation result will not be overridden by React update.
107+
@ThreadConfined(UI)
108+
private final SparseArrayCompat<Map<String, Object>> mTagToSynchronousMountProps =
109+
new SparseArrayCompat<>();
110+
99111
private final int mSurfaceId;
100112

101113
public SurfaceMountingManager(
@@ -682,13 +694,110 @@ public void createViewUnsafe(
682694
}
683695
}
684696

697+
private static void overridePropsReadableMap(
698+
Map<String, Object> patchMap, WritableMap outputReadableMap) {
699+
for (Map.Entry<String, Object> entry : patchMap.entrySet()) {
700+
String propKey = entry.getKey();
701+
if (outputReadableMap.hasKey(propKey)) {
702+
Object propValue = entry.getValue();
703+
if (propKey.equals("transform")) {
704+
assert (outputReadableMap.getType(propKey) == ReadableType.Array
705+
&& propValue instanceof ArrayList);
706+
WritableArray array = new WritableNativeArray();
707+
for (Object item : (ArrayList<?>) propValue) {
708+
if (item instanceof HashMap) {
709+
WritableNativeMap itemMap = new WritableNativeMap();
710+
for (Map.Entry<String, Object> itemEntry :
711+
((HashMap<String, Object>) item).entrySet()) {
712+
if (itemEntry.getValue() instanceof String) {
713+
itemMap.putString(itemEntry.getKey(), (String) itemEntry.getValue());
714+
} else if (itemEntry.getValue() instanceof Number) {
715+
itemMap.putDouble(
716+
itemEntry.getKey(), ((Number) itemEntry.getValue()).doubleValue());
717+
}
718+
}
719+
array.pushMap(itemMap);
720+
}
721+
}
722+
outputReadableMap.putArray(propKey, array);
723+
} else if (propKey.equals("opacity")) {
724+
assert (outputReadableMap.getType(propKey) == ReadableType.Number
725+
&& propValue instanceof Number);
726+
outputReadableMap.putDouble(propKey, ((Number) propValue).doubleValue());
727+
}
728+
}
729+
}
730+
}
731+
732+
private static Map<String, Object> getHashMapFromPropsReadableMap(ReadableMap readableMap) {
733+
HashMap<String, Object> outputMap = new HashMap<>();
734+
735+
Iterator<Map.Entry<String, Object>> iter = readableMap.getEntryIterator();
736+
while (iter.hasNext()) {
737+
Map.Entry<String, Object> entry = iter.next();
738+
String propKey = entry.getKey();
739+
Object propValue = entry.getValue();
740+
if (propKey.equals("transform") && propValue instanceof ReadableArray) {
741+
ArrayList<HashMap<String, Object>> arrayList = new ArrayList<>();
742+
for (int i = 0; i < ((ReadableArray) propValue).size(); i++) {
743+
ReadableMap map = ((ReadableArray) propValue).getMap(i);
744+
if (map != null) {
745+
arrayList.add(map.toHashMap());
746+
}
747+
}
748+
outputMap.put(propKey, arrayList);
749+
} else if (propKey.equals("opacity") && propValue instanceof Number) {
750+
outputMap.put(propKey, ((Number) propValue).doubleValue());
751+
}
752+
}
753+
754+
return outputMap;
755+
}
756+
757+
public void storeSynchronousMountPropsOverride(int reactTag, ReadableMap props) {
758+
if (ReactNativeFeatureFlags.overrideBySynchronousMountPropsAtMountingAndroid()) {
759+
Map<String, Object> propsMap = getHashMapFromPropsReadableMap(props);
760+
if (mTagToSynchronousMountProps.containsKey(reactTag)) {
761+
Map<String, Object> mergedPropsMap =
762+
Assertions.assertNotNull(mTagToSynchronousMountProps.get(reactTag));
763+
mergedPropsMap.putAll(propsMap);
764+
mTagToSynchronousMountProps.put(reactTag, mergedPropsMap);
765+
} else {
766+
mTagToSynchronousMountProps.put(reactTag, propsMap);
767+
}
768+
}
769+
}
770+
771+
public void updatePropsSynchronously(int reactTag, ReadableMap props) {
772+
updateProps(reactTag, props, true);
773+
}
774+
685775
public void updateProps(int reactTag, ReadableMap props) {
776+
updateProps(reactTag, props, false);
777+
}
778+
779+
@UiThread
780+
private void updateProps(
781+
int reactTag, ReadableMap props, Boolean shouldSkipSynchronousMountPropsOverride) {
686782
if (isStopped()) {
687783
return;
688784
}
689785

690786
ViewState viewState = getViewState(reactTag);
691-
viewState.mCurrentProps = new ReactStylesDiffMap(props);
787+
788+
if (ReactNativeFeatureFlags.overrideBySynchronousMountPropsAtMountingAndroid()
789+
&& !shouldSkipSynchronousMountPropsOverride
790+
&& mTagToSynchronousMountProps.containsKey(reactTag)) {
791+
WritableMap modifiedProps = new WritableNativeMap();
792+
modifiedProps.merge(props);
793+
Map<String, Object> directPropsMap =
794+
Assertions.assertNotNull(mTagToSynchronousMountProps.get(reactTag));
795+
overridePropsReadableMap(directPropsMap, modifiedProps);
796+
viewState.mCurrentProps = new ReactStylesDiffMap(modifiedProps);
797+
} else {
798+
viewState.mCurrentProps = new ReactStylesDiffMap(props);
799+
}
800+
692801
View view = viewState.mView;
693802

694803
if (view == null) {
@@ -1057,6 +1166,11 @@ public void deleteView(int reactTag) {
10571166
return;
10581167
}
10591168

1169+
if (ReactNativeFeatureFlags.overrideBySynchronousMountPropsAtMountingAndroid()
1170+
&& mTagToSynchronousMountProps.containsKey(reactTag)) {
1171+
mTagToSynchronousMountProps.remove(reactTag);
1172+
}
1173+
10601174
ViewState viewState = getNullableViewState(reactTag);
10611175

10621176
if (viewState == null) {

‎packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -742,7 +742,8 @@ folly::dynamic NativeAnimatedNodesManager::managedProps(
742742
if (const auto node = getAnimatedNode<PropsAnimatedNode>(iter->second)) {
743743
return node->props();
744744
}
745-
} else {
745+
} else if (!ReactNativeFeatureFlags::
746+
overrideBySynchronousMountPropsAtMountingAndroid()) {
746747
std::lock_guard<std::mutex> lockUnsyncedDirectViewProps(
747748
unsyncedDirectViewPropsMutex_);
748749
if (auto it = unsyncedDirectViewProps_.find(tag);
@@ -761,7 +762,8 @@ bool NativeAnimatedNodesManager::hasManagedProps() const noexcept {
761762
return true;
762763
}
763764
}
764-
{
765+
if (!ReactNativeFeatureFlags::
766+
overrideBySynchronousMountPropsAtMountingAndroid()) {
765767
std::lock_guard<std::mutex> lock(unsyncedDirectViewPropsMutex_);
766768
if (!unsyncedDirectViewProps_.empty()) {
767769
return true;
@@ -771,10 +773,13 @@ bool NativeAnimatedNodesManager::hasManagedProps() const noexcept {
771773
}
772774

773775
void NativeAnimatedNodesManager::onManagedPropsRemoved(Tag tag) noexcept {
774-
std::lock_guard<std::mutex> lock(unsyncedDirectViewPropsMutex_);
775-
if (auto iter = unsyncedDirectViewProps_.find(tag);
776-
iter != unsyncedDirectViewProps_.end()) {
777-
unsyncedDirectViewProps_.erase(iter);
776+
if (!ReactNativeFeatureFlags::
777+
overrideBySynchronousMountPropsAtMountingAndroid()) {
778+
std::lock_guard<std::mutex> lock(unsyncedDirectViewPropsMutex_);
779+
if (auto iter = unsyncedDirectViewProps_.find(tag);
780+
iter != unsyncedDirectViewProps_.end()) {
781+
unsyncedDirectViewProps_.erase(iter);
782+
}
778783
}
779784
}
780785

@@ -828,7 +833,8 @@ void NativeAnimatedNodesManager::schedulePropsCommit(
828833
mergeObjects(updateViewPropsDirect_[viewTag], props);
829834
} else if (!layoutStyleUpdated && directManipulationCallback_ != nullptr) {
830835
mergeObjects(updateViewPropsDirect_[viewTag], props);
831-
{
836+
if (!ReactNativeFeatureFlags::
837+
overrideBySynchronousMountPropsAtMountingAndroid()) {
832838
std::lock_guard<std::mutex> lock(unsyncedDirectViewPropsMutex_);
833839
mergeObjects(unsyncedDirectViewProps_[viewTag], props);
834840
}

0 commit comments

Comments
 (0)