Summary:
I've noticed that `UIManagerCommitHooks` are applied after calling `layoutIfNeeded`, meaning that if a commit hook makes some changes to the tree, it needs to calculate the layout again, effectively making the first calculation redundant.
This PR swaps the order of these two operations and moves `shadowTreeWillCommit` call before `layoutIfNeeded`. Thanks to this change, commit hooks don't need to manually trigger layout calculations as well as can potentially operate on unsealed nodes which can make it more efficient in some cases.
Finally, this PR eliminates a crash on `emitLayoutEvents(affectedLayoutableNodes);` when commit hook actually modifies the tree and thus de-allocates old shadow nodes.
cc sammy-SC
## Changelog
[GENERAL] [CHANGED] - Run commit hooks before layout calculation
Pull Request resolved: https://github.com/facebook/react-native/pull/36216
Test Plan: The only `UIManagerCommitHook` I could find in the OSS repo is [`TimelineController`](https://github.com/facebook/react-native/blob/8bd3edec88148d0ab1f225d2119435681fbbba33/ReactCommon/react/renderer/timeline/TimelineController.h#L26).
Reviewed By: cipolleschi
Differential Revision: D43569407
Pulled By: sammy-SC
fbshipit-source-id: 9ab1de0954cac2eb00346be7af8c9b3572bd2c88
Summary:
In [this commit](https://github.com/facebook/react-native/commit/e6cba997443ad72961cee1eee39346f80b48184e), we renamed `Geometry.h` to `Vector.h`.
This is the last change of a series of updates that progressively altered the shape of `Geometry.h` from its [original version](https://github.com/facebook/react-native/blob/0.71-stable/ReactCommon/react/renderer/graphics/Geometry.h).
This is a breaking change as several libraries in the OSS are depending on Geometry.h and we removed it without deprecating it.
This change aims to put the file back, with a deprecation message, so that we can safely remove it in the next version.
## Changelog
[General][Fixed] - Restore Geometry.h to avoid breaking change
Reviewed By: cortinico
Differential Revision: D43621983
fbshipit-source-id: 96b976901f7ec337f2ec64c592e4b4d19d89ab11
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36258
This fixes a few instances where YogaLayoutableShadowNode (or general shadownode casting) could offer better memory safety.
1. The reference form of traitCast() now terminates on invalid cast, instead of debug assert, since it is better to crash in production than to corrupt memory (which will crash somewhere later, in a much more confusing way).
2. We use traitCast() in more places where we previously would static_cast. This means needing to formally add a mutable version.
3. We bounds-check yoga children access in a single place by using `std::vector` `at()` instead of `[]`.
4. Removed `Trait::UnreservedTrait1` API, since multiple libraries using it can collide and we lose the memory safety benefits of `traitCast`.
This change is in response to a bug where `YogaLayoutableShadowNode` may perform an invalid `static_cast` of `RawTextShadowNode` if a text or number is rendered directly inside of a `<View>` (instead of a `<Text>` element).
This does not yet fix the underlying logic of YogaLayoutableShadowNode to act gracefully when a RawTextShadowNode makes its way into children. We just terminate, instead of corrupting memory.
Changelog:
[General][Breaking] - Better Fabric ShadowNode Memory Safety (Removes `Trait::UnreservedTrait` API)
Reviewed By: javache
Differential Revision: D43271779
fbshipit-source-id: 727c1230f72664bf4d261871c66ca61ddf0d5ffa
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36210
One of the circular dependencies we have in OSS was between React-Codegen and React-Fabric.
React-Codegen generates component which has to depends on React-Fabric because they need to use the files contained in the `react/renderer/view` folder.
React-Fabric contains some components that depends on RNCore, which was generated inside the React-Codegen folder.
This change generates the RNCore components inside the `ReactCommon/react/renderer/components/rncore` folder, breaking the dependency as `rncore` folder is now contained by React-Fabric itself.
**Fun Fact:** That's how it always should have been. There was already a line in the `.gitignore` to exclude the content of `ReactCommon/react/renderer/components/rncore` folder. I guess that with some of the refactoring/previous projects on Codegen, this requirements has slipped.
## Changelog:
[iOS][Breaking] - generates RNCore components inside the ReactCommon folder and create a new pod for platform-specific ImageManager classes
Reviewed By: sammy-SC, dmytrorykun
Differential Revision: D43304641
fbshipit-source-id: ebb5033ce73dbcd03f880c3e204511fdce04b816
Summary:
Update podspecs with the right search paths to include the required framework by every module.
## Changelog:
[iOS][Changed] - Update search paths to support `use_frameworks!` with Fabric
Reviewed By: sammy-SC, dmytrorykun
Differential Revision: D43089372
fbshipit-source-id: 4bbfc4b98bd289d66ce4015429d581856d9c05b3
Summary:
changelog: [internal]
Making a copy of shared_ptr is order of magnitude more expensive than moving it. This diff avoids two redundant copies in commit phase.
Reviewed By: rubennorte, cipolleschi
Differential Revision: D43306245
fbshipit-source-id: cfa942cc67b1e5c91be47803b80f7c8cda2e32d8
Summary:
changelog: [internal]
C++17 has implementation of shared_mutex in standard library. Let's use it instead of folly.
Reviewed By: cipolleschi
Differential Revision: D43275493
fbshipit-source-id: d766251226aa230110011aca94b4e697fe0d31a1
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36164
Changelog:
[General][Fixed] - Invalid prop values no longer trigger assertion failures in Fabric
## Context
Fabric has historically been very strict about prop parsing. It originally `abort()`ed on any prop value that failed to parse according to the expected type; this was replaced with dev-only assertions in D27540903 (https://github.com/facebook/react-native/commit/cb37562f8355b624e271c5b74416c257b0e62a00). We've received feedback that C++ assertions (and other similar mechanisms in the legacy renderer) are still too aggressive and disruptive as a diagnostic for developers working on JS code.
We are changing React Native to behave more like a browser in this regard, reflecting a new principle that **bad style values are not runtime errors**. (See e.g. D43159284 (https://github.com/facebook/react-native/commit/d6e9891577c81503407adaa85db8f5bf97557db0).) The recommended way for developers to ensure they are passing correct style values is to use a typechecker (TypeScript or Flow) in conjunction with E2E tests and manual spot checks.
More broadly, values passed from JS product code should not be able to crash the app, which is why we're not strictly limiting this change to style props. From now on, if a JS developer can trigger an internal assertion in React Native simply by writing normal application code, that is a bug.
## This diff
This diff introduces a new macro called `react_native_expect` which serves as a drop-in replacement for `react_native_assert`, but logs (to glog / logcat / stdout) instead of asserting. This way we don't need to fully delete the existing call sites. This will be helpful if we decide that we want to repurpose these checks for a new, more visible diagnostic.
I'm *intentionally* opting for the simplest possible improvement here, which is to silence the assertions - not to print them to the JS console, not to convert them to LogBox warnings, etc. The hypothesis is that this is already strictly an improvement over the previous behaviour, will help us get to feature parity between renderers faster, and allow us to design improved diagnostics that are consistent and helpful.
## Next steps
1. There are still places where Fabric can hit an unguarded assertion in prop conversion code (e.g. unchecked casts from `RawValue` with no fallback code path). I will fix those in a separate diff.
2. Paper on iOS needs a similar treatment as it calls `RCTLogError` liberally during prop parsing (resulting in a native redbox experience that is nearly as bad as an outright crash). I will fix that in a separate diff.
3. I'll add some manual test cases to RNTester to cover these scenarios.
4. We will eventually need to take a clear stance on PropTypes, but since they provide reasonable, non-breaking diagnostics (recoverable JS LogBox + component stack) it is less urgent to do so.
Reviewed By: sammy-SC
Differential Revision: D43184380
fbshipit-source-id: 0c921efef297d935a2ae5acc57ff23171356014b
Summary:
`setComponentDescriptorProviderRequest` is only used on iOS for dynamic registration of additional components. This variable is not default initialized, so will point to invalid memory when a component is missing and thus crash.
Changelog: [Internal] Fix Android crash when component is missing
Reviewed By: rubennorte
Differential Revision: D43305656
fbshipit-source-id: 70dd2973d50abbfcc19e051bd24c1deb90883941
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36158
To properly setup the Header Search Paths for `use_frameworks!`, we need to move the files that are in the `.../textlayoutmanager/platform/ios` folder into the `.../textlayoutmanager/platform/ios/react/renderer/textlayoutmanager` folder.
This mimic the same folder structure we have also `android`
## Changelog
[iOS][Changed] Moved the files from `.../textlayoutmanager/platform/ios` to `.../textlayoutmanager/platform/ios/react/renderer/textlayoutmanager`
Reviewed By: cortinico
Differential Revision: D43088586
fbshipit-source-id: 9589fe62f36fbff2744fdfbf3475e95954424232
Summary:
To properly setup the Header Search Paths for `use_frameworks!`, we need to move the files that are in the `.../imagemanager/platform/ios` folder into the `.../imagemanager/platform/ios/react/renderer/imagemanager` folder.
This mimic the same folder structure we have also `android`
## Changelog
[iOS][Changed] Moved the files from `.../imagemanager/platform/ios` to `.../imagemanager/platform/ios/react/renderer/imagemanager`
Reviewed By: cortinico
Differential Revision: D43088421
fbshipit-source-id: c3b86a95e67cc1ab9531997bb1bfbf011b7f730f
Summary:
To properly setup the Header Search Paths for `use_frameworks!`, we need to move the files that are in the `.../textinput/iostextinput` folder into the `.../textinput/iostextinput/react/renderer/components/iostextinput` folder.
This mimic the same folder structure we have also `android`
## Changelog
[iOS][Changed] Moved the files from `.../textinput/iostextinput` to `.../textinput/iostextinput/react/renderer/components/iostextinput`
Reviewed By: cortinico
Differential Revision: D43084639
fbshipit-source-id: fe0f55d9f372bda0fbd59c6c567ac747ca308a69
Summary:
To Properly setup the Header Search Paths for `use_frameworks!`, we need to move the files that are in the `react/renderer/graphics/platform/ios` folder into the `react/renderer/graphics/platform/ios/react/renderer/graphics` folder.
This mimic the same folder structure we have also in `cxx` and `android`
## Changelog:
[iOS][Changed] - Moved the files from `.../platform/ios` to `.../platform/ios/react/renderer/graphics`
Reviewed By: sshic
Differential Revision: D43082032
fbshipit-source-id: 519d2e12eb7edd50b5f71bac5b1c618c6bf89919
Summary:
changelog: [internal]
There was a bug in prop parsing where `accessibilityRole` and `accessibilityTraits` had wrong default value. Instead of falling back to sourceProps, they would set role to empty and trait to none. The fix is to fall back to sourceProps.
The issue surfaces with Animated, which clones props.
Reviewed By: cipolleschi
Differential Revision: D43190775
fbshipit-source-id: ec6b48c082c1dcc5b6e81dbff4f92b3a58a41da2
Summary:
changelog: [internal]
`useLayoutEffect` has two guarantees which React Native breaks:
- Layout metrics are ready.
- Updates triggered inside `useLayoutEffect` are applied before paint. State between first commit and update is not shown on the screen.
React Native breaks the first guarantee because it uses Background Executor. Background executor moves Yoga layout to another thread. If user core reads layout metrics in `useLayoutEffect` hook, it is a race. The information might be there, or it might not. They can even read partially update information. This diff does not affect this. We already have a way to turn off Background Executor. We haven't done this because it introduces regressions on one screen but we have a solution for that.
React Native breaks the second guarantee. After Fabric's commit phase, Fabric moves to mounting the changes right away. In this diff, we queue the mounting phase and give React a chance to change what is committed to the screen. To do that, we schedule a task with user blocking priority in `RuntimeScheduler`. React, if there is an update in `useLayoutEffect`, schedules a task in the scheduler with higher priority and stops the mounting phase.
We are not delaying mounting, this just gives React a chance to interrupt it.
Fabric commit phase may be triggered by different mechanisms. C++ state update, surface tear down, template update (not used atm), setNativeProps, to name a few. Fabric only needs to block paint if commit originates from React. Otherwise the scheduling is wrong and we will get into undefined behaviour land.
Rollout:
This change is gated behind `react_fabric:block_paint_for_use_layout_effect` and will be rolled out incrementally.
Reviewed By: javache
Differential Revision: D43083051
fbshipit-source-id: bb494cf56a11763e38dce7ba0093c4dafdd8bf43
Summary:
changelog: [internal]
Passing MountingCoordinator argument by value instead of reference. Using reference does not make sense since we eventually take ownership of shared_ptr anyway. This better communicates the intent.
Reviewed By: christophpurrer
Differential Revision: D43082955
fbshipit-source-id: 29e20abb9824c10a5f0d5e0ba1049ff6d67cee98
Summary:
Changelog: [Internal]
Caller needs to explicitly set commit options. This is for readability and making sure caller is aware of what are the options of the commit. This will be important in subsequent diff where we will add another commit option.
Reviewed By: christophpurrer
Differential Revision: D43082837
fbshipit-source-id: 1417205299c19430f902453c2b6d9bb9ca31707d
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36051
[Changelog][Internal]
This has been on my backlog for some time, submitting the diff to get it out of the way.
It makes the macro-based code in the "iterator-based property parsing" branch somewhat less horrible and less error prone (by removing duplication vs the default values in the class declaration).
Reviewed By: sammy-SC
Differential Revision: D42990595
fbshipit-source-id: e4b91160c6e09d3d1eab2ba70a58d390243bb335
Summary:
This is a prototype to add circular dependencies detection on CMake for ReactCommon and ReactAndroid.
It can be enabled per module and works as follows:
```
set(ALLOWED_HEADER_IMPORT_PATHS
react/renderer/graphics
react/debug)
check_for_circular_dependencies("${ALLOWED_HEADER_IMPORT_PATHS}")
```
The allowed header import path must be manually specified as libraries are exposing wrong header search paths (so can't be reused).
The CI will be red till the circular dependency on `graphics` is resolved.
Changelog:
[Internal] [Changed] - Add macro to detect circular dependencies on Cmake
Reviewed By: cipolleschi
Differential Revision: D42927036
fbshipit-source-id: b1393dfd43fd042e2ebf1d5b46b24bd9f5e20d58
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/35953
DimensionValue is a reserved prop type that can be a number or string (such as '50%'). On Java, it will get converted to a YogaValue (converter added to this diff); on C++ it will get converted to a YGValue (converter already exists as it's used in Fabric).
Changelog:
[Internal][Added] - Add codegen support for DimensionValue for components
Reviewed By: cipolleschi
Differential Revision: D42650799
fbshipit-source-id: 1d2bc30bbd93837dedbbb4c74f814963c8140957
Summary:
On Fabric, `onContentSizeChange` of `TextInput` component was never fired on `iOS`, since the logic dispatching it was implemented in `RCTBaseTextInputShadowView` on Paper: https://github.com/facebook/react-native/blob/0f8dc067ac079f7b14696cfcafa37e3ec19a0409/Libraries/Text/TextInput/RCTBaseTextInputShadowView.m#L105. This class is not used on Fabric, therefore the event was never dispatched. On Paper, it was dispatched in `dirtyLayout` method, so I added dispatching of this event based on the change of content size in `layoutSubviews` method, since this method seems the closest one on Fabric. I am not sure if it is the best place for it though.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry.
Pick one each for the category and type tags:
For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[IOS] [ADDED] - dispatch `onContentSizeChange` event on Fabric.
Pull Request resolved: https://github.com/facebook/react-native/pull/35816
Test Plan:
Try to use `onContentSizeChange` callback in `TextInput` component:
```tsx
import React from 'react';
import {TextInput, SafeAreaView} from 'react-native';
const App = () => {
return (
<SafeAreaView style={{flex: 1, backgroundColor: 'red'}}>
<TextInput
multiline={true}
placeholder="type here"
onContentSizeChange={e => console.log(e)}
/>
</SafeAreaView>
);
};
export default App;
```
Reviewed By: christophpurrer
Differential Revision: D42499974
Pulled By: sammy-SC
fbshipit-source-id: 3e010ff096cf91cb3e7b87ed2753e9d0e7be9650
Summary:
This PR implements logical border-radius as requested on https://github.com/facebook/react-native/issues/34425. This implementation includes the addition of the following style properties
- `borderEndEndRadius`, equivalent to `borderBottomEndRadius`.
- `borderEndStartRadius`, equivalent to `borderBottomStartRadius`.
- `borderStartEndRadius`, equivalent to `borderTopEndRadius`.
- `borderStartStartRadius`, equivalent to `borderTopStartRadius`.
## Changelog
[GENERAL] [ADDED] - Add logical border-radius implementation
Pull Request resolved: https://github.com/facebook/react-native/pull/35572
Test Plan:
1. Open the RNTester app and navigate to the `RTLExample` page
2. Test the new style properties through the `Logical Border Radii Start/End` section
https://user-images.githubusercontent.com/11707729/206623732-6d542347-93f9-40da-be97-f7dcd5f66ca9.mov
Reviewed By: necolas
Differential Revision: D42002043
Pulled By: NickGerleman
fbshipit-source-id: a0aa9783c280398b437aeb7a00c6eb3f767657a5
Summary:
[Changelog][Internal]
The diff changes underlying storage types used by different enums that are used in ViewProps data structure, together with some eventual field rearranging to reduce padding overhead.
This **shaves off 128 bytes** from each `ViewProps` instance, which is **a ~10% improvement**.
Given that an average RN app may have thousands of shadow tree nodes, and correspondingly `ViewProps` instances in flight (e.g. Oculus Store has 2-3K of them nominally), the overall memory win is about **300K+** for this change only. Plus slightly better cache locality, which never a bad thing either.
Reviewed By: mdvacca
Differential Revision: D42372127
fbshipit-source-id: d3a832af2b2e89f50a5b8e04d24d0df92869ea4d
Summary:
Changelog: [Internal]
`MapBuffer` already has a move constructor but did not have a move assignment operator prior to this diff. Changing this requires removing the `const` qualifier from `bytes_`, but this seems OK in practice as it will still be treated as `const` by all the `const` accessors on `MapBuffer`.
Reviewed By: RSNara
Differential Revision: D42369493
fbshipit-source-id: f0fcce533d8e0883dcf85d7262620ea77f377644
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/35768
Changelog: [Internal]
This implements native side mechanics for reporting user events timing to JS (PerformanceObserver API).
See the standard for more details: https://www.w3.org/TR/event-timing/
The events are only logged when there are any active subscriptions (via `PerformanceObserver.observe`), also we only log "discrete events" (i.e. no likes of mouse move), so the overhead is non-existing.
There are two main metrics of interest for an event lifecycle:
* Time the event is spent in the queue, i.e. the time between it's created and dispatched
* Time that is spend in the event handler on the JS side (event dispatch), or processing time
Both of these are measured, and the corresponding fields are populated.
Reviewed By: sammy-SC
Differential Revision: D42294947
fbshipit-source-id: 4fd7938c04b942400befa4057d4929fb2763cee1
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: [internal]
This is experimental implementation of `setNativeProps` in Fabric.
This diff brings `setNativeProps` to Fabric to make migration easier. I tried to stay as close as possible to Paper's behaviour, with all of its flaws (ready CAUTION section on https://reactnative.dev/docs/direct-manipulation)
State can't be stored in views because on iOS, eventually on Android, views are not the source of truth, shadow tree is. Fabric's implementation keeps state from setNativeProps in shadow node family in very inefficient data structure folly::dynamic. It is always reconciled with new prop updates. The performance cost is only paid in case node has used `setNativeProps` before.
Reviewed By: mdvacca
Differential Revision: D41875413
fbshipit-source-id: 453a5f7612a6f86a4cece269b13bd2ffd0c0e2d1
Summary:
This PR implements `inset` logical properties as requested on https://github.com/facebook/react-native/issues/34425. This implementation includes the addition of the following style properties
- `inset`, equivalent to `top`, `bottom`, `right` and `left`.
- `insetBlock`, equivalent to `top` and `bottom`.
- `insetBlockEnd`, equivalent to `bottom`.
- `insetBlockStart`, equivalent to `top`.
- `insetInline`, equivalent to `right` and `left`.
- `insetInlineEnd`, equivalent to `right` or `left`.
- `insetInlineStart`, equivalent to `right` or `left`.
## Changelog
[GENERAL] [ADDED] - Add Fabric implementation of inset logical properties
Pull Request resolved: https://github.com/facebook/react-native/pull/35692
Test Plan:
1. Open the RNTester app and navigate to the `View` page
2. Test the new style properties through the `Insets` section
<table>
<tr>
<td>Android</td>
<td>iOS</td>
</tr>
<tr>
<td><img src="https://user-images.githubusercontent.com/11707729/208821212-fbbac6ed-09a4-43f4-ba98-dfd2cbabf044.png" alt="1" width="360px" />
</td>
<td>
<img src="https://user-images.githubusercontent.com/11707729/208816997-ef044140-8824-4b1b-a77b-085f18ea9e0e.png" alt="2" width="360px" />
</td>
</tr>
</table>
Reviewed By: NickGerleman
Differential Revision: D42193661
Pulled By: ryancat
fbshipit-source-id: 3db8bcd2c4db0ef4886b9ec49a46424d57362620
Summary:
This is a two step (1/2) fix to a race that could caused a `DELETE`...`CREATE` mutations being sent over to the fabric mounting layer. Such combination was assumed not possible from the differ, yet it happened at least in the existence of layout animation and when certain commits happen while animation is active.
This race condition could cause a view to get deleted at the end of one UI frame, yet the mount instructions generated from animation in the next frame still need to access the deleted view and caused crashes like T112157805. Note that even though such crash is recorded as `RetryableMountingLayerException` and is a soft crash (which only gets logged but not crash in production), the out-of-order mount instructions could lead to illegal view state and make the surface unusable, like what's shown here:
{F820669000}
The diff fixes this issue by removing the `DELETE` [conflict animation](https://fburl.com/code/5ctckvz3) keyframe, as well as the `CREATE` [immediate mutations](https://fburl.com/code/txyomytd) from the layout animation. The Fabric mounting layer assumes no combination of `DELETE...CREATE` in the same frame from differ + [layout animation overrides](https://fburl.com/code/zn17uqch).
Reviewed By: sammy-SC
Differential Revision: D41895427
fbshipit-source-id: d6df02663ba707af6db4a63a325ac776ca54d18e
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