Skip to content

Commit 8189b14

Browse files
jingjing2222jingjing2222
committed
Fix Android Fabric sync prop null override
1 parent eeb17ba commit 8189b14

5 files changed

Lines changed: 149 additions & 12 deletions

File tree

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/PropsAnimatedNode.kt‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,21 @@ internal class PropsAnimatedNode(
6565
}
6666

6767
fun restoreDefaultValues() {
68-
// In Fabric, we don't restore default values since the FabricUIManager doesn't have access
69-
// to the ShadowNode layer. This method was only relevant for the legacy Paper renderer.
68+
if (connectedViewTag == -1) {
69+
return
70+
}
71+
72+
val defaultPropsMap = JavaOnlyMap()
73+
for ((key, value) in propNodeMapping) {
74+
val node = nativeAnimatedNodesManager.getNodeById(value)
75+
requireNotNull(node) { "Mapped property node does not exist" }
76+
if (node is StyleAnimatedNode) {
77+
node.collectViewDefaultValues(defaultPropsMap)
78+
} else {
79+
defaultPropsMap.putNull(key)
80+
}
81+
}
82+
connectedViewUIManager?.synchronouslyUpdateViewOnUIThread(connectedViewTag, defaultPropsMap)
7083
}
7184

7285
fun updateView() {

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/StyleAnimatedNode.kt‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,5 +58,11 @@ internal class StyleAnimatedNode(
5858
}
5959
}
6060

61+
fun collectViewDefaultValues(propsMap: JavaOnlyMap) {
62+
for (key in propMapping.keys) {
63+
propsMap.putNull(key)
64+
}
65+
}
66+
6167
override fun prettyPrint(): String = "StyleAnimatedNode[$tag] mPropMapping: $propMapping"
6268
}

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

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -626,13 +626,14 @@ internal constructor(
626626
public fun storeSynchronousMountPropsOverride(reactTag: Int, props: ReadableMap): Unit {
627627
if (ReactNativeFeatureFlags.overrideBySynchronousMountPropsAtMountingAndroid()) {
628628
val propsMap = getMapFromPropsReadableMap(props)
629-
var synchronousMountProps = tagToSynchronousMountProps[reactTag]
630-
if (synchronousMountProps != null) {
631-
synchronousMountProps.putAll(propsMap)
629+
val synchronousMountProps = tagToSynchronousMountProps[reactTag] ?: mutableMapOf()
630+
removeNullPropsFromPropsReadableMap(props, synchronousMountProps)
631+
synchronousMountProps.putAll(propsMap)
632+
if (synchronousMountProps.isEmpty()) {
633+
tagToSynchronousMountProps.remove(reactTag)
632634
} else {
633-
synchronousMountProps = propsMap
635+
tagToSynchronousMountProps[reactTag] = synchronousMountProps
634636
}
635-
tagToSynchronousMountProps[reactTag] = synchronousMountProps
636637
}
637638
}
638639

@@ -1332,7 +1333,10 @@ internal constructor(
13321333
for ((propKey, propValue) in patchMap) {
13331334
if (outputReadableMap.hasKey(propKey)) {
13341335
if (propKey == PROP_TRANSFORM) {
1335-
assert(outputReadableMap.getType(propKey) == ReadableType.Array && propValue is List<*>)
1336+
val outputType = outputReadableMap.getType(propKey)
1337+
assert(
1338+
(outputType == ReadableType.Array || outputType == ReadableType.Null) &&
1339+
propValue is List<*>)
13361340
val array = WritableNativeArray()
13371341
for (item in propValue as List<*>) {
13381342
if (item is Map<*, *>) {
@@ -1350,7 +1354,10 @@ internal constructor(
13501354
}
13511355
outputReadableMap.putArray(propKey, array)
13521356
} else if (propKey == PROP_OPACITY) {
1353-
assert(outputReadableMap.getType(propKey) == ReadableType.Number && propValue is Number)
1357+
val outputType = outputReadableMap.getType(propKey)
1358+
assert(
1359+
(outputType == ReadableType.Number || outputType == ReadableType.Null) &&
1360+
propValue is Number)
13541361
outputReadableMap.putDouble(propKey, (propValue as Number).toDouble())
13551362
}
13561363
}
@@ -1387,6 +1394,19 @@ internal constructor(
13871394
return outputMap
13881395
}
13891396

1397+
private fun removeNullPropsFromPropsReadableMap(
1398+
readableMap: ReadableMap,
1399+
outputMap: MutableMap<String, Any>,
1400+
) {
1401+
val iterator = readableMap.keySetIterator()
1402+
while (iterator.hasNextKey()) {
1403+
val propKey = iterator.nextKey()
1404+
if (readableMap.getType(propKey) == ReadableType.Null) {
1405+
outputMap.remove(propKey)
1406+
}
1407+
}
1408+
}
1409+
13901410
// prevents unchecked conversion warn of the <ViewGroup> type
13911411
private fun getViewGroupManager(viewState: ViewState): IViewGroupManager<View> {
13921412
val viewManager =

‎packages/react-native/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.kt‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1076,7 +1076,7 @@ class NativeAnimatedNodeTraversalTest {
10761076
}
10771077

10781078
@Test
1079-
fun testRestoreDefaultPropsIsNoOp() {
1079+
fun testRestoreDefaultPropsSendsNullUpdate() {
10801080
val viewTag: Int = 1001
10811081
val propsNodeTag = 3
10821082
nativeAnimatedNodesManager.createAnimatedNode(
@@ -1097,7 +1097,9 @@ class NativeAnimatedNodeTraversalTest {
10971097

10981098
reset(uiManagerMock)
10991099
nativeAnimatedNodesManager.restoreDefaultValues(propsNodeTag)
1100-
verify(uiManagerMock, never()).synchronouslyUpdateViewOnUIThread(anyInt(), any<ReadableMap>())
1100+
val stylesCaptor: ArgumentCaptor<ReadableMap> = ArgumentCaptor.forClass(ReadableMap::class.java)
1101+
verify(uiManagerMock).synchronouslyUpdateViewOnUIThread(eq(viewTag), stylesCaptor.capture())
1102+
assertThat(stylesCaptor.value.isNull("opacity")).isTrue()
11011103
}
11021104

11031105
/**

‎packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerSynchronousMountPropsTest.kt‎

Lines changed: 97 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
package com.facebook.react.fabric
1111

1212
import com.facebook.react.ReactRootView
13+
import com.facebook.react.bridge.JavaOnlyArray
1314
import com.facebook.react.bridge.JavaOnlyMap
15+
import com.facebook.react.bridge.ReadableArray
1416
import com.facebook.react.bridge.ReactTestHelper
1517
import com.facebook.react.fabric.mounting.MountingManager
1618
import com.facebook.react.fabric.mounting.MountingManager.MountItemExecutor
@@ -19,6 +21,7 @@ import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests
1921
import com.facebook.react.uimanager.ThemedReactContext
2022
import com.facebook.react.uimanager.ViewManager
2123
import com.facebook.react.uimanager.ViewManagerRegistry
24+
import com.facebook.react.views.view.ReactViewGroup
2225
import com.facebook.react.views.view.ReactViewManager
2326
import com.facebook.testutils.shadows.ShadowNativeLoader
2427
import com.facebook.testutils.shadows.ShadowNativeMap
@@ -67,7 +70,7 @@ class SurfaceMountingManagerSynchronousMountPropsTest {
6770
themedReactContext = ThemedReactContext(reactContext, reactContext, null, -1)
6871
mountingManager =
6972
MountingManager(
70-
ViewManagerRegistry(listOf<ViewManager<*, *>>(ReactViewManager())),
73+
ViewManagerRegistry(listOf<ViewManager<*, *>>(TestReactViewManager())),
7174
MountItemExecutor {},
7275
)
7376
}
@@ -83,6 +86,25 @@ class SurfaceMountingManagerSynchronousMountPropsTest {
8386
smm.addViewAt(surfaceId, tag, 0)
8487
}
8588

89+
private class TestReactViewManager : ReactViewManager() {
90+
override fun setTransformProperty(
91+
view: ReactViewGroup,
92+
transforms: ReadableArray?,
93+
transformOrigin: ReadableArray?,
94+
) {
95+
view.translationY = 0.0f
96+
if (transforms == null) {
97+
return
98+
}
99+
for (i in 0..<transforms.size()) {
100+
val transform = transforms.getMap(i) ?: continue
101+
if (transform.hasKey("translateY")) {
102+
view.translationY = transform.getDouble("translateY").toFloat()
103+
}
104+
}
105+
}
106+
}
107+
86108
/** Stored synchronous opacity should override a stale Fabric mount update. */
87109
@Test
88110
fun storeSynchronousProps_overridesStaleOpacityInUpdateProps() {
@@ -137,6 +159,80 @@ class SurfaceMountingManagerSynchronousMountPropsTest {
137159
assertThat(smm.getView(tag).alpha).isEqualTo(0.2f)
138160
}
139161

162+
/** A null Fabric prop update should not clear the stored synchronous opacity override. */
163+
@Test
164+
fun updateProps_withNullOpacity_keepsStoredSynchronousProp() {
165+
val smm = startSurface()
166+
val tag = 42
167+
createAndAttachView(smm, tag)
168+
169+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("opacity", 0.3))
170+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("opacity", 0.3))
171+
assertThat(smm.getView(tag).alpha).isEqualTo(0.3f)
172+
173+
smm.updateProps(tag, JavaOnlyMap.of("opacity", null))
174+
175+
assertThat(smm.getView(tag).alpha).isEqualTo(0.3f)
176+
}
177+
178+
/** A synchronous null update should clear the stored synchronous opacity override. */
179+
@Test
180+
fun updatePropsSynchronously_withNullOpacity_removesStoredSynchronousProp() {
181+
val smm = startSurface()
182+
val tag = 42
183+
createAndAttachView(smm, tag)
184+
185+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("opacity", 0.3))
186+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("opacity", 0.3))
187+
assertThat(smm.getView(tag).alpha).isEqualTo(0.3f)
188+
189+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("opacity", null))
190+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("opacity", null))
191+
assertThat(smm.getView(tag).alpha).isEqualTo(1.0f)
192+
193+
smm.updateProps(tag, JavaOnlyMap.of("opacity", null))
194+
195+
assertThat(smm.getView(tag).alpha).isEqualTo(1.0f)
196+
}
197+
198+
/** A null Fabric prop update should not clear the stored synchronous transform override. */
199+
@Test
200+
fun updateProps_withNullTransform_keepsStoredSynchronousProp() {
201+
val smm = startSurface()
202+
val tag = 42
203+
val transform = JavaOnlyArray.of(JavaOnlyMap.of("translateY", 40.0))
204+
createAndAttachView(smm, tag)
205+
206+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("transform", transform))
207+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("transform", transform))
208+
assertThat(smm.getView(tag).translationY).isEqualTo(40.0f)
209+
210+
smm.updateProps(tag, JavaOnlyMap.of("transform", null))
211+
212+
assertThat(smm.getView(tag).translationY).isEqualTo(40.0f)
213+
}
214+
215+
/** A synchronous null update should clear the stored synchronous transform override. */
216+
@Test
217+
fun updatePropsSynchronously_withNullTransform_removesStoredSynchronousProp() {
218+
val smm = startSurface()
219+
val tag = 42
220+
val transform = JavaOnlyArray.of(JavaOnlyMap.of("translateY", 40.0))
221+
createAndAttachView(smm, tag)
222+
223+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("transform", transform))
224+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("transform", transform))
225+
assertThat(smm.getView(tag).translationY).isEqualTo(40.0f)
226+
227+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("transform", null))
228+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("transform", null))
229+
assertThat(smm.getView(tag).translationY).isEqualTo(0.0f)
230+
231+
smm.updateProps(tag, JavaOnlyMap.of("transform", null))
232+
233+
assertThat(smm.getView(tag).translationY).isEqualTo(0.0f)
234+
}
235+
140236
/**
141237
* When a view is deleted, stored synchronous props should be cleaned up. A recreated view with
142238
* the same tag should not be affected by the old stored props.

0 commit comments

Comments
 (0)