Summary:
There are two methods in ReactRootView to handle touch events "onInterceptTouchEvent" and "onTouchEvent", for Venice we have ReactSurfaceView inherits ReactRootView but the implementation for above 2 touch handling methods still calls into it's super implementation which uses the bridge.
In this diff we make ReactSurfaceView inherits ReactRootView's "dispatchJSTouchEvent" and "dispatchJSPointerEvent". So that Venice has separate implementation for touch events handling.
Changelog:
[Android][Changed] - Revamp touch event dispatching methods
Reviewed By: fkgozali, JoshuaGross
Differential Revision: D36629466
fbshipit-source-id: fb7c5950afe6249d22edd3fac3fa5d3b83b3af84
Summary:
As per commit: https://github.com/facebook/react-native/commit/4f3b17412018a10f9293247c802598d2b94a844b which states that "React Native requires that the RootView id be managed entirely by React Native, and will crash in addRootView/startSurface if the native View id isn't set to NO_ID."
This behaviour can not be guaranteed in **hybrid** apps that have a native android layer over which ReactRootViews are added and the native views need to have ids on them in order to work. **The control of views can jump back and forth between native android and react-native (fabric). As the ReactRootView is added to ViewGroups (or layouts) in Android Fragments and Activities, they contain ids on their views which might get passed down to the reactRootView by features like DataBinding**
Hence this can cause unnecessary crashes at runtime for hybrid apps even when they are not changing the id of the reactRootView object they are adding to their ViewGroups.
Our app is a hybrid app that uses both native android and react-native on different screens and on one such screen that has a Fragment adding a ReactRootView to its FrameLayout to render native android views to render in ReactNative, this crash occurs on pressing the back button as well as on unlocking the screen while staying on the same screen.
The app was running fine on more than a 100 million devices on React Native 0.63.4 but after updating to 0.67.2, that features this commit, it crashes on the very first device it was tested on.
Refer to the issue: https://github.com/facebook/react-native/issues/33121 for more information on the crash
The fragment in which this issues arises is like this:
```binding.frameLayout.addView(getReactRootView())```
where getReactRootView() is like this:
```
private var mReactRootView: ReactRootView? = null
private var mReactInstanceManager: ReactInstanceManager? = null
mReactRootView = ReactRootView(context)
if (activity != null) {
val application = activity?.application
if (application is MainApplication) {
mReactInstanceManager = application.reactInstanceManager
}
}
fun getReactRootView():View?{
return mReactRootView
}
```
So converting this to a soft exception such that pure react-native devs can still see the error while hybrid apps continue to run without crashes.
### Snippet of the change:
```
if (getId() != View.NO_ID) {
ReactSoftExceptionLogger.logSoftException(
TAG,
new IllegalViewOperationException(
"Trying to attach a ReactRootView with an explicit id already set to ["
+ getId()
+ "]. React Native uses the id field to track react tags and will overwrite this"
+ " field. If that is fine, explicitly overwrite the id field to View.NO_ID."));
}
```
## Changelog
[GENERAL] [ADDED] - A ReactSoftException log instead of a direct exception being thrown:
```
if (getId() != View.NO_ID) {
ReactSoftExceptionLogger.logSoftException(
TAG,
new IllegalViewOperationException(
"Trying to attach a ReactRootView with an explicit id already set to ["
+ getId()
+ "]. React Native uses the id field to track react tags and will overwrite this"
+ " field. If that is fine, explicitly overwrite the id field to View.NO_ID."));
}
```
[GENERAL] [REMOVED]- Directly throwing an exception even when the code is not responsible for this issue:
```
if (getId() != View.NO_ID) {
throw new IllegalViewOperationException(
"Trying to attach a ReactRootView with an explicit id already set to ["
+ getId()
+ "]. React Native uses the id field to track react tags and will overwrite this"
+ " field. If that is fine, explicitly overwrite the id field to View.NO_ID.");
}
```
Pull Request resolved: https://github.com/facebook/react-native/pull/33133
Test Plan:
This crash is hard to reproduce but when it occurs, this is the only way to fix it.
If any app used to crash with this exact error, it will no longer crash but show an error log in Logcat for developers to be informed about the issue.
Reviewed By: ShikaSD
Differential Revision: D34304212
Pulled By: cortinico
fbshipit-source-id: f0eaeef2e905a6e0587df088b43cc49cabda397a
Summary: This diff adds flag to `ReactRootView` to control if the touch event is to be dispatched to the JS side. This is needed for subclass of `ReactRootView` to control if the dispatch is needed.
Reviewed By: javache
Differential Revision: D35033684
fbshipit-source-id: febab6988cc3e4259e726d03d797dd0ffc978d24
Summary:
Fixes a potential crash was introduced by https://github.com/facebook/react-native/issues/30919 that aimed to get the keyboard height on devices with a Notch. The problem is that it considers that any ReactRootView will have an insets available.
When using [react-native-navigation](https://github.com/wix/react-native-navigation) and assigning a Navigation button to the TopBar as a component, the component gets registered as a RootView but won't have any insets attach to the view.
[getRootWindowInsets()](https://developer.android.com/reference/android/view/View#getRootWindowInsets()) in fact return a `WindowInset` only available if the view is attached, so when executing `checkForKeyboardEvents` method from ReactRootView, is trying to access the `DisplayCutout` of a null object, leading to a crash.
## Changelog
[Android] [Fixed] - Fix potential crash if ReactRootView does not have insets attached.
Pull Request resolved: https://github.com/facebook/react-native/pull/32989
Test Plan:
Without the code change: Notice how the second screen being push contains a React Component on the top right of the navigation bar, and when component is unmounted (going back) the app crashes.
https://user-images.githubusercontent.com/6757047/151558235-39b9a8b5-be73-4c31-8053-02ce188637b8.mp4
crash log
```
2022-01-28 10:27:52.902 15600-15600/com.mattermost.rnbeta E/AndroidRuntime: FATAL EXCEPTION: main
Process: com.mattermost.rnbeta, PID: 15600
java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.DisplayCutout android.view.WindowInsets.getDisplayCutout()' on a null object reference
at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.checkForKeyboardEvents(ReactRootView.java:778)
at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.onGlobalLayout(ReactRootView.java:769)
at android.view.ViewTreeObserver.dispatchOnGlobalLayout(ViewTreeObserver.java:1061)
at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:3214)
at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:2143)
at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8665)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1037)
at android.view.Choreographer.doCallbacks(Choreographer.java:845)
at android.view.Choreographer.doFrame(Choreographer.java:780)
at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1022)
at android.os.Handler.handleCallback(Handler.java:938)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loopOnce(Looper.java:201)
at android.os.Looper.loop(Looper.java:288)
at android.app.ActivityThread.main(ActivityThread.java:7839)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
```
After applying the patch which is only a null check validation and does not change any previous behavior
https://user-images.githubusercontent.com/6757047/151558429-9ff1a608-abb6-4168-8db9-df0c3c71d79e.mp4
Reviewed By: cortinico
Differential Revision: D33844955
Pulled By: ShikaSD
fbshipit-source-id: ed5579ad3afeed009c61cc1851eee45c70087cf5
Summary:
Quick refactor of ReactRootView where I extract 'Fabric' check into a method
changelog: [internal] internal
Reviewed By: fkgozali
Differential Revision: D33269025
fbshipit-source-id: 2647d5870f3b5fcc5a940a5924a39ca43b5bebac
Summary:
Inits RootView to display size for the early surface start experiment.
Before that experiment, we always had the view measured before the surface was initialized, but here layout can sometimes happen before measure. Surface defaults use [0; Inf) for size constraints, potentially causing a crash in Yoga. This change avoids that by making a guess on default layout size with `displayMetrics`
Changelog: [Internal]
Reviewed By: feedthejim
Differential Revision: D33190397
fbshipit-source-id: 3b1b84135a4980ef2fde4024ec84a448199e00b8
Summary:
Changelog: [Internal]
# Context
Whilst looking at Marketplace Loom traces, ShikaSD made the remark that we could potentially start the JS work much earlier instead of waiting for the View.onMeasure call, a behaviour that is already built-in Venice.
Reviewed By: ShikaSD
Differential Revision: D32559505
fbshipit-source-id: cc6337955ad2b6a6581a0347f1f976679eaca54d
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:
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:
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: - This is crashing too much in debug, which is good signal but making it harder to test, and test unrelated features.
Reviewed By: JoshuaGross
Differential Revision: D29857626
fbshipit-source-id: c52cfb6131747ae420b27de0591620fe79f47359
Summary:
See comments in ReactClippingProhibitedView for details and motivation behind this new feature.
You may have a View class inherit from the ReactClippingProhibitedView interface in order to enable this feature for instances of that View type.
This can be added to Views that should /never/ be clipped from the View hierarchy - namely, TTRC components or other telemetry components that always need to be rendered in order for some feature to function.
Changelog: [Added] Opt-in mechanism to allow native Android Views to be marked as "not clippable", soft exceptions will be logged if these Views are clipped from the View hierarchy
Reviewed By: sshic
Differential Revision: D29472439
fbshipit-source-id: b3be53df836b452aed5dc40514ff585ce0ad812b
Summary:
This diff moves the method getViewportOffset out of ReactRootView. This is necessary to avoid Fabric to depend from paper.
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D26716901
fbshipit-source-id: cec67c24860a776fb361d7cda08d3142e1214c8c
Summary:
If there's a JS/native crash and the surface is stopped, the ReactRootView could be reused when the surface is restarted (or potentially to show the errors in a logbox; I'm don't think that's possible, but not 100% sure).
Regardless, now it will crash in those cases because of T83802049. There's no reason to do that, so just mark the ReactRootView as explicitly unused once the surface is stopped.
Changelog: [Internal]
Reviewed By: ShikaSD
Differential Revision: D26469742
fbshipit-source-id: ee79c094393a8ae8b5c53f04c14b90bc8e1a5a17
Summary:
The RootView being managed by Fabric should have an id of View.NO_ID when it is "handed over" to RN. This is true for Fabric and non-Fabric
and setting a custom id on the ReactRootView has never been supported. I'm temporarily (?) adding an additional check earlier and into ReactRootView to hopefully
catch any of these issues early.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26077509
fbshipit-source-id: 59e1ec080504e50698acc654c29120f039238a96
Summary:
In some cases, onMeasure/onLayout are called on the RootView before `startSurface` in Fabric has been able to set surfaceId on the RootView.
With the new SurfaceMountingManager, this causes a crash because we need a valid surfaceId to perform an operation. Before the SurfaceMountingManager refactor, a surfaceId of 0 would be passed to `mBinding.setConstraints` in the FabricUIManager, and setConstraints in C++ noops if there's an invalid surfaceId.
For now, FabricUIManager will also fail silently if the surfaceId is invalid when updateRootLayoutSpecs is called, just to be conservative and to be consistent with previous behavior. This will be upgraded to a hard-crash in the future.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26051266
fbshipit-source-id: ca2d80f899cdba9b3962af68546bd83b77be0680
Summary:
This diff fixes a race condition in the execution of FabricUIManager.StartSurface method.
The rootcause is that startSurface is executing getViewportOffset from a background thread.
changelog: [internal]
Reviewed By: shergin
Differential Revision: D25617154
fbshipit-source-id: 9351201088164e74bb0b9454e30651e1de0da912
Summary:
The root cause of this bug is a race condition between the onMeasure method and setupReactContext.
ReactInstanceManager.attachRootView() method is responsible of the initialization of ReactRootViews and it is invoked by ReactRootView.onMeasure() method in the UIThread
Important initialization steps:
1. Clear the Id of the ReactRootView
2. Add the ReactRootView to the mAttachedReactRoots
3. Call StartSurface (if the bridge has been initialized)
Sometimes, when this method is invoked for the first time, the bridge is not initialized, in those cases we delay the start of the surface.
Once the bridge is initialized, StartSurface is called by the setupReactContext() running in the NativeModuleThread.
Since onMeasure can be called multiple times, it is possible that we call "StartSurface" twice for the same ReactRootView, causing the bug reported on T78832286.
This diff adds an extra check to prevent calling "StartSurface" twice. The fix is done using an AtomicInteger comparison and it is gated by the flag "enableStartSurfaceRaceConditionFix". Once we verify this works fine in production we will clean up the code, remove the flags and maybe revisit the API of ReactRoot.
changelog: [Android] Fix race-condition on the initialization of ReactRootViews
Reviewed By: JoshuaGross
Differential Revision: D25255877
fbshipit-source-id: ca8fb00f50e86891fb4c5a06240177cc1a0186d9
Summary:
This feature flag (enableStopSurfaceOnRootViewUnmount) was used to gate usage of the "stopSurface" API, which is now fully supported, and has been used
in the FB app for several months. This is safe to ship in code.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25275192
fbshipit-source-id: fa22bfd00aa023297bc19c83c138f133e9ff1645
Summary:
Add three markers for ReactRootView perf logging: onMeasure, attachToReactInstanceManager, and updateLayoutSpecs.
It is suspected that one or all of these have regressed under Fabric in some cases.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D24909635
fbshipit-source-id: 6b6c0cc792c4b2d72f2360d6cffc02ef00e8a88b
Summary:
Currently the dimensions are created once, and then cached. This change will reload the dimensions when the device orientation changes to insure that dimension update events follow orientation changed events.
this should help address the following issues, that I know of:
https://github.com/facebook/react-native/issues/29105https://github.com/facebook/react-native/issues/29451https://github.com/facebook/react-native/issues/29323
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[Android] [Fixed] - Dimension update events are now properly sent following orientation change
Pull Request resolved: https://github.com/facebook/react-native/pull/30324
Test Plan: Open up RNTester app. Select the Dimensions API list item. Rotate the device and verify that the dimensions are correct based on orientation.
Reviewed By: fkgozali
Differential Revision: D24874733
Pulled By: ejanzer
fbshipit-source-id: 867681ecb009d368a2ae7b67d94d6355e67dea7b
Summary:
Sometimes stopSurface crashes when unmountReactApplication is called under Fabric; knowing the stack trace up to this point might assist in debugging.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D24532027
fbshipit-source-id: f350e77fb1a2de52eb146b449f1d2f6e960fa017
Summary:
There are cases under investigation where unmountReactApplication is called before the ReactRootView gets an ID; in some or all of those cases, UIManagerBinding.stopSurface cannot get the ReactFabric JSI module and crashes there.
It's still unclear why `unmountReactApplication` is being called in these circumstances - maybe another crash causing React Native teardown?
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D24214712
fbshipit-source-id: 796594653f4ff6d87088c4841b89f06cc873b46f
Summary:
In D23640968 (https://github.com/facebook/react-native/commit/78b42d7fb7180102c1e8ec917dcccd2d9d4076db) I introduced a mechanism to update offsetX/offsetY whenever onMeasure/onLayout were called, to ensure that `measureInWindow` had the correct metrics and would work properly.
However, now `uiManager.updateRootLayoutSpecs` gets spammed and is called too often. For example, whenever a TextInput is focused/blurred, `uiManager.updateRootLayoutSpecs` may be called 5+ times even though
the measure specs/offsets may only change once.
Thus, we just compare with previous values before calling into the UIManager. This should give us a very small perf improvement.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D24176867
fbshipit-source-id: f0dcc816e651a843607e9e5d40d8f3489894d4ba
Summary:
Changelog: [internal]
# Why is text laid out twice in Fabric?
Layout constraints (min and max size) change during startup of Fabric surface.
1. `Scheduler::startSurface` is called with max size being {inf, inf}.
2. `Scheduler::constraintSurfaceLayout` is called with max size equal to viewport.
These are two operations that don't happen one after the other and on Android, CompleteRoot is called from JS before second operation is called. This triggers layout with max size {inf, inf} and later when second operation is called. Layout happens again now with correct size.
# Fix
Make sure `Scheduler::startSurface` is called with proper values and not {inf, inf}.
Reviewed By: JoshuaGross, yungsters
Differential Revision: D23866735
fbshipit-source-id: b16307543cc75c689d0c1f0a16aa581458f7417d
Summary:
In the past, in Fabric (Android), we never called stopSurface. Ever! This is bad for memory and can cause other issues, so... let's not do that.
Instead, when the ReactRootView is being torn down, we check the View ID to see if it's a Fabric RootView, and if it is, we call Fabric's stopSurface method.
Fabric stopSurface only has impact on (1) Fabric mount instructions being executed after that point and (2) tells JS to stop running the surface, on the JS thread, asynchronously.
Anecdotally it looks like all the MountItems involved in deleting and removing views from the hierarchy are executed before stopSurface is called.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21965837
fbshipit-source-id: 5169c5a1e339fd9e016ae1234d8389b2bd30be70
Summary:
Removing logs related to T62192299 that we don't need anymore.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21773629
fbshipit-source-id: d16c01f87be3ed7512fe90b6e261b4c7efbd3068
Summary:
This diff removes the variable 'useSurface' which wasn't beeing used in ReactRootView anymore
changelog: [Internal][Android] Internal cleanup of ReactRootView
Reviewed By: JoshuaGross
Differential Revision: D21318109
fbshipit-source-id: f850b27811608d16b22b6a3964455b172705c4c7
Summary:
This is the codemod portion of the parent diff.
I modified all call-sites to `ReactContext.getNativeModule` to do a null check on the returned NativeModule.
Changelog:
[Android][Fixed] - Check if NativeModules returned from CatalystInstanceImpl.getNativeModule are null before using them.
Reviewed By: JoshuaGross
Differential Revision: D21272028
fbshipit-source-id: 6bd16c6bf30605f2dfdf4c481352063712965342
Summary:
Fabric debugging.
I also have an agenda of removing all instances of `ReactConstants.TAG`, because it's overly broad and not helpful during debugging.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D20016401
fbshipit-source-id: 91e5cd7e8eb662b1a4a887c1e4e66a31ec741b46
Summary:
This diff refactors the usages of UIManagerHelper.getUIManager() to make sure we always consider null objects.
Some of the callsites were throwing a NullPointerExcetpion, now they throw a more explicit exception.
changelog: [internal]
Reviewed By: makovkastar
Differential Revision: D19383064
fbshipit-source-id: 1806a37528e80cab1c8fdff5eb631aaf47bde819
Summary:
ReactRootView currently caches the last seen DisplayMetrics so we can compare them against the current values in DisplayMetricsHolder to determine if the screen dimensions have changed. (If they have changed, we emit an event to notify JS of the new dimensions).
However, ReactRootView initializes these member variables with empty DisplayMetrics, which means that the first time we check against them in onGlobalLayout, we will *always* emit an event to JS.
This seems reasonable if you only have one ReactRootView, but if you create a new RRV for each RN screen in your app, then you're going to get updated dimensions on each navigation event, even though the screen dimensions have probably not changed.
In this diff, I'm no longer storing the DisplayMetrics in ReactRootView at all, but instead am using temporary variables to check the new DisplayMetrics values against the old.
Changelog: [Android][Fixed] Only update dimensions in ReactRootView if they've changed
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D19395326
fbshipit-source-id: c01aee73064764503c9b49208032c790b83a1d29
Summary:
This diff exposes the surfaceID as part of ReactRootView.
Changelog: [internal]
Reviewed By: JoshuaGross
Differential Revision: D18474014
fbshipit-source-id: 98f651b211d3cc4df88c6b1eb257bc12129eff57
Summary:
Fixes T54997838 by preventing any view mutations during `onMeasure` calls.
There might still be places where this is possible, but this is where I'm seeing all the crashes currently.
See comments in ReactRootView for why views were mutated during onMeasure.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D18518591
fbshipit-source-id: 1406af8a6b0bfcc86f4cc5b451b3967f312dfd85
Summary:
Add UI thread assertions and annotations to ReactRootView. Shouldn't have any immediate effect since these methods already call other methods that assert they're on the UI thread. Doing this to hoist assumptions higher up.
Doing some very simple refactoring to the way cleanup happens to ensure aggressive cleanup in more instances.
Reviewed By: shergin
Differential Revision: D17860291
fbshipit-source-id: c87e0336594fa2327271b648eb8340e250235250
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
Summary:
This diff formats the Java class files inside xplat/js/react-native-github. Since google-java-format was enabled in D16071401 we want to codemode the existing code so that users don't have to deal with formatter lint noise at diff-time.
```arc f --paths-cmd 'hg files -I "**/*.java"'```
drop-conflicts
Reviewed By: cpojer
Differential Revision: D16071725
fbshipit-source-id: fc6e3852e45742c109f0c5ac4065d64201c74204
Summary:
Every call site is either already using `createReactContextInBackground` correctly or guarding the invocation using `hasStartedCreatingInitialContext`. This is an unnecessary and overly complex dance that can be simplified.
This revision simplifies the use of `createReactContextInBackground` by integrating the check. This is not a breaking change.
Reviewed By: zackargyle, mdvacca
Differential Revision: D15566632
fbshipit-source-id: 7b50285c9ac6776d1297d2c9c53dff208851b722
Summary:
We currently have two different codepaths for actually rendering a surface with Fabric on iOS and Android: on iOS we use Fabric's `UIManagerBinding.startSurface` to call `AppRegistry.runApplication`, but on Android we don't; instead we use the same codepath as paper, calling `ReactRootView.runApplication`.
This diff does a few different things:
1. Unify iOS and Android by removing the `#ifndef` for Android so that we call `startSurface` for both
2. Pass through the JS module name on Android so that this actually works (it currently passes in an empty string)
3. Remove the call to `ReactRootView.runApplication` for Fabric so that we don't end up doing this twice
4. Copy over some logic that we need from `ReactRootView.runApplication` (make sure that root layout specs get updated, and that content appeared gets logged)
Reviewed By: mdvacca
Differential Revision: D15501666
fbshipit-source-id: 5c96c8cf036261cb99729b1dbdff0f7c09a32d76