fix: only intercept events with waiting handlers

[mhoran]

Summary

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

Test plan

See reproducer in #6684. Without this change, onLayout will not fire on device rotation. With this change, onLayout will fire as expected.

[tomekzaw]

@mhoran Thanks for the PR. Just making sure if I understand the problem and the proposed solution correctly.

Currently, all onLayout events are intercepted by Reanimated (while still on the UI thread), and calling asJSIValue results in layoutEventState->wasDispatched being set to true. Then, once the event is processed on the JS thread, asJSIValue returns null.

After this change, the flow described above will be true only for events that pass isAnyHandlerWaitingForEvent test. For all other onLayout events (those which are not awaited by Reanimated), asJSIValue will not be called on the UI thread so asJSIValue will return the actual event data when called on the JS thread.

Is that correct?

[mhoran]

Currently, all onLayout events are intercepted by Reanimated (while still on the UI thread), and calling asJSIValue results in layoutEventState->wasDispatched being set to true. Then, once the event is processed on the JS thread, asJSIValue returns null.

Correct. While the Reanimated event listener will bail on events dispatched from the JS thread:

react-native-reanimated/packages/react-native-reanimated/apple/reanimated/apple/REAModule.mm

Lines 148 to 153 in 254af50

	if (!RCTIsMainQueue()) {
	// event listener called on the JS thread, let's ignore this event
	// as we cannot safely access worklet runtime here
	// and also we don't care about topLayout events
	return false;
	}

the rotation layout event dispatches from the UI thread. This results in the handler being called with the layout event, which Reanimated intercepts.

After this change, the flow described above will be true only for events that pass isAnyHandlerWaitingForEvent test. For all other onLayout events (those which are not awaited by Reanimated), asJSIValue will not be called on the UI thread so asJSIValue will return the actual event data when called on the JS thread.

Correct. This would ultimately happen when NativeReanimatedModule::handleEvent calls in to EventHandlerRegistry::processEvent, but unfortunately due to the way asJSIValue is implemented, and the debounce logic in the layout lambda, just getting the value will intercept the event, which is not desired.

I am not sure what would happen if one were to set up an onLayout handler that was intended to be handled by Reanimated. Is this even a valid use case? I think it would then get dispatched due to isAnyHandlerWaitingForEvent returning true, but then would not be dispatched on the React Native side. That seems fine, but I'm not sure how to even test this.

I added a test for this as well. Triggering a layout event by modifying a view's styles from the UI thread is equivalent to a device rotation (which dispatches on the UI thread.) The test will fail prior to this code change but passes after.

[tomekzaw]

Thanks for clarification as well as adding the tests.

I am not sure what would happen if one were to set up an onLayout handler that was intended to be handled by Reanimated. Is this even a valid use case?

Sounds like a valid use case, here's a draft PR where I register an event listener for topLayout event: #5738

[mhoran]

Sounds like a valid use case, here's a draft PR where I register an event listener for topLayout event: #5738

I've added a test to cover this use case as well.