From d8a240eadc1f6a28c02010c4fe4f9198ecdb57f5 Mon Sep 17 00:00:00 2001 From: Matthew Horan Date: Wed, 20 Nov 2024 18:43:13 -0500 Subject: [PATCH 1/2] fix: only intercept events with waiting handlers Previously, NativeReanimatedModule::handleRawEvent would intercept all events received by the event listener. This resulted in an issue where onLayout would not fire in JS on the New Architecture. Instead, only intercept events with waiting handlers. This prevents asJSIValue from being called on the Reanimated event loop and allows onLayout to bubble up in JS. See https://github.com/facebook/react-native/blob/v0.76.2/packages/react-native/ReactCommon/react/renderer/components/view/BaseViewEventEmitter.cpp#L82-L112, which prevents onLayout from being dispatched more than once. asJSIValue evaluates the lambda above in https://github.com/facebook/react-native/blob/v0.76.2/packages/react-native/ReactCommon/react/renderer/core/ValueFactoryEventPayload.cpp#L16. Fixes #6684 --- .../RuntimeTests/RuntimeTestsExample.tsx | 2 + .../RuntimeTests/tests/core/onLayout.test.tsx | 71 +++++++++++++++++++ .../NativeModules/ReanimatedModuleProxy.cpp | 5 ++ 3 files changed, 78 insertions(+) create mode 100644 apps/common-app/src/examples/RuntimeTests/tests/core/onLayout.test.tsx diff --git a/apps/common-app/src/examples/RuntimeTests/RuntimeTestsExample.tsx b/apps/common-app/src/examples/RuntimeTests/RuntimeTestsExample.tsx index 6a460dbf4ef..4ad23f2af2d 100644 --- a/apps/common-app/src/examples/RuntimeTests/RuntimeTestsExample.tsx +++ b/apps/common-app/src/examples/RuntimeTests/RuntimeTestsExample.tsx @@ -51,6 +51,8 @@ export default function RuntimeTestsExample() { require('./tests/core/useDerivedValue/chain.test'); require('./tests/core/useSharedValue/animationsCompilerApi.test'); + + require('./tests/core/onLayout.test'); }, }, { diff --git a/apps/common-app/src/examples/RuntimeTests/tests/core/onLayout.test.tsx b/apps/common-app/src/examples/RuntimeTests/tests/core/onLayout.test.tsx new file mode 100644 index 00000000000..bb138eccc5a --- /dev/null +++ b/apps/common-app/src/examples/RuntimeTests/tests/core/onLayout.test.tsx @@ -0,0 +1,71 @@ +import { useEffect } from 'react'; +import type { LayoutChangeEvent } from 'react-native'; +import { StyleSheet, View } from 'react-native'; +import Animated, { runOnJS, runOnUI, useAnimatedStyle, useEvent, useSharedValue } from 'react-native-reanimated'; +import { describe, expect, notify, render, test, wait, waitForNotify } from '../../ReJest/RuntimeTestsApi'; + +interface TestResult { + height: number; + animatedHandlerCalled: boolean; +} + +const TestComponent = ({ result, notifyId }: { result: TestResult; notifyId: number }) => { + const height = useSharedValue(styles.smallBox.height); + + const onLayout = (event: LayoutChangeEvent) => { + result.height = event.nativeEvent.layout.height; + if (result.height === 200) { + notify(`onLayout${notifyId}`); + } + }; + + const animatedStyle = useAnimatedStyle(() => { + return { height: height.value }; + }); + + useEffect(() => { + runOnUI(() => { + height.value += 100; + })(); + }, [height]); + + const setAnimatedHandlerCalled = () => { + result.animatedHandlerCalled = true; + notify(`animatedOnLayout${notifyId}`); + }; + + const animatedOnLayout = useEvent(() => { + 'worklet'; + runOnJS(setAnimatedHandlerCalled)(); + }, ['onLayout']); + + return ( + + + + ); +}; + +describe('onLayout', () => { + test('is not intercepted when there are no registered event handlers', async () => { + const result = {} as TestResult; + await render(); + await Promise.race([waitForNotify('onLayout1'), wait(1000)]); + expect(result.height).toBe(200); + }); + + test('is dispatched to the registered event handler', async () => { + const result = {} as TestResult; + await render(); + await Promise.race([waitForNotify('animatedOnLayout2'), wait(1000)]); + expect(result.animatedHandlerCalled).toBe(true); + }); +}); + +const styles = StyleSheet.create({ + smallBox: { + width: 100, + height: 100, + backgroundColor: 'pink', + }, +}); diff --git a/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp b/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp index b159b070d6c..39599f6e45e 100644 --- a/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp +++ b/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp @@ -553,6 +553,11 @@ bool ReanimatedModuleProxy::handleRawEvent( if (eventType.rfind("top", 0) == 0) { eventType = "on" + eventType.substr(3); } + + if (!isAnyHandlerWaitingForEvent(eventType, tag)) { + return false; + } + jsi::Runtime &rt = workletsModuleProxy_->getUIWorkletRuntime()->getJSIRuntime(); const auto &eventPayload = rawEvent.eventPayload; From 6adb38026d846a4179266b154bd49d0afddf33d9 Mon Sep 17 00:00:00 2001 From: Matthew Horan Date: Fri, 22 Nov 2024 20:16:51 -0500 Subject: [PATCH 2/2] chore: reset notification registry between tests Using notify in the runtime tests would result in test pollution. Reset the notification registry between tests, similar to the call tracker registry. --- .../ReJest/TestRunner/NotificationRegistry.ts | 6 +++++- .../RuntimeTests/ReJest/TestRunner/TestRunner.ts | 1 + .../RuntimeTests/tests/core/onLayout.test.tsx | 14 +++++++------- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/apps/common-app/src/examples/RuntimeTests/ReJest/TestRunner/NotificationRegistry.ts b/apps/common-app/src/examples/RuntimeTests/ReJest/TestRunner/NotificationRegistry.ts index 6a97b0670ea..a75f80a24a7 100644 --- a/apps/common-app/src/examples/RuntimeTests/ReJest/TestRunner/NotificationRegistry.ts +++ b/apps/common-app/src/examples/RuntimeTests/ReJest/TestRunner/NotificationRegistry.ts @@ -1,6 +1,6 @@ import { runOnJS } from 'react-native-reanimated'; -const notificationRegistry: Record = {}; +let notificationRegistry: Record = {}; function notifyJS(name: string) { notificationRegistry[name] = true; } @@ -25,4 +25,8 @@ export class NotificationRegistry { }, 10); }); } + + public resetRegistry() { + notificationRegistry = {}; + } } diff --git a/apps/common-app/src/examples/RuntimeTests/ReJest/TestRunner/TestRunner.ts b/apps/common-app/src/examples/RuntimeTests/ReJest/TestRunner/TestRunner.ts index e16ff623c6d..659e568a871 100644 --- a/apps/common-app/src/examples/RuntimeTests/ReJest/TestRunner/TestRunner.ts +++ b/apps/common-app/src/examples/RuntimeTests/ReJest/TestRunner/TestRunner.ts @@ -127,6 +127,7 @@ export class TestRunner { private async runTestCase(testSuite: TestSuite, testCase: TestCase) { this._callTrackerRegistry.resetRegistry(); + this._notificationRegistry.resetRegistry(); this._currentTestCase = testCase; if (testSuite.beforeEach) { diff --git a/apps/common-app/src/examples/RuntimeTests/tests/core/onLayout.test.tsx b/apps/common-app/src/examples/RuntimeTests/tests/core/onLayout.test.tsx index bb138eccc5a..a22860ae769 100644 --- a/apps/common-app/src/examples/RuntimeTests/tests/core/onLayout.test.tsx +++ b/apps/common-app/src/examples/RuntimeTests/tests/core/onLayout.test.tsx @@ -9,13 +9,13 @@ interface TestResult { animatedHandlerCalled: boolean; } -const TestComponent = ({ result, notifyId }: { result: TestResult; notifyId: number }) => { +const TestComponent = ({ result }: { result: TestResult }) => { const height = useSharedValue(styles.smallBox.height); const onLayout = (event: LayoutChangeEvent) => { result.height = event.nativeEvent.layout.height; if (result.height === 200) { - notify(`onLayout${notifyId}`); + notify('onLayout'); } }; @@ -31,7 +31,7 @@ const TestComponent = ({ result, notifyId }: { result: TestResult; notifyId: num const setAnimatedHandlerCalled = () => { result.animatedHandlerCalled = true; - notify(`animatedOnLayout${notifyId}`); + notify('animatedOnLayout'); }; const animatedOnLayout = useEvent(() => { @@ -49,15 +49,15 @@ const TestComponent = ({ result, notifyId }: { result: TestResult; notifyId: num describe('onLayout', () => { test('is not intercepted when there are no registered event handlers', async () => { const result = {} as TestResult; - await render(); - await Promise.race([waitForNotify('onLayout1'), wait(1000)]); + await render(); + await Promise.race([waitForNotify('onLayout'), wait(1000)]); expect(result.height).toBe(200); }); test('is dispatched to the registered event handler', async () => { const result = {} as TestResult; - await render(); - await Promise.race([waitForNotify('animatedOnLayout2'), wait(1000)]); + await render(); + await Promise.race([waitForNotify('animatedOnLayout'), wait(1000)]); expect(result.animatedHandlerCalled).toBe(true); }); });