Summary:
The new UIManagerCommitHook can be used to implement commit-atering features without modifying the core.
E.g. State Reconciation seems can be implemented as a coomit hook but first we will use it for a new feature called Timeline (see the coming diffs).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D25221312
fbshipit-source-id: dbe41b475bc8b36e1780d81447ab43b32758bdff
Summary:
With the change, a new delegate method allows a receiver to alter a new (proposed) shadow tree with another tree by returning the altered tree.
We will it use in future diffs to implement Commit Hooks.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D25221313
fbshipit-source-id: 9f83577d862b713fff71fa365ce660cc1de87c84
Summary:
After fixing `calculateShadowViewMutationsForNewTree` I realized that it will be even better to test Stacking Context and mutation instructions infra using both functions: `calculateShadowViewMutationsForNewTree` (used for testing) and the Differentiator itself. This diff implements it.
Now we have two similarly working functions with different implementations that we can use for testing Differentiator and other parts of the infra.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D25576922
fbshipit-source-id: 7922e9ebfb9d6ef1792566554ba0c4a14f835ae2
Summary:
Working on zIndex tests I found that calculateShadowViewMutationsForNewTree (that we use for testing) does not take zIndex into account. This fixes it.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D25576923
fbshipit-source-id: a71b3a4630430488c783cd5010c0fbb7273bdaa5
Summary:
The test covers most props that must generate views.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D25576921
fbshipit-source-id: df5bedb8f6d409b5142e472ca2edcb1953bee4e1
Summary:
This should make `testID` prop work as it works in pre-Fabric renderer on iOS.
On Android it should already work fine.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D25524890
fbshipit-source-id: 3f25eb427d4449abaab790099546be18ae573f98
Summary:
Changelog: [internal]
Cloning of `LegacyViewManagerInteropViewProps` causes loss of `sourceProps.otherProps` if the cloning happens before shadow node is mounted. This was happening in WebView and callback `onLoadEnd` was dropped because of this.
Reviewed By: JoshuaGross
Differential Revision: D25474581
fbshipit-source-id: 74d7c5cd32b7318bb99306c82bc8b5e5eab63db2
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:
The `index` parameter for UpdateMutation is optional, and is normally just -1. It's not useful, so remove it. `parentShadowView` is also not relevant and is not used; in some existing use-cases the actual parent view of the updated view is available, and in some contexts the parent view is not set.
The function now will always set the index to -1 for UpdateMutations, and `{}` for ParentShadowView.
This should have no impact on iOS or Android, as this parameter is not used. It could theoretically have an impact on lifetimes of objects retained (now not retained) by not passing parentShadowView into the mutation. For example, any shared props or state associated with the parent will not be retained in the Update mutation now.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D25342943
fbshipit-source-id: 0ddbef76a6e2eefc2629c9729f721d8674d7737e
Summary:
If a ShadowNode is being animated from opacity 0 to 1, and that animation is interrupted by another update to the same ShadowNode, we stop the animation and send the original "final" ShadowView
to the mounting layer. On iOS, this is enough to make the Mounting layer consistent with the Shadow layer. However, on Android, since only prop deltas are passed to the mounting layer and not the
entire props bag, this will NOT be enough because the opacity will not be updated in that final step.
Therefore, in those cases where we've detected a conflict and we're cleaning up after an interrupted animation, we must send two updates to the mounting layer: one to force the opacity to 1,
and another to make the ShadowTree consistent with the Mounting layer.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25324942
fbshipit-source-id: 5d9666128feaae87d7530c394ef05db580aa5a75
Summary:
In very marginal cases, it was possible to set up an animation of the following diffing instructions:
```
REMOVE X from parent Y
DELETE X
```
If your LayoutAnimation configuration had no "delete" config, the DELETE would be executed immediately; the REMOVE was erroneously being categorized as an "update" (now fixed)
which caused the REMOVE to be delayed, but then executed very shortly thereafter. So the order of instructions would become:
```
DELETE X
REMOVE X from parent Y
```
which would crash (or at least fail an assertion) when the REMOVE instruction was processed.
This fixes the issue by ensuring that REMOVEs have a corresponding "delete" config, or they are also executed immediately; unless followed by an INSERT (and any combination of `REMOVE, DELETE, INSERT` in the same frame is not possible).
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25292560
fbshipit-source-id: 7ffdd6cbcb43126de07a70c197dfaf1ebff83555
Summary:
The "reparenting differ" has been the default differ for several months; ship it by removing config and the old differ.
Some functions can't be deleted yet because unit testing relies on it heavily; this can be refactored in the future if we care a lot.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25257205
fbshipit-source-id: 6f1dcc490bb1efe3d12506addf5f0843ca48c5c6
Summary:
Changelog: [internal]
Touch events should be of a type PressEvent. Fabric only provided touches, changedTouches and targetTouches and leaves out force, identifier, locationX, locationY, pageX, pageY and timestamp.
Reviewed By: shergin
Differential Revision: D25243347
fbshipit-source-id: e824558bd43f51c0c6dcca62bfc98318aa61678e
Summary:
Changelog: [internal]
This is already defined in Touch.h
Reviewed By: shergin
Differential Revision: D25242843
fbshipit-source-id: 23bac2a60f3d995e34d342c3a189760875f4bc77
Summary:
Changelog: [internal]
Return value from `LayoutAnimationKeyFrameManager::getAndEraseConflictingAnimations` was a tuple with 3 elements. Two of them are not being used so let's get rid of them.
Reviewed By: JoshuaGross
Differential Revision: D25220601
fbshipit-source-id: 35781e735b6a2e518337fdeaf956c18bb370993b
Summary:
Changelog: [internal]
The assert is still firing, let's disable it until we can investigate why layout animations creates two delete mount instructions.
Reviewed By: majak
Differential Revision: D25216794
fbshipit-source-id: 6328a2afb5eaf7fceebdc05bc75804f2eb44ddd2
Summary:
These configs are never actually empty, so they shouldn't be optionals.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25129254
fbshipit-source-id: 626119fefad0440732541c680286ebbbfab6aeba
Summary:
Changelog: [internal]
`currentAnimation_` is accessed on multiple threads and has dedicated mutex, but it was not acquiring the mutex in `LayoutAnimationKeyFrameManager::shouldAnimateFrame`
Reviewed By: JoshuaGross
Differential Revision: D25121654
fbshipit-source-id: 38b1c82eaabab283beab18dc210ea21379edbe93
Summary:
Changelog: [Internal]
Contents of `callComplete_` are accessed from JavaScript thread and main thread and there is possible race condition. This diff changes `bool` to `atomic_bool` to prevent the race.
Reviewed By: JoshuaGross
Differential Revision: D25094907
fbshipit-source-id: 6a2c6e33ab5ba0c6ab728e175f2e5c11fdd0a579
Summary:
Changelog: [Internal]
General improvements. Behaviour should be exactly the same.
Reviewed By: JoshuaGross
Differential Revision: D25092505
fbshipit-source-id: 584640ece3e02d468f6bcb84577d7a6c899cc253
Summary:
Imagine the scenario in which there's an ongoing UPDATE animation; a REMOVE+INSERT (move) is queued up for the same tag, but there's no
new corresponding UPDATE - so maybe the indices of the view have changed, but the layout stays the same. Under the old model, the previous animation would be canceled and the node would jump to the final position. In theory, if there's no new UPDATE, we should continue animating the node to its final position.
I'm much happier with this - conflicting animations transition into each other super seamlessly now, and I think the logic is more straightforward as well.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25071664
fbshipit-source-id: fcefc4619dc34cdafdc4d8e8e730b935e5528290
Summary:
This is noisy when enabled, and not very useful.
Changelog: [Internal]
Reviewed By: PeteTheHeat
Differential Revision: D25071584
fbshipit-source-id: 7205b5fa39622feccaf315ccebb181dbdac4281d
Summary:
See comments, hopefully they explain this situation. This fixes the last remaining case that I have repro'd where StubViewTree asserts fire.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25062135
fbshipit-source-id: a28afba21f4094200aa0502b1e085dcbc10f9835
Summary:
The current implementation of LayoutAnimations assumed that the "previous/old" ShadowView passed into the diff mutation didn't matter except for purposes of diffing.
As it turns out, iOS components could possibly use the "old" version of props, state, etc - so we should try to keep track of the current value in the tree as much as possible.
This diff accomplishes that by keeping track of the "previous" view, which the AnimationDriver will update over time. This also allows us to simplify logic around conflicting animations.
I'm also adding a few additional asserts to assist in debugging.
This doesn't totally eliminate all asserts hit on iOS, yet, but it does reduce the number of times the asserts are hit in StubViewTree.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25048644
fbshipit-source-id: d00aeece5af04624d8193063be453c7ce4a6e565
Summary:
Like the task mentions `strongSelf->_eventInterceptors` was crashing, probably because the coordinator was cleaned up before this block ran.
Check to make sure self is still valid before attempting to access any instance variables.
Changelog: [Internal]
Reviewed By: fkgozali
Differential Revision: D25073812
fbshipit-source-id: cdf666f2ac028b5523097f15ff51fbae9f9ffbd8
Summary:
Changelog: [internal]
Reference here is incorrect, we need a container for `ShadowViewMutation`.
Reviewed By: JoshuaGross
Differential Revision: D25024080
fbshipit-source-id: f59a18d859ad391bc168c8990d40b25d18003f74
Summary:
Changelog: [internal]
Previous implementation of coalescing would only look at the last element in `eventQueue_` and if it was the same type and target, it would coalesce the two together. This was problem when user would scroll in UIScrollView, this triggers onTouchMove and onScroll events at high rates and prevents coalescing of them.
This changes changes the behaviour to search the `eventQueue_` backwards for an event of the same type and target. If one if found, it is moved into its place. If even of another type is found before for the same target, the event is pushed back onto the queue.
Reviewed By: JoshuaGross
Differential Revision: D24992941
fbshipit-source-id: fc1eae4ecd100af6202346674778b0634ed7a15b
Summary:
This diff adds more enforcement for consistency of `ShadowNodeMutation`s, including:
* `Props` object for newly created or updated view must not be nullptr;
* `oldShadowView` must describe the previous state of the view for `Update` instruction;
* `ignoreDuplicateCreates` option was removed.
I suspect some of the crashes we see in Fabric are caused by a violation of one of these constraints. If one of these fails in debug builds, we will get an early signal.
Changelog: [Internal]
Reviewed By: JoshuaGross
Differential Revision: D24880821
fbshipit-source-id: 8c8a3d8e205ce34f6e0335e8a2b0cf676930c284
Summary:
This change introducing using `updateStateWithAutorepeat` for state updates in RCTSafeAreaViewComponentView. This way we can reduce the number of active commits and reduce jumps & relayout passes.
The approach with a callback is better than using `_lastPaddingStateWasUpdatedWith` because:
* When we compare the values, we can compare them with actual previous padding numbers stored in Shadow Tree.
* The value stored in a UIView instance can go away because of a view being remounted because of flattening.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D24719345
fbshipit-source-id: 9bf1ae284875b4c99cf23be2fcc9a829eb8a895e
Summary:
This implements a new ShadowNode trait that helps to propagate Yoga node `isDirty` flag down the root of the tree and clone siblings appropriately.
Several Fabric components mutate its Yoga styles after the node was cloned. In such cases, we need to mark the node as dirty after doing so. The problem with this is that the parent node and its siblings were already updated (cloned or not) based on the previous value of the `isDirty` flag. This happens because this logic is implemented in YogaLayoutableShadowNode which is a base constructor that must be called before any other logic from a subclass can run.
For now, this change enables that for SafeAreaView only (which seems to help with some junkiness issues), later we can extend the usage of this for other components if needed.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D24719347
fbshipit-source-id: b0d050afea5de9c470e05e1b4c9e7052e00ae949
Summary:
For some interrupted animations we will execute a "final" mutation associated with the animation, if it exists. For example, "UPDATE" animations always have a final Update animation associated with them.
Some, however, do not. For example: INSERT animations do not have a final mutation associated by default. In these cases (before this diff) if the animation from opacity 0 to 1 was interrupted, the View will
appear "stuck" at some intermediate opacity. To mitigate that, we generate a synthetic "final" mutation at 100% progress through the animation if it is interrupted.
Changelog: [Internal]
Reviewed By: fred2028
Differential Revision: D24691151
fbshipit-source-id: d9730b8a3493a5eeac4de325e7e0a7a64f73c8a0
Summary:
When shouldEnableLoggingForRequestUrl is false, ImageTelemetry is not initialized, and no logging is done.
* Replace `- (NSString *)loaderModuleNameForRequestUrl:(NSURL *)url` with `- (BOOL)shouldEnableLoggingForRequestUrl:(NSURL *)url`
* Rename RCTImageLoaderInstrumentableProtocol.h -> RCTImageLoaderLoggableProtocol.h
Reviewed By: fkgozali
Differential Revision: D24523984
fbshipit-source-id: a5463eceea1c40f9452b0ad2ee6bf047f71a02c1
Summary:
The callback `dispatchEvent` is called asynchronously on the JavaScript thread, so all data it uses must be copied to the lambda. To work around this constraint we use a shared pointer for the counter.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D24598840
fbshipit-source-id: fb5581858d54dc806863caf0c7c4f612ed6046e2
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
Summary:
Changelog: [internal]
There are two exceptions inside `TelemetryController::pullTransaction`:
- Empty Optional cannot be unwrapped
- mutex lock failed: Invalid argument
By marking this method `noexcept`, stack trace is lost and it makes it more difficult to track down the issue.
What does compiler do if a method is marked `noexcept`?
```
void f() noexcept {
try {
// do work
}
catch (...) {
std::terminate(); // This is the std::terminate() we are seeing in stack traces.
}
}
```
Removing noexcept specifier might give us more information about the exception.
Reviewed By: JoshuaGross
Differential Revision: D24477861
fbshipit-source-id: 80f26e9ab160a5330c2848b89a01d60bfc0a4611
Summary:
#changelog: [internal]
When I built ThreadStorage I didn't know about existence of `thread_local` keyword. Because it achieves the same goal, using built in c++ features is preferred over building our own.
Reviewed By: JoshuaGross, shergin
Differential Revision: D24380680
fbshipit-source-id: e961fc34c6d3f085fc9b918b20bb4827de0d5624
Summary:
changelog: [internal]
Prevents 2 type converions:
1. int <-> size_t
2. int <-> int32_t
# Why is using size_t better when working with indexes.
## 1. Type conversion isn't for free.
Take this example
```
size_t calculate(int number) {
return number + 1;
}
```
It generates following assembly (generated with armv8-a clang 10.0.0):
```
calculate(int): // calculate(int)
sub sp, sp, #16 // =16
str w0, [sp, #12]
ldr w8, [sp, #12]
add w9, w8, #1 // =1
mov w8, w9
sxtw x0, w8
add sp, sp, #16 // =16
ret
```
That's 9 instructions.
If we get rid of type conversion:
```
size_t calculate(size_t number) {
return number + 1;
}
```
Assembly (generated with armv8-a clang 10.0.0):
```
calculate(unsigned long): // calculate(unsigned long)
sub sp, sp, #16 // =16
str x0, [sp, #8]
ldr x8, [sp, #8]
add x0, x8, #1 // =1
add sp, sp, #16 // =16
ret
```
Compiler now produces only 7 instructions.
## Semantics
When using int for indexing, the type doesn't say much. By using `size_t`, just by looking at the type, it gives the reader more information about where it is coming from.
Reviewed By: JoshuaGross
Differential Revision: D24332248
fbshipit-source-id: 87ef982829ec14906ed9e002ea2e875fda4a0cd8
Valentin Shergin
Samuel Susla
Emily Janzer
Joshua Gross
Peter Argany
Paige Sun
Pieter De Baets