Summary:
With the updates to touch processing rolled out, we can remove the feature flag and clean up the old code. The old path is now used exclusively by legacy renderer and Fabric uses new `EventEmitter#receiveTouches` method to process touches.
Changelog:
[Changed][Android] - Update touch processing internals
Reviewed By: mdvacca
Differential Revision: D32953664
fbshipit-source-id: 517a4ce6ce9bc15528c2db94d7d11bdff8b78743
Summary:
Changelog:
[Android][Added] Adding new API `onChildEndedNativeGesture` to the RootView interface to let its implementations notify the JS side that a child gesture is ended.
Reviewed By: javache
Differential Revision: D32228745
fbshipit-source-id: ad1f26546dd60f9c5a569b0bc3ad5020a01b90cc
Summary:
The `Event.getSurfaceIdForView` method I added recently is actually a duplicate of `UIManagerHelper.getSurfaceId`, except that the latter doesn't support RootViews very well.
Changelog:
[Android][Changed] - Improved UIManagerHelper.getSurfaceId and removed Event.getSurfaceIdForView
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D32102175
fbshipit-source-id: 01741df6b646037a4575e9ca302ea248af9fd6f3
Summary:
Ensures that copy of the native touch objects happens before consuming them.
The reverse order seems accidental after refactor, as copying objects doesn't consume them whereas adding to a native array does. This behavior didn't show up during testing in dev environment (only affects CANCEL events), and it seems to be the cause of high-firing crash on production.
Changelog: [Internal] Copy touch objects before consuming them in the new touch path.
Reviewed By: mdvacca
Differential Revision: D32112036
fbshipit-source-id: e9ec47689b7ceb0a40a23bab9f03367c4acb8632
Summary:
Makes new touch processing path check for double dispose on touch events.
Old event dispatcher has a race condition which makes it double-dispose some events, so we need to make sure it also processes touches correctly.
Changelog: [Internal] Check for double dispose when sending touch event
Reviewed By: cortinico
Differential Revision: D32110250
fbshipit-source-id: d6a12cbac60f9ff5e836cfaca5a47c467bea06c7
Summary:
Colocate the surface ID generate with the code that consumes it. This allows us to re-use this method in other event emitter locations.
Changelog: [Android][Changed] Add helper to get surfaceId for event dispatching
Reviewed By: philIip
Differential Revision: D31651881
fbshipit-source-id: 109e189f90261d3ba0077ffa519c3d12a9111439
Summary:
Changelog:
[Android][Changed] RootView's onChildStartedNativeGesture now takes the child view as its first argument
Reviewed By: philIip
Differential Revision: D31399515
fbshipit-source-id: b9438f6118e604a04799ef67d0b46303a06d6434
Summary:
Using `WriteableNativeArray` directly in common code is breaking unit tests on CircleCI.
Changelog:
[Android][Internal] - Use mock of native array for sending touches
Reviewed By: mdvacca
Differential Revision: D31665842
fbshipit-source-id: 886418ff6a3f07046e8e17d4743060d80c26b288
Summary:
Updates touch events in Fabric to be dispatched through the same pipeline as the rest of events, instead of relying on custom dispatch behavior.
Previous method of handling touches was reusing Paper behavior which required:
1. Transform event into a Paper-compatible form of WritableArray and dispatch it to `RCTEventEmitter.receiveTouches`.
2. Intercept `receiveTouches` for Fabric and redirect it to `FabricEventEmitter`
3. Perform transformations copied from Paper JS renderer in Java, transform it to the final form and dispatch this event as usual after.
The new behavior uses emitter's `receiveEvent` method directly to dispatch events. Additionally, it should decrease allocations done when transforming events during step 3 above, as `WritableNativeMap`-based operations performed many re-allocations when reading/re-creating arrays.
Changelog:
[Android][Changed] - Added an experimental touch dispatch path
Reviewed By: JoshuaGross
Differential Revision: D31280052
fbshipit-source-id: 829c2646ac6b0ebff0f0106159e76d84324ac732
Summary:
Simplifies logic of touch dispatch by retrieving surface id and other require info from the event directly.
Changelog: [Android][Internal] - Simplify logic of dispatching touches
Reviewed By: cortinico
Differential Revision: D31583314
fbshipit-source-id: c6b6e131a759c2ebe0cf4441c3aeb1a8b9f5781e
Summary:
Propagate event category definition to every event that is using `dispatchModernV2` (gated in production), providing opportunity to override categories of some events if needed. No events are meaningfully affected by this change, as coalesced events (e.g. scroll) are always dispatched as continuous and touch events are handled separately.
Changelog:
[Internal] Expose event category in Event class
Reviewed By: cortinico
Differential Revision: D31276249
fbshipit-source-id: f9a756b3a5cf5897e17209f3d0aed6a1c16cbd2e
Summary:
Event merging or "coalescing" is done on Java side from Android, but Fabric also includes some Cxx logic to merge those events. Although Android doesn't need this logic in particular, it is important to follow this path to ensure these events (e.g. scroll) are dispatched as "continuous", allowing for correct prioritization in Concurrent Mode.
`dispatchModernV2` selects between `dispatch` and `dispatchUnique` based on the `canBeCoalesced` parameter of the event, which is exactly what we need. The logic is only used in the "new" event dispatcher at the moment, so I wrapped it with feature flag to validate it doesn't cause any regressions.
Changelog:
[Android][Internal] - Try dispatching coalescing events as unique to Fabric
Reviewed By: sammy-SC
Differential Revision: D31272585
fbshipit-source-id: 6b67b61bd13fbff019d9eb8c5172bdd814a7b5b8
Summary:
Assuming this method was deprecated to mean experimental.
This execution path is used by overwhelming majority of events and it seems as stable as it can be.
Changelog:
[Android][Changed] Removed experimental deprecation from `dispatchModern`
Reviewed By: cortinico
Differential Revision: D31270721
fbshipit-source-id: 5a7e50455ab2850adf9bc86a248773b170bf0ab9
Summary:
Updates event category deduction to match iOS implementation.
The event priority is used by concurrent mode to prioritize certain events, where Cxx part already assigns the correct priority based on the `ContinuousStart` -> `ContinuousEnd` spans. These spans can be deduced from the touch events, which we do in this implementation.
All events that can be "coalesced" (dispatched through `invokeUnique`) are assigned `Continuous` by default in Fabric core, so scroll/slider change events will never be discrete.
Changelog:
[Internal] Add category deduction to Android touch events
Reviewed By: mdvacca
Differential Revision: D31233233
fbshipit-source-id: f5b039aa137f1b4d2e2b15578bfc29ab6903a081
Summary:
When TouchEvent is created in RN, we're currently using System.currentTimeMillis - but this can differ from the MotionEvent timestamp by a few milliseconds.
This difference is very minor but makes it challenging to implement touch telemetry. It's easy and should be zero-impact otherwise to align the timestamps.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D31183733
fbshipit-source-id: 5b275ee534658dc429beb1d3cec0c83a779b5ea3
Summary:
Just a fix of a typo
changelog: [internal] internal
Reviewed By: javache
Differential Revision: D31076782
fbshipit-source-id: 192de92ba080a565acd67e038b370917ea9fcddc
Summary:
ReactSoftException makes it seem like the class is an Exception, when it's actually a logger. This rename I think is more appropriate.
Changelog: [Internal]
Reviewed By: JoshuaGross
Differential Revision: D30251735
fbshipit-source-id: 550bd543388e698774ec31753200bd3f722d8c17
Summary:
It seems that, possibly due to optimizations and refactoring elsewhere in the event system, some TouchEvents are being over-disposed.
This doesn't really pose a problem besides performance; and could even indicate that an Event was in a pool but never properly initialized. In these
cases it seems perfectly reasonable to silently continue, and to log a soft exception. This WILL still crash in debug mode, so we can gather more information
if we find a good repro; otherwise we will continue to get production data from this soft exception if it's an issue and we can investigate further.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D30061178
fbshipit-source-id: 05d1f60afc382ce0a202ac8f3de34770cf9a760d
Summary:
I suspect that T94864568 is caused by TouchEvents being dispatched after they've been recycled. This needs further analysis, but to stop the bleeding, we can drop events at the point they'd be dispatched before the crash, and log a soft error.
Changelog: [Internal]
Reviewed By: ShikaSD
Differential Revision: D29594749
fbshipit-source-id: f50df8df2125b83126616ceaf4e529127d154c7c
Summary:
It is unlikely but possible that the crash T94864568 is caused by a TouchEvent being initialized with a null MotionEvent. Regardless, we should guard against this case.
Changelog: [Internal]
Reviewed By: ShikaSD
Differential Revision: D29594750
fbshipit-source-id: 3a409b716a9f1eec8017002ae7e23273677e53ba
Summary:
TL;DR: simplify and delete a bunch of stuff that shouldn't be necessary in Fabric.
I discovered that this event dispatcher (and the older one this is based on) is triple-queueing: we queue events into "staging", and then post "dispatch events" to only run /on the JS thread/. Even in Fabric. Then, each of these events is emitted into C++ where they are queued /again/! This refactor eliminates one more level of queueing - instead of scheduling dispatch for the JS thread, we just emit them directly to C++ when they're received in Java.
Unfortunately, the EventDispatcher is also used to drive AsyncEventBeat in C++:
1. EventBeatManager.onBatchEventDispatched: https://fburl.com/diffusion/qf6dyhsw
In the C++ impl, it indirectly will drive the AsyncEventBeat/AsyncEventBeatV2.
2. onBatchEventDispatched is ONLY called from EventDispatcherImpl: https://fburl.com/codesearch/mxk8ifyj
3. Which is queued and only runs on the JS thread: https://fburl.com/codesearch/czvbst4u
This means the AsyncEventBeat is only ticked when the JS thread is free, and ticks will be skipped when the JS thread is occupied for long periods.
Now, in this refactor, when this class is used it will drive AsyncEventBeat on every UI tick. This is also potentially not correct. On iOS (Fabric), AsyncEventBeat is driven when the UI thread is "about to sleep".
For now I'm not going to worry about that detail - it is significant, but Fabric+Android is currently /not doing the right thing/ and it's not clear that we want to maintain iOS behavior. This is something we need to discuss further and figure out.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D28654033
fbshipit-source-id: b3cb9b706343c8dd3c4cf84f24388908c57e2138
Summary:
EventDispatcherImpl uses synchronized blocks all over to make it thread-safe. I'm concerned about the perf implications of this and creating contention between JS and UI threads.
This is locked behind a feature flag.
Enabled only for Fabric in StaticViewConfig mode, and a feature flag, for now.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D28591331
fbshipit-source-id: ea8f93a2e1343ce37fa78690dcb62fe03594120f
Summary:
See comments inline for motivation. It's not safe to use viewtag of an Event to infer whether or not the view is in a Fabric or non-Fabric RootView.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D28365566
fbshipit-source-id: 187ddcc5d5a43a31a71232fdb2f1f5b334bec8c2
Summary:
Sometimes ```hasActiveCatalystInstance()``` is used to check if it's safe to access the CatalystInstance, which will still crash in Venice.
Previously we mitigate this by changing ```reactContext.hasActiveCatalystInstance()``` to ```reactContext.hasActiveCatalystInstance() || reactContext.isBridgeless()```.
To solve this for all and good the plan is:
1, Rename ```hasActiveCatalystInstance()``` to ```hasActiveReactInstance()``` so it won't sounds like CatalystInstance-only.
2, Implement hasActiveReactInstance() for Venice. D27343867
3, Remove previous mitigation. D27343952
This diff is the first step, by xbgs there are **58** non-generated callsites of ```hasActiveCatalystInstance()``` in code base which are all renamed in this diff.
Changelog:
[Android][Changed] - Rename "hasActiveCatalystInstance" to "hasActiveReactInstance"
Reviewed By: mdvacca
Differential Revision: D27335055
fbshipit-source-id: 5b8ff5e09b79a492e910bb8f197e70fa1360bcef
Summary:
Simplify addLifecycleEventListener for the flaky test because we just want to test that listener is working.
Changelog:
[Android][Changed] - Add a spare implementation of addLifecycleEventListener for test purpose.
Reviewed By: PeteTheHeat
Differential Revision: D26749256
fbshipit-source-id: 5af216e6bfa37a15eb189aa24a3df35a7a7112de
Summary:
We want to be able to instrument touch processing delays in JS, which does not have access to systemUptime; therefore we want a UNIX timestamp, which JS has access to and can compare to the touch time.
It only matters that there is relative consistency between multiple touch events in JS, which is still the case; so this should have no impact on product code.
Changelog: [Internal]
Reviewed By: yungsters
Differential Revision: D26705429
fbshipit-source-id: 0f2db726048fcab9a30e830970d7d8a8d2eae446
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056725
fbshipit-source-id: 771e5dd4cd1aeca3d2afd1a67ee58b9ac21eda79
Summary:
The `Event` interface has been improved such that:
1. `getEventData` is now a default method on Event that returns null
2. `dispatch` has a default implementation that relies on getEventName() and getEventData()
3. `dispatchModern` can detect if surfaceId and event data are present; if not, it falls back to dispatch
This will dramatically ease future migrations: at some point in the (distant) future, we can simply delete RCTEventEmitter and
all use-cases will be supported by the current `Event` class without needing to introduce a 3rd transitional interface; and 99%
of all Event classes can be simplified, delet their `dispatch` implementation and need no further work.
At their core, all Events are simply: (1) a name, (2) data, (3) a target (surfaceId and tag). The interface now reflects that but still
allows for flexibility of the data and names being generated on-demand if necessary; but for the vast majority of Event classes, code
will be dramatically simplified.
I also migrate a single Event class, ContentSizeChangeEvent, to use this new method of dispatch.
Changelog: [Android][Changed] Added convenience methods to simplify native Event classes and ease migrations
Reviewed By: mdvacca
Differential Revision: D26043325
fbshipit-source-id: bc308105f7f6e654d45fd156dbf4a2bcbc45819c
Summary:
See title. dispatchV2 was just introduced yesterday, so nothing relies on it yet / it's safe to rename (there are no released versions of RN with `dispatchV2`).
And... dispatchModern is a better name.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26039008
fbshipit-source-id: 009ae721b8caef23a389c33b51f6f8952a6a73da
Summary:
Refactor EventEmitters to take an optional SurfaceId that Fabric will use, and non-Fabric will not.
Migrating touches is a good proof-of-concept for how this could be used generally, and as it turns out, TouchEvent's API is more flexible than most other event APIs (because it uses a dictionary to pass data around, so we can just stuff SurfaceId into it - not efficient, but flexible).
All new APIs are backwards-compatible and designed to work with old-style events, with Fabric and non-Fabric. Native Views that migrate to the new API will be backwards-compatible and get an efficiency boost in Fabric.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26025135
fbshipit-source-id: 5b418951e9d0a3882f2d67398f2aaadac8a3a556
Summary:
Create V2 EventEmitter that surfaceId can be passed into, with a backwards-compat layer, and some debug-only logging to help assist with migration.
Changelog: [Changed][Android] RCTEventEmitter has been deprecated in favor of RCTModernEventEmitter for optimal Fabric support; RCTEventEmitter will continue to work in Fabric, but there are minor perf implications.
Reviewed By: mdvacca
Differential Revision: D26027104
fbshipit-source-id: 784ca092bbc88d19c82f6c42537c34460d96de86
Summary:
This diff fixes a racecondition in registration of event listeners.
mPostEventDispatchListeners is accessed from different threads, most of the times this variable is used to executed the listeners. It is only written during initialization and turn down of the renderer.
changelog: [internal]
Reviewed By: PeteTheHeat
Differential Revision: D25667988
fbshipit-source-id: 1bf95f5193d55a737bad9403206cc3320185b8cb
Summary:
This diff fixes a ConcurrentModificationException that is thrown when registering events in React Native.
This bug was introduced by D22483508 (https://github.com/facebook/react-native/commit/80f13412e548c8666b6ad770e6d3d5c54a717bc2), before event listeners were registered in the NativeModule Thread, now they are registered in the UI Thread.
As part of this diff I change the type of mListeners variable to use CopyOnWriteArrayList instead of ArrayList because this variable is accessed from different threads. This will prevent the exception to happen, but additionally we need to verify if the change of threading made in D22483508 (https://github.com/facebook/react-native/commit/80f13412e548c8666b6ad770e6d3d5c54a717bc2) will cause any other issue (e.g. events not being delivered becuase the listeners are registered too late in the UI Thread)
changelog:[Internal]
Reviewed By: JoshuaGross
Differential Revision: D22599747
fbshipit-source-id: 5c5e46710c4a559badbd713f536e6e6e464fda23
Summary:
This changes how we access the EventDispatcher from the FabricUIManager in bridgeless mode.
Currently, we have implemented a similar API to what we use for Fabric (used in UIManagerHelper): `BridgelessReactContext.getJSIModule(UIManager).getEventDispatcher()`. However, `getJSIModule` does not have a nullable return type, which means that we have to throw an exception if the UIManager can't be found - if, for example, the instance is not initialized yet (or has been destroyed). This is causing crashes when a view tries to access the EventDispatcher before the instance is initialized, which takes longer for Venice because we include JS bundle loading as part of initialization (we may need to revisit that).
Ideally, we'd like to create a first-class API for `getEventDispatcher()`, and make sure that it never crashes if the instance is destroyed, because we don't care if JS events aren't delivered at that point. However, there are some obstacles to making this change for the bridge - mostly related to avoiding circular dependencies between the bridge module and the uimanager module. (Also, this might be a behavior change for the bridge, because I think we currently start queueing events before it's initialized? and product code might be relying on that).
Reviewed By: mdvacca
Differential Revision: D21672949
fbshipit-source-id: a38e96cd40c6f70124b7ca2a5c9722988fe7fcf4
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/28983
Creating an interface for EventDispatcher so I can implement it from a dummy/no-op EventDispatcher to use in bridgeless mode when the instance isn't alive.
Changelog: [Android] [Changed] Renamed EventDispatcher to EventDispatcherImpl and created EventDispatcher interface; calls to EventDispatcher contstructor need to be updated
Reviewed By: makovkastar
Differential Revision: D21695401
fbshipit-source-id: 46066a467efcf03a5f484bb9fb58c662d46a2c4e
Summary:
This should resolve some of the crashes in T9871006. It's unclear if these events are being emitted before Catalyst is set up or after it's torn down, but it's likely a race between teardown and some other event.
Should be safe to just log and continue / fail silently, if this is happening after teardown.
Also upgrading logs to error so that these get logged to logviews.
Changelog: [Internal], will fail silently in a marginal number of existing edge-case crashes for events sent before startup or after shutdown
Reviewed By: mdvacca
Differential Revision: D19269891
fbshipit-source-id: 30fc28693c0be7a15bfc5b5bc7eb88e10c7b4a5a
Summary: Adding `DoNotStrip` to all the interfaces that extend `JavaScriptModule` to ensure they don't get stripped from release builds (because they have no Java implementors).
Reviewed By: emma0303
Differential Revision: D17534719
fbshipit-source-id: a793764caf17040bf1252be7ec4c72176d6989d4
Summary:
This is a easy change in ReactEventEmitter that refactors the TAG used by this class to log events.
This is a follow up of D17422611
Reviewed By: makovkastar
Differential Revision: D17437034
fbshipit-source-id: 9974a244b24b04315c6a328696954b7acf7efbfd
Summary:
This diff forces ReactEventEmitter to use RCTEventEmitter as default event emitter class. This is necessary because of T54134149.
This is a temporary fix until we find the root cause of T54134149
Reviewed By: JoshuaGross
Differential Revision: D17422611
fbshipit-source-id: 159b216434c79466427b7b1a2b0f71f466b0f606
Summary:
This diff refactors ReactEventEmitter to not use the method ReactContext.getJSModule()
Since the initialization of events is performed by UIManagerModule or FabricUIManager classes we don't need to use JSModules as part of this class
Reviewed By: ejanzer
Differential Revision: D17176948
fbshipit-source-id: 6915a74b486851fbeda24f779d97873df22fd79b
Summary:
This diff migrates the usages Nullable and NonNull annotations to AndroidX instead of javax.
The purpose of this change is to bring consistency in the annotations used by the core of RN
Reviewed By: makovkastar
Differential Revision: D16054504
fbshipit-source-id: 21d888854da088d2a14615a90d4dc058e5286b91
Andrei Shikov
Xin Chen
Pieter De Baets
CodemodService FBSourceGoogleJavaFormatLinterBot
Joshua Gross
David Vacca
Ramanpreet Nara
Lulu Wu
Emily Janzer
Andres Suarez
Emily Janzer