Summary:
This diff refactors the initial render of Fabric in order to set the layout metrics as we start the surface.
This prevents to create an additional fabric commit during initial render. Also this migth help fixing T63495589 (I wasn't able to repro T63495589 again when using this diff)
changelog: [Internal][Android] Internal change to reduce the amount of commits during initial render of Fabric
Reviewed By: JoshuaGross
Differential Revision: D21330072
fbshipit-source-id: 758c49b52ea4c12d5623b7c7d68c7318f4a6cd83
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 diff reduces exposure of UIManagerModule in the NativeAnimatedNodesManager class, this is necessary to enable NativeDriverAnimations in Venice
changelog: [Internal][Android] Internal change to enable native driver animations in RN bridgless mode
Reviewed By: ejanzer
Differential Revision: D21317629
fbshipit-source-id: 81cd4ade296de4757acefe566e1466154d6b4e4b
Summary:
The AndroidInfoModule class defines a `getServerHost()` method that duplicates the logic in `AndroidInfoHelpers.getServerHost(context)`. This commit makes AndroidInfoModule call into AndroidInfoHelpers so that potential future changes to `AndroidInfoHelpers.getServerHost` don't need to be duplicated in `AndroidInfoModule.getServerHost`.
## Changelog
[Android] [Changed] - Internal change to make `PlatformConstants` use the same method to determine `ServerHost` as other code paths
Pull Request resolved: https://github.com/facebook/react-native/pull/28756
Test Plan: Tested by running the RNTester app and editing the root component to print out `NativeModules.PlatformConstants.getConstants()` and verified one of the properties was: `"ServerHost": "10.0.2.2:8081"`.
Differential Revision: D21252158
Pulled By: shergin
fbshipit-source-id: b460197e5f1d972a5b91991c32a929294e358d9f
Summary:
Task is closed, I'm removing the TODO. I verified that the soft error is not being triggered
changelog:[Internal][Android] Internal cleanup of fabric android
Reviewed By: sammy-SC
Differential Revision: D21303999
fbshipit-source-id: 11d4a14c71d27ffc70a6d3b955a21085dd11859b
Summary:
This diff rejects the promise when if an error occurs while processing prefetching result, it applies the same concept as other methods in this class (e.g. see getSizeWithHeaders)
changelog: [internal][Android] Internal change in RN Image prefetching
Reviewed By: JoshuaGross
Differential Revision: D21295612
fbshipit-source-id: c3675e5f2d9c8e38094a538b388ff63a6ea18360
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/28779
Creating a JNI wrapper class for RuntimeExecutor so we can pass it to Fabric and TurboModules in Java.
Changelog: [Internal]
Reviewed By: RSNara
Differential Revision: D21049385
fbshipit-source-id: f833004225d9837acf6ffafd3988f89748cf12aa
Summary:
When I initially made TurboModuleManager.getModule thread-safe, I unintentionally broken TurboModule cleanup. This diff fixes that mistake.
## Mistake
```
Override
public void onCatalystInstanceDestroy() {
synchronized (mTurboModuleCleanupLock) {
mTurboModuleCleanupStarted = true;
}
final Set<String> turboModuleNames = new HashSet<>(mTurboModuleHolders.keySet());
for (final String moduleName : turboModuleNames) {
// Retrieve the TurboModule, possibly waiting for it to finish instantiating.
final TurboModule turboModule = getModule(moduleName); // ERROR!
if (turboModule != null) {
// TODO(T48014458): Rename this to invalidate()
((NativeModule) turboModule).onCatalystInstanceDestroy();
}
}
```
Before calling `getModule(moduleName)`, I set `mTurboModuleCleanupStarted = true`. This assignment makes all calls to `getModule` return `null`, which means that none of the TurboModules were having their `onCatalystInstanceDestroy()` method invoked.
Changelog:
[Android][Fixed] - TurboModule cleanup
Reviewed By: fkgozali
Differential Revision: D21290582
fbshipit-source-id: 3645b9a4f8c6f41498ebd9d51f9b5775edf2dbd7
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:
## Description
When the TurboModule and the NativeModule systems are alive at the same time, after RN cleanup, if a TurboModule is required, we return `null` from TurboModuleManager. This causes `CatalystInstanceImpl.getNativeModule` to do a lookup on NativeModule registry, which throws an `AssertionError`, because the TurboModule isn't found. Instead of throwing an `AssertionError` from `CatalystInstanceImpl.getNativeModule`, this diff instead has that method return `null` in this particular case.
## Rationale
This should eliminate the crashes we're seeing in T46487253.
## Future action
In the future, we should guard `CatalystInstanceImpl.getNativeModule` with an `if (mDestroyed) return null;` statement. This'll ensure that if NativeModules are required after React Native has started cleanup, they'll be returned as `null`. Right now, we either return the destroyed NativeModule object, or create/initialize the NativeMoulde and return it, which is wrong.
Changelog:
[Android][Changed] - Make CatalystInstance.getNativeModule nullable
Reviewed By: JoshuaGross
Differential Revision: D21272029
fbshipit-source-id: 099ad9ab9fa2146299df4cf7f86ae7a8e526bb15
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/28730
Moving RuntimeExecutor out of react/utils and into its own subdir of ReactCommon. I'm doing this because I'm going to be pulling this into CatalystInstance on Android, and I don't want to pull in all the files we don't need there; also, this should hopefully make the OSS NDK stuff easier (this uses the react/utils prefix to export, and I'm not sure if you can include a '/' in a gradle module name?)
Changelog: [Internal]
Reviewed By: shergin, mdvacca
Differential Revision: D21098528
fbshipit-source-id: 9fbd72901ece522b1caec3ec34fafb8f9b327275
Summary:
On Android, when using split-screen mode, the Window-relative position of a native View is not calculated properly when the app is running on the right (or bottom) half of the screen. The coordinates are currently calculated only based on View.getLocationOnScreen() with subtracting status bar height. Scenarios, where the window does not fill the entire screen (such as split-screen mode) are not supported.
We need to use a more general solution to subtract the actual position of the window from position of the view within the screen. This PR fixes the issue by subtracting coordinates of the Window retrieved from View.getWindowVisibleDisplayFrame(), which covers all scenarios when Window can be in a different position than the screen (incl. status bar offset).
## Changelog
[Android] [Fixed] - Calculating view position within the window in split-screen mode
Pull Request resolved: https://github.com/facebook/react-native/pull/28449
Test Plan:
1. Run an app in split-screen mode on the right half of the screen
2. Call UIManagerModule.measureInWindow() from JS to fetch a position of a View within a Window
3. Observe the wrong coordinates returned
4. Try the same with the fix
Reviewed By: mdvacca
Differential Revision: D21246297
Pulled By: shergin
fbshipit-source-id: 1f54b1a5d6610be17bf05521200304db2ba263ab
Summary:
According to the Flow types, `contentOffset` is nullable. Support that.
Changelog: [Internal] Fix to (1) support null contentOffset in ScrollView and HorizontalScrollView, added on Android after the last release. (2) Correctly add support for contentOffset in ScrollView (I missed that when adding it to HorizontalScrollView in the previous diff).
Reviewed By: alsun2001
Differential Revision: D21243028
fbshipit-source-id: ebef9a9054a3e4dd88556739e836b7ece48fda12
Summary:
This diff removes unnecessary (int) casts in the calculation of layout for TextInlineViews
changeLog: [Internal][Android] Internal optimization on the calculation of layout for TextInlineViews
Reviewed By: JoshuaGross
Differential Revision: D21211532
fbshipit-source-id: 920c1f88d042f3e1f6bfd0f560371f7482a62064
Summary:
Fixes text layout so that the parent bounds are correctly respected. This fixes two bugs:
- **Parent width is not respected.** This was caused by the recent change to shrink-wrap text layout.
- **Parent height is not respected.** This has always been a bug.
After this change, Android will behave like iOS.
Changelog:
[Android] [Fixed] - Text layout no longer ignores parent bounds
Reviewed By: mdvacca
Differential Revision: D21199030
fbshipit-source-id: cc072bdcff64167db1f79b7bf965e57a7396cdf4
Summary:
For a very long time, iOS has supported the `contentOffset` property but Android has not:
https://github.com/facebook/react-native/issues/6849
This property can be used, primarily, to autoscroll the ScrollView to a starting position when it is first rendered, to avoid "jumps" that occur by asynchronously scrolling to a start position.
Changelog: [Android][Changed] ScrollView now supports `contentOffset`
Reviewed By: mdvacca
Differential Revision: D21198236
fbshipit-source-id: 2b0773569ba42120cb1fcf0f3847ca98af2285e7
Summary:
Updating state with a null wrapper is neither desirable, nor possible. The underlying task was closed, just cleaning up comments.
Changelog: [Internal] comments only
Reviewed By: mdvacca
Differential Revision: D21186545
fbshipit-source-id: d14ddd59d42e8fd91c6e7fd50037311d4e8d0b60
Summary:
folly_futures was compiled with and exported -DFOLLY_MOBILE=1, while
folly_json did not. This flag disables fancy F14 data structures for
folly::dynamic in favor of a simple std::unordered_map.
This caused inlined/templated code from modules depending on
folly_futures to disagree with the implementations in folly_json,
leading to a crash.
The only such libraries were libhermes-inspector and (transitively)
libhermes-executor-debug, and these only use folly::dynamic for CDP
serialization, which is why the problem was not more apparent.
Changelog: [Internal] Fix crash when attaching a Hermes debugger
Reviewed By: mhorowitz
Differential Revision: D21193307
fbshipit-source-id: 2b795bb6f4f7f991e2adaacec62d62616117322b
Summary:
Changelog: [Internal]
Flip text alignment in case layout direction is RTL.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D21130371
fbshipit-source-id: cf56ca052c17a48e321803b0f99f8a4baaa0e67b
Summary:
Easy diff to add a TODO to refactor `sendAccessibilityEvent` to use ViewCommands
This was orginally added D17142507
changelog: [Internal] Internal change
Reviewed By: JoshuaGross
Differential Revision: D21137348
fbshipit-source-id: aff38ccad8dfbb222f83161e2bd5da82f543e5db
Summary:
This PR fixes the drawing of the border rounded edges when the border-radius is small than the border-width. The current implementation capped the possible border-radius making it impossible to set smaller border-radii when using thicker borders. After inspection it was found that the rounded-rect calculation is incorrect.
## Changelog
`[Android] [Fixed] - Fix rounded border-drawing when border-radius is smaller than border-width`
Pull Request resolved: https://github.com/facebook/react-native/pull/28358
Test Plan:
**Faulty situation:**
As you can see, when the border-radius becomes very low, the border is stuck at a minimum value. Only after setting the border-radius fully to 0 is it again rendered correctly.
**After the fix:**
Differential Revision: D21124739
Pulled By: shergin
fbshipit-source-id: cefd1776b77b5b9fb335e95fd7fdd7f345579dc4
Summary:
This PR fixes incorrect drawing of the View borders on Android, after changing the border-radius back to 0 *(and when no background-color is defined)*.
This happens because the `drawRoundedBackgroundWithBorders` function in ReactViewBackgroundDrawable changes the style on the Paint object to `STROKE`. This style is however never reverted back to `FILL`. This change ensures that the Paint style is set to `FILL` for the full execution of the `drawRectangularBackgroundWithBorders` function.
## Changelog
`[Android] [Fixed] - Fix border-drawing when changing border-radius back to 0`
Pull Request resolved: https://github.com/facebook/react-native/pull/28356
Test Plan:
**Faulty situation:**
**After the fix:**
Differential Revision: D21124741
Pulled By: shergin
fbshipit-source-id: 2044f8e8ad59a58df42b64d7ee8c4ad1d3b562f1
Summary:
We need to break up the `uimanager` module in order to solve circular dependencies problem (which future diff would have otherwise).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D20885645
fbshipit-source-id: 8148bd934879802b076261ed86fa78acf0a07ed3
Summary:
We need to break up the `uimanager` module in order to solve circular dependencies problem (which future diff would have otherwise).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D20885163
fbshipit-source-id: 08eb1ba1d408fc0948e8d0da62380786a40973af
Summary:
fix typo in `YogaJNIBase.java` as there is no such file called `YGJNI.cpp`
Pull Request resolved: https://github.com/facebook/yoga/pull/990
Reviewed By: pasqualeanatriello
Differential Revision: D20735102
Pulled By: SidharthGuglani
fbshipit-source-id: 3f9f4d78ba390feae3451330f997a221ab4ec70e
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/28658
This moves the Java files to FB internal and updates all the buck files.
## Changelog:
[Android] [Removed] This diff removes the CheckBox export from React Native. Internally, we are requiring CheckBox directly now and externally people will have to use the community maintained module.
Reviewed By: cpojer
Differential Revision: D21066998
fbshipit-source-id: 76821fcae899ff7342697ea7dd4737ef3b008213
Summary:
This diff introduces a new "Open Debugger" menu item for VMs that support on device debugging and for opening the React DevTools in Flipper.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D20784279
fbshipit-source-id: caecdace00007224692d994a75c106842c8b2acb
Summary:
When text is in a constrained parent view using `maxWidth`, long text may wrap. When the text wraps, the final width is dependent on the word breaking strategy and text content. This means that the text width is not necessarily `maxWidth`.
However, the current way that we compute text layout does not shrinkwrap the text width as much as possible. This leads to visual gaps to the end-side of wrapped text.
This changes the text layout slightly so that we use the length of the longest line.
This bug only exists on Android. After this change, Android behaves like iOS.
Changelog:
[Android] [Fixed] - Fixed excessive space in Text view with word-wrapping
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D21056031
fbshipit-source-id: e9b7793f2632caafcce69bc15bac61330b0ed958
Summary:
## Problem
Every time we want to add, remove, or change the data passed to JavaTurboModule's constructor, we have to modify the C++ TurboModule codegen. (The same is true of `ObjCTurboModule`).
**Why was this necessary?**
- `JavaTurboModule` is effectively an abstract class whose constructor is always invoked by code-generated C++ classes. These C++ code-generated class constructors accept an argument list, and manually foward each and every item in that list to `JavaTurboModule::JavaTurboModule`.
## The fix
In this diff, I introduce a struct `JavaTurboModule::InitParams`, to represent a bag of arguments:
```
class JSI_EXPORT JavaTurboModule : public TurboModule {
public:
struct InitParams {
std::string moduleName;
jni::alias_ref<JTurboModule> instance;
std::shared_ptr<CallInvoker> jsInvoker;
std::shared_ptr<CallInvoker> nativeInvoker;
};
```
All `JavaTurboModules` will be created with an instance of this `InitParams` struct, instead of a list of arguments. Our code-generated C++ `jsi::HostObject` sublcasses will simply accept `InitParams` in their constructor, and forward it to `JavaTurboModule`'s constructor. This way, the codegen remains oblivious to what arguments JavaTurboModule requires.
## Okay, but why do we need this change now?
In the future, I plan to modify the constructor for `JavaTurboModule` to accept a performance logger, and a `RuntimeExecutor`. Similar modifications are planned for ObjC. For this reason, to avoid these four codemods, and any potential other codemods that occur because we're making modifications to `JavaTurboModule` or `ObjCTurboModule`, I'm launching this codemod, and the codemods in this stack.
## Misc Fix
- Previously, we were generating the TurboModule name from the Spec filename. This is incorrect because that name represents the spec name. Now, the name will be forwarded from TurboModuleManager in the `JavaTurboModule::InitParams` struct.
## Alternative implementations
I initially considered using `ContextContainer`, but decided against it because:
1. There are no type-safety guarantees.
2. I think it's a bit overkill for this scenario. We just need an opaque bag of data, and for our purposes a simple struct does the job fine.
## Commands run
Reviewed By: fkgozali
Differential Revision: D21035208
fbshipit-source-id: 9542cafea192081bc34d337ab3a7a783083eb06c
Summary:
In D20659799, I improved `TurboModuleManager.getModule(moduleName)` thread-safety by ensuring that if two threads race to require the same NativeModule, only one thread creates the NativeModule, while the other one waits until it's created.
## The problem:
What I failed to realize was that when two threads race to require two different NativeModules, we can get concurrent calls into `TurboModuleManagerDelegate.getModule(moduleName)`, and `TurboModuleManagerDelegate.getLegacyCxxModule(moduleName)`, which don't have any thread-safe guarantees.
## The fix
`TurboModuleManagerDelegate` is supposed to be an input to the TurboModule system. So, rather than expecting that all TurboModuleManagerDelegates are thread-safe, which might be a reasonable ask (see T65532092), this diff has `TurboModuleManager` acquire the delegate's lock before calling into it. This ensures that we don't get concurrent access into the delegate, which could be reading from, or writing to, some data structure in these method calls. (This was the case with `ReactPackageTurboModuleManagerDelegate`, which is what Fb4a and Workplace use under the hood).
Changelog:
[Android][Fixed] - Control concurrent calls into TMMDelegate from TurboModuleManager
Reviewed By: mdvacca
Differential Revision: D21025965
fbshipit-source-id: d22c4abfe87f9e534717a06f186dde87d3cd24df
Summary:
This cache is unnecessary, because:
1. TurboModuleManager caches all created TurboModules
2. TurboModuleManager calls into the TurboModuleManagerDelegate at most once per NativeModule `moduleName`.
This diff also makes ReactPackageTurboModuleManager thread-safe, which should help get rid of the crashes in T46487253.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21027998
fbshipit-source-id: c9b5ccc3da7b81787b749e70aa5e55883317eed7
Summary:
We're still seeing NativeModule eager-init crashes in T46487253. So, just to be extra careful, in case this diff doesn't fix the problem, I'm adding logging into `TurboModuleManager.getModule(moduleName)` to see why TurboModules are showing up as `null`.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21027984
fbshipit-source-id: 74ee62aeac09a4fdb29547e90ef4fa7c07de17a6
Summary:
Changelog: [Internal]
We don't use view command `setMostRecentEventCount`, let's get rid of it.
Reviewed By: JoshuaGross
Differential Revision: D21016600
fbshipit-source-id: 6491c063e9d6a89252300cb47c010b248e473f4b
Summary:
Early ViewCommand Dispatch will solve this category of crashes by going through an entirely different codepath. For users not in that experiment, it might be good to have a mitigation that prevents non-critical issues from crashing (like "blur" failing).
Currently, "blur" failures cause lots of screens to crash. There's no useful signal and those crashes aren't super actionable, so seems better to swallow.
If/when early viewcommand dispatch ships as the default/only mode, we can remove this try/catch entirely.
The only concern I have with landing this is the perf implications of putting a try/catch inside this loop.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21023213
fbshipit-source-id: 310fe2d55a44bc424692a2365ccd5882f35f9d82
Summary:
This diff renames the analyticsTag prop for the intenral_analyticsTag in ImageView component
changelog: [internal] Creation of internal_analyticTag prop in ImageView, for now this prop is meant to be used internally.
Reviewed By: TheSavior
Differential Revision: D20904497
fbshipit-source-id: 2a28f746772ee0f9d657ec71549020c1f3e9d674
Summary:
Changelog: [Internal]
The cause of crash was `NullPointerException`, which happened because of `mReactContextForRootTag.get(rootTag)` returning `null`. This is solved by checking whether it returns `null` before passing it to `I18nUtil`.
Reviewed By: mdvacca
Differential Revision: D20890623
fbshipit-source-id: c884c6838b83b944a5438375a4c060c1f5b1dc6e
Summary:
This diff extends the Android Image View manager to support the new analyticsTag prop. this prop is going to be used to track performance for images in android
changelog: [Android][Added] Add analyticsTag prop into ImageView component
Reviewed By: JoshuaGross
Differential Revision: D20880602
fbshipit-source-id: e302e8fa83706e6517b228d44a3094a1686830f7
Summary: According to our logs, 80% of these warnings are coming from AppStateModule. It's not particularly interesting or surprising that the CatalystInstance would be torn down when there's some app event, so let's stop taking up DB space with a useless message.
Reviewed By: ejanzer, mdvacca
Differential Revision: D20879426
fbshipit-source-id: b1182461aed4a66d82cb34bbd4b12782af6ed7b3
Summary:
In bridgeless mode, the CatalystInstance doesn't exist, but we still need to be able to access the sourceURL in SourceCodeModule (which is needed to render the images in LogBox warnings and errors). This diff adds a new API for getting the sourceURL directly from ReactContext, instead of having to call context.getCatalystInstance().getSourceURL(), and updates SourceCodeModule to use it.
Changelog: [Internal]
Reviewed By: RSNara
Differential Revision: D20848700
fbshipit-source-id: 3ecda81a17121178b76bbb3e9b0f27f103c1961a
Summary:
NativeModules can be created from any number of threads. In the legacy system, `ModuleHolder`, the class responsible for creating NativeModules, has built-in concurrency control to ensure that NativeModule creation is thread-safe. This diff introduces that thread-safety to the TurboModule infra. Basically, after this diff, if `n` threads race to create a TurboModule x, only the first thread will create x. All other threads will wait until x is created.
Changelog:
[Android][Fixed] - Make TurboModule creation thread-safe
Reviewed By: mdvacca
Differential Revision: D20659799
fbshipit-source-id: 2b720fe1ea49e40ae0d6dae50d422f23a6f45520
David Vacca
generatedunixname89002005287564
James Ide
Ramanpreet Nara
Emily Janzer
Lulu Wu
Jakub Kinst
Joshua Gross
Tim Yung
Will Holen
Samuel Susla
Hein Rutjes
Hein Rutjes
Valentin Shergin
acton393
Lauren Tan
Rick Hanlon
Bruno Barbieri
Vojtech Novak
