Summary:
This PR slightly improves the implementation of `dispatchCommand` method of `UIManagerBinding` to use existing variable `shadowNode` instead of calling `shadowNodeFromValue` again.
## Changelog
[INTERNAL] [CHANGED] - Eliminated double call of `shadowNodeFromValue` in `dispatchCommand`
Pull Request resolved: https://github.com/facebook/react-native/pull/35695
Test Plan: Launch RNTester with Fabric enabled and check if `scrollTo` or some other command works properly.
Reviewed By: christophpurrer
Differential Revision: D42233216
Pulled By: robhogan
fbshipit-source-id: db152206060ff599962f47c43fda8ea797f2a8cb
Summary:
changelog: Introduce setNativeProps to Fabric
Add support for `setNativeProps` in Fabric for backwards compatibility. It is still recommended to move away from `setNativeProps` because the API will not work with future features.
We can make step [Migrating off setNativeProps](https://reactnative.dev/docs/new-architecture-library-intro#migrating-off-setnativeprops) in migration guide optional.
Reviewed By: yungsters, mdvacca
Differential Revision: D41521523
fbshipit-source-id: 4d9bbd6304b8c5ee24a36b33039ed33ae1fc21f8
Summary:
changelog: [internal]
Add findShadowNodeByTag_DEPRECATED` to Fabric's UIManager.
This API makes it possible to get ShadowNode reference for given native tag.
We will leverage this API to make UIManager.measure* calls backwards compatible in the new architecture.
Reviewed By: yungsters
Differential Revision: D41550734
fbshipit-source-id: a45b6a76634d034eac70603bad8ac60f9135ff24
Summary:
We want to fix the order of execution of layout, layout effects and passive effects in React Native, but the use of the Fabric background executor for layout complicates this (and other things).
This brings back a flag to disable this background thread to do layout synchronously in JavaScript, before the execution of layout effects and passive effects.
This is expected to regress performance on some screens, so we need to address the antipatterns in those screens before shipping this.
Changelog: [internal]
Reviewed By: javache
Differential Revision: D39727131
fbshipit-source-id: 4323b089234d3304ca3bfe5697668fb44ac64c12
Summary:
Changelog: [Internal] Remove unused RuntimeExecutor from UIManagerBinding constructor
Since [UIManager is the class that has a reference to RuntimeExecutor](https://github.com/facebook/react-native/blob/ce50c43986bae05ad62552be46f4d5bb4a46f097/ReactCommon/react/renderer/uimanager/UIManager.h#L35-L36), there's no reason for `UIManagerBinding` to own a reference to RuntimeExecutor too.
The purpose of `UIManagerBinding` is to bind the Fabric `std::shared_ptr<UIManager>` in C++ to `global.nativeFabricUIManager` in JS.
Doing this cleanup to make the next diff easier.
Reviewed By: sammy-SC
Differential Revision: D39490927
fbshipit-source-id: 4c393dccc6985bef44e3b0afc32b9f79bea25f80
Summary:
changelog: [internal]
Add more clang tidy rules to prevent common class of bugs.
Reviewed By: javache
Differential Revision: D39245194
fbshipit-source-id: 5521c5c4653d7005b96ebba494e810ba5075afbc
Summary:
Fix todo and inconsistency across codebase. It's better to have just one way to refer to these types.
Changelog: [Internal]
Reviewed By: philIip
Differential Revision: D37653146
fbshipit-source-id: e82f09caa6cd6eec5512b78f413708d9c04a7a83
Summary:
Adding more context for event dispatching process in systrace.
Changelog: [Internal]
Reviewed By: philIip
Differential Revision: D35208257
fbshipit-source-id: 4a70e15a0074d4a53a895066e6fa1e60a6ebda0d
Summary:
changelog: [internal]
pass raw ShadowNode instead of shared_ptr. Ownership is not transferred, shared_ptr is misleading.
Reviewed By: javache
Differential Revision: D33917010
fbshipit-source-id: 4d9fdd4b4e0376149f1719ad160b957de4afdce3
Summary:
changelog: [internal]
You can read more about this rule on https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
# Isn't it wasteful to copy? Isn't reference more efficient?
This rule of thumb is no longer true since C++11 with move semantics. Let's look at some examples.
# Option one
```
class TextHolder
{
public:
TextBox(std::string const &text) : text_(text) {}
private:
std::string text_;
};
```
By using reference here, we prevent the caller from using rvalue to and avoiding copy. Regardless of what the caller passes in, copy always happens.
# Option two
```
class TextHolder
{
public:
TextBox(std::string const &text) : text_(text) {}
TextBox(std::string &&text) : text_(std::move(text)) {}
private:
std::string text_;
};
```
Here, we provide two constructors, one for const reference and one for rvalue reference. This gives the caller option to avoid copy. But now we have two constructors, which is not ideal.
# Option three (what we do in this diff)
```
class TextHolder
{
public:
TextBox(std::string text) : text_(std::move(text)) {}
private:
std::string text_;
};
```
Here, the caller has option to avoid copy and we only have single constructor.
Reviewed By: fkgozali, JoshuaGross
Differential Revision: D33276841
fbshipit-source-id: 619d5123d2e28937b22874650366629f24f20a63
Summary:
changelog: [internal]
Just moving code that doesn't belong to UIManagerBinding out of the class.
Reviewed By: philIip
Differential Revision: D33060412
fbshipit-source-id: 2d54929072cef14fd1fa6b70bde382ae21ecff45
Summary:
Applies suggestions from default preset that make sense in our codebase
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D33010231
fbshipit-source-id: 6bc9edf01fd9bd9938d211e3494dd1a127b24eaa
Summary:
changelog: [internal]
Provide `UIManager` to `UIManagerBinding` in constructor to make the API safer.
Reviewed By: philIip
Differential Revision: D32668892
fbshipit-source-id: a15cd295196a60c3f46997e59c05c4f90503e18d
Summary:
changelog: [internal]
Exposes a new flag on RuntimeScheduler: `unstable_getIsSynchronous`. Flag indicates if the current code is run synchronously and therefore commit phase should be synchronous.
Unit tests will be added later, to keep this diff short. This code path is not executed yet.
Reviewed By: mdvacca, ShikaSD
Differential Revision: D32677814
fbshipit-source-id: e01d4fff7e716d627ff99fe104965851138c3aef
Summary:
changelog: [internal]
Background executor has been shipped on both platforms for a long time.
I've kept the flag around because I wanted to run tests and compare Concurrent Mode vs Background Executor. The intention was to see if we can get rid of Background Executor to simplify the threading model.
Since then, React team has moved away from Concurrent Mode towards more gradual rollout of concurrent rendering and it no longer makes sense to do this comparison. Right now, we don't have a concern with concurrent rendering and Background Executor. If we ever want to run the an experiment, this gating will need to be added again.
Reviewed By: javache
Differential Revision: D32674798
fbshipit-source-id: a1e51c9c5b8e48efa4cb0f25379d58e7eb80ccd9
Summary:
changelog: [internal]
This is a pre-condition to get rid of `shared_ptr` from `EventEmitterWrapper`. Also saves us a few copies of shared_ptr, this is negligible though.
Reviewed By: mdvacca
Differential Revision: D31307048
fbshipit-source-id: b84654bed2359b66faf3995795e135e88fe51cb6
Summary:
This diff exposes a new API in FabricUIManager called getInspectorDataForInstance. The goal of this method is to return React metadata for a Native view.
This data will be used from partner teams to build tools that uses React metadata in RN
Data returned from React: P429701924
changelog: [internal] internal
allow-large-files
Reviewed By: JoshuaGross
Differential Revision: D29747864
fbshipit-source-id: 8cb55573be08cb530f7e3c83eed8b4fcf43e7781
Summary:
I'm hunting down the source of a perf regression on a screen and think that having these systrace sections could be handy for this and future investigations.
Changelog: [internal]
Reviewed By: mdvacca
Differential Revision: D29802969
fbshipit-source-id: f4030261da8888ddeb32ae41b9cf2b25af6a5583
Summary:
In the refactor in D28933824 (https://github.com/facebook/react-native/commit/6b601db8b83337415113a123d780fb64aa67af42), an early return was accidentally deleted. This causes incorrect measurements to be returned, which can break a few things including Text Inline Views.
Changelog: [internal]
Reviewed By: mdvacca
Differential Revision: D29122235
fbshipit-source-id: 76c8991132e22cd4e0fc1c277447c4dba751adfb
Summary:
Changelog: [internal]
Add an option to make all measure calls asynchronous.
Reviewed By: JoshuaGross
Differential Revision: D28934444
fbshipit-source-id: 57a320b03add0182b4646b13ed4b692b899ddea3
Summary:
Changelog: [internal]
Move call of getRelativeLayoutMetrics to lambda.
The reason for this is so we can make all of "measure" calls asynchronous.
Reviewed By: JoshuaGross
Differential Revision: D28933824
fbshipit-source-id: f77fc1d06d5cd4706d52a02253a3e754e8e8fa44
Summary:
Changelog: [internal]
Use getter function instead of accessing ivar. This makes for clearer APIs.
React part is implemented in https://github.com/facebook/react/pull/21553
Reviewed By: JoshuaGross
Differential Revision: D28675372
fbshipit-source-id: cf99f8482067bfc0fd57d39fda9656abd665bb69
Summary:
This diff extends startSurface and setSurfaceProps methods with the new parameter called displayMode
changelog: [internal] internal
Reviewed By: yungsters
Differential Revision: D27669847
fbshipit-source-id: c2ddb690ca897e46e00f07b491b91bb2bc8e847d
Summary:
This diff introduces a new fabric API called setSurfaceProps that will be used to call the new JS api "setSurfaceProps"
changelog: [internal] internal
Reviewed By: sammy-SC
Differential Revision: D27607588
fbshipit-source-id: 36a19f728af244d7e72687d9305b1c568e2b8ec6
Summary:
Changelog: [internal]
Add support for a return value when calling JavaScript functions in UIManagerBinding.
Reviewed By: mdvacca
Differential Revision: D27470817
fbshipit-source-id: 38de92dd913af61d879f1cc5962d417f51da73b0
Summary:
The non-Fabric API has a `blockNativeResponder` param in setJSResponder. Make sure to pass that along in Fabric.
On Android this allows us to respect the flag and do the same thing non-Fabric was doing if `blockNativeResponder` is false. It's not clear yet what the impact is on iOS.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27058806
fbshipit-source-id: aa5074fa46191d78f5292a93d9040ab4bb58ca66
Summary:
Right now there are places in UIManagerBinding.cpp where native java exceptions will be thrown if calling JSI functions results in errors, such as:
```Trying to report error getPropertyAsObject: property '__fbBatchedBridge' is undefined, expected an Object
Error: getPropertyAsObject: property '__fbBatchedBridge' is undefined, expected an Object
```
https://www.internalfb.com/intern/logview/details/facebook_android_javascripterrors/358181062b47b9561e60427bbb3816a9
In this diff I added LOG(ERROR) and checks because:
1, Throwing errors neither prevents the JSI errors nor handles them properly, checks prevent JSI errors from happening.
2, Errors are aggregated in Logview with other JSI errors as "Error: android_crash:com.facebook.react.devsupport.JSException:facebook::jni::throwNewJavaException" which keeps the SLA task open forever, checks can prevent JSI errors so they won't lead to exceptions, and LOG(ERROR) will make sure we have enough info for debugging.
Changelog:
[General][Changed] - Add checks and logs to for better error handling
Reviewed By: JoshuaGross
Differential Revision: D26885562
fbshipit-source-id: c0c1c057342e9efc0ff708188703f4332036e7a9
Summary:
Changelog: [internal]
UIManager shouldn't be retained in lambda because if it outlives runtime it causes a crash.
UIManager -> ComponentDescriptorRegistry -> ParagraphComponentDescriptor -> TextLayoutManager's cache -> AttributedString::Fragment -> ShaviewView -> EventEmitter -> EventTarget -> Pointer
This is the chain of ownership that can cause a crash if UIManager is retained for longer than JS runtime.
{F459238235}
Reviewed By: JoshuaGross
Differential Revision: D26851194
fbshipit-source-id: 644cdee34ba9286618659d847fb2e78bc301d049
Summary:
Changelog: [internal]
Check for null before passing it to `EventTarget` constructor.
Reviewed By: JoshuaGross
Differential Revision: D26605779
fbshipit-source-id: a2773c8123d83c25736bccefe656d1def8794091
Summary:
Changelog: [internal]
`shadowNodeFromValue` can return nullptr. Let's make sure it returns valid value before dispatching command.
Reviewed By: JoshuaGross
Differential Revision: D26605350
fbshipit-source-id: eb9a0347c95ba07fd7e9b7ddeca7e6d6011f50ad
Summary:
Changelog: [internal]
### Why does the crash happen?
The crash can happen if runtime is destroyed before background executor lambda is run. Destroying a shadow node after runtime leads to a crash through the chain of ownerships.
Chain of ownership:
`ShadowNode -> ShadowNodeFamily -> EventEmitter -> EventTarget -> Pointer`
Pointer tries to call `invalidate` method on raw pointer to the runtime which is gone.
https://www.internalfb.com/intern/diffusion/FBS/browse/master/xplat/js/react-native-github/ReactCommon/jsi/jsi/jsi.h?commit=2ee3ae0c6a64&lines=335-339
To work around this, weak pointers are passed to lambda. This way the lambda is less likely to be the last owner of shadow nodes. Possibility of race still exists but it less likely to happen.
## Other solution
Alternatively, we could make sure native Runnable queue in Java is emptied as part of tear down process. We can even implement both solutions as they are semantically correct.
Reviewed By: shergin
Differential Revision: D26582554
fbshipit-source-id: b1b8a92237902bc4c40376176f575caa24a41a05
Summary:
See react_native_assert.{h,cpp}. Because of the BUCK+Android issue where NDEBUG is always defined, we use react_native_assert instead of assert to enable xplat asserts in debug/dev mode.
This migrates most of the codebase, but probably not 100%. The goal is to increase assertion coverage on Android, not to get to 100% (yet).
Changelog: [Internal]
Reviewed By: RSNara
Differential Revision: D26562866
fbshipit-source-id: a7bf2055b973e1d3650ed8d68a6d02d556604af9
Summary:
This is a Fabric-compliant implementation of `JSResponder` feature. To make it work e2e we also need to update FabricRenderer in React repository. But before we can do this, we need to ship the native changes.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D24630027
fbshipit-source-id: 70c30e1250b554d83862956b536714704093072f
Summary:
Changelog: [internal]
Name `shouldCancel` is misleading. It implies the commit is cancelled and doesn't happen.
`shouldYield` expresses the intent better, because the commit's changes do eventually reach mounting layer but the current commit is rather yielding to the next one.
Reviewed By: mdvacca
Differential Revision: D26126121
fbshipit-source-id: 18988f217cbc651f0010f6e2381682bdbaed8bd4
Summary:
Changelog: [internal]
In theory, a commit from different surface could cancel a commit for unrelated surface. Adding surfaceId to cancellation logic assures this doesn't happen.
Reviewed By: shergin
Differential Revision: D26126003
fbshipit-source-id: 15b9b73f1000a2b4ae882e0a5129fe26fbb53fd2
Summary:
Changelog: [internal]
This simplifies logic, there is no need to keep the counter as an ivar of UIManager.
Reviewed By: JoshuaGross, shergin
Differential Revision: D26125928
fbshipit-source-id: bd266586463a9f9b85d6dc189cdab19f79e3d107
Summary:
This changes how we access UIManagerBinding in Fabric code. Instead of storing a pointer to it and accessing it, now we just request it from JavaScript runtime on demand. After a week of testing, I hope we will be able to simplify retaining and referencing logic between UIManager and UIManagerBinding. The change is gated just in case to have the ability to turn this new implementation off.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D26082788
fbshipit-source-id: 211c551fe1aaba9b0bff18478e40e1e1f32e999c
Summary:
This is a small simplification of the logic we have in UIManagerBinding choosing the way to call `completeRoot`. It removes an additional check for for `sharedUIManager->backgroundExecutor_` not being empty. We don't need it because it never changes and we already have the same check several lines above.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D25953674
fbshipit-source-id: 683b3e51a792160d480551a65c40492ce28938e4
Summary:
This is the core (Cxx) Fabric implementation of sendAccessibilityEvent. No platform support is added in this diff (Android and iOS do not build yet).
See following diffs.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25852412
fbshipit-source-id: 88b7d8e2e251183dfe7fb377a4cffbac8f458656
Summary:
Changelog: [internal]
Previous implementation of "measure" in UIManagerBinding returned 0, 0 for x and y coordinates.
Reviewed By: shergin
Differential Revision: D25681586
fbshipit-source-id: fa69b6d4803f083a8299e00cae8bb59932c8bf78
Summary:
This fix is still a little hypothetical. We have a few different JS errors that we're seeing with bridgeless mode that seem to be caused by Fabric trying to access `__fbBatchedBridge` from C++. I think what's happening is:
1. User encounters an unrelated JS error very early in rendering a new surface (possibly while the bundle is still loading?)
2. In release builds, BridgelessReactFragment handles the error by stopping the surface and rendering a retry button (actually, the surface is stopped in a bunch of places in BaseFbReactFragment, which might be why this is popping up now - I recently refactored that class to share more of its logic in bridgeless mode)
3. Fabric stops the surface by first checking to see if the custom binding `RN$stopSurface` exists; if not, it falls back to calling the registered callable module `ReactFabric`.
I think #3 is where things are going wrong for bridgeless mode; if you call stopSurface before `RN$stopSurface` is installed (which happens when ReactFabric shim is required) then you'll fall back to the bridge version.
My solution here is to instead rely on a flag set in C++ to determine whether we're in bridgeless mode, and then check to see if the stopSurface binding has been installed. If not, we just noop - if the ReactFabric shim hasn't been required, we probably don't actually have a React surface that needs to be stopped.
At least, that's my current theory. We'll see if this actually works.
Changelog: [Fixed][iOS] Fix an issue calling stopSurface in bridgeless mode before surface is started
Reviewed By: mdvacca
Differential Revision: D25453696
fbshipit-source-id: bff76675c43989101d0ba5ae0aba60089db230bf
Summary:
Changelog: [internal]
Background executor performs unnecessary operations when second `completeRoot` message from React arrives before first `completeRoot` was finished. This produces unnecessary `ShadowViewMutations`.
Mechanism:
Everytime `completeRoot` is received, before the call is dispatched on the background queue, `completeRootEventCounter_DO_NOT_USE_` is incremented.
Inside `ShadowTree::tryCommit` we check if the value has been incremented to determine if another `completeRoot` is queued.
Reviewed By: JoshuaGross
Differential Revision: D24419160
fbshipit-source-id: 11e19026feca01db6c8981b093a691a6b58a006f
Tomek Zawadzki
Samuel Susla
Rubén Norte
Paige Sun
Samuel Susla
Pieter De Baets
Xin Chen
Andres Suarez
Andrei Shikov
David Vacca
Joshua Gross
Peter Argany
Lulu Wu
Valentin Shergin
Emily Janzer