Summary:
Add unit tests for Layout Animations.
This first batch generates a random mutation, then animates it to completion.
I found one issue with UPDATE+REMOVE+INSERT animation consistency. That shouldn't cause any crashes in production, but is a chance to improve consistency of mutations overall - and could in theory point to memory corruption, though it's somewhat unlikely.
I ran with randomized seeds, found issues, fixed them, re-ran to ensure issues were fixed, rinsed and repeated. At the end I was able to run dozens of times (with random seeds) and found nothing.
The next step is to repeatedly generate mutations that conflict with ongoing animations.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D28343750
fbshipit-source-id: c1c60d89a31be3ac05d57482f0af3c482b866abe
Summary:
When REMOVE animations finish, they are hitting asserts in StubViewTree because of UPDATEs and REMOVEs being out-of-order. By simply not forcing REMOVEs to happen before UPDATEs, this problem seems to go away.
Long-term (in a couple weeks, likely) I want to add unit tests to catch all cases like this, but I'm comfortable with this change since it does fix a known assert failure.
Changelog: [Internal]
Differential Revision: D28086223
fbshipit-source-id: eb77ea94d76e996d7b2cb37038ce6f2a9799f8b4
Summary:
This is more insurance against future changes than fixing any existing bugs. In a very small number of cases it looks like this sorting isn't working, for reasons that are not easily reproducible (way less than 1 in 10000 times for me, if that) and not obvious, since we're sorting vectors in a canonical and straightforward way.
Again, this insures that logic won't change in the future.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27492936
fbshipit-source-id: 7f47e44ac1101915e1a0433c8f0a50a3c6a0c7b3
Summary:
This chunk of code is repeated 3x in the codebase with minor variations. Consolidate as `queueFinalMutationsForCompletedKeyFrame`.
Should be no changes in behavior.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407766
fbshipit-source-id: 196f0d4c7868007f0d103b024feb4450640a3f62
Summary:
Call getAndEraseConflictingAnimations recursively, to clean up any conflicting animations.
With a specific sequence of mutation instructions, it is possible that an animation on a tree is set up, and the entire tree needs to be cleaned up if a parent is deleted, say.
This can happen especially if rapidly mutating trees in such a way that invokes view flattening and unflattening (which is not recommended, but is certainly possible).
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407776
fbshipit-source-id: 5125250f051c58870692f8fdc43ed23da5058a7f
Summary:
In general, when an animation is interrupted or completed, we want to forcibly "flush" an update that will make the StubViewTree/mounting layer consistent with the ShadowTree.
However, in cases where a conflict creates a second animation and stops the first, we simply updated the "viewPrev" of the second animation and smoothly transition to it.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407774
fbshipit-source-id: 928363867d8994c9d69c53154a07bda94f954afa
Summary:
One of the struggles with the LA engine is to make sure that LA doesn't break the StubViewTree. In particular, we have strict requirements that the "oldShadowView" with every mutation matches exactly what is in the shadow tree, so it appears that there is a linear progression of ShadowNodes for every mutation instruction.
This is a struggle to do with the current setup, requires some wacky code, and doesn't work properly. Instead of spreading REMOVE/DELETE (especially) or INSERT/UPDATE instructions across multiple keyframes, we now allow keyframes to have multiple "final" instructions to execute,
which makes it much easier to keep state consistent.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407769
fbshipit-source-id: 3a709503dc30be69efc345690cb1920eb9591e9a
Summary:
Just makes it easier to log and inspect these values.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27407778
fbshipit-source-id: 68e93d100b56bc9065f0ff1370260412d729312d
Summary:
On Android we have the notion of "virtual views", which are defined consistently but the logic is scattered and duplicated throughout the codebase.
The logic exists to mark nodes that exist in the ShadowTree, but not the View tree. We want to CREATE, UPDATE, and DELETE them on the platform, but not INSERT or REMOVE
them. They basically exist as EventEmitter objects.
The only issue with this is (1) duplicated code, which opened the possibility for inconsistent definition (2) StubViewTree did not account for virtualized views, which caused
assert crashes in debug mode for certain LayoutAnimations on Android.
By moving the definition to ShadowViewMutation and accounting for it in StubViewTree, asserts are correct and consistent on all platforms.
This was not caught until recently, because, until recently, no asserts actually ran on Android.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27001199
fbshipit-source-id: eb29085317037ba8a286d7813bdd57095ad4746f
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/30988
We have a bunch of flags scattered throughout the codebase with poor hygiene and commenting. Consolidate.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26392518
fbshipit-source-id: 2823de123a5009d6b8c358e8a3f451b9fa0e05b7
Summary:
remove dead code; this flag is always defaulted to false, and not used anywhere
Changelog: [Internal]
Reviewed By: sammy-SC, mdvacca
Differential Revision: D26271507
fbshipit-source-id: e2277cc24f164c53f2e8a0aa72456ac400834d70
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:
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]
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:
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:
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:
Index adjustment is tricky. Seems more reliable to adjust each immediate mutation, and then immediately adjust delayed mutations based on it, rinse and repeat.
Previously it was possible to construct examples where the UI would get into a weird state because index adjustment caused items to be inserted in the wrong location.
Changelog: [Internal]
Reviewed By: kacieb
Differential Revision: D24232926
fbshipit-source-id: f8c445213528c2d2aedacf3e0c73c5bbeb62bc3d
Summary:
The sorting function currently forms a partial ordering, not a total ordering. This can cause problems with certain sequences of immediate or conflicting mutations, leading to UI corruption or crashes.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D24002668
fbshipit-source-id: edc9b4c1e3104897cb0c5fd6da563ec43d800494
Summary:
In this diff I simplify index adjustment and add comments to rigorously describe what we're doing at each step of index adjustment.
I've also made unflattening detection more correct, robust, and slightly more efficient.
Bugs that existed before:
1) The reparenting detection that existed in the animations layer had some subtle bugs. I don't have proof that it results in any correctness issues or crashes, but I suspect it.
2) Correctness of index adjustment: there were cases where the Android mounting layer would crash because LayoutAnimations would try to remove a View at an index, but the index was wrong. This is why I sat down and diagrammed the relationships between all the bits of data we have for index readjustment - I believe this to be correct now.
3) Correctness of INSERT index adjustment: I had the insight that when we have batches of INSERTs to consecutive indices, we essentially want them to be treated as a single INSERT for adjustment purposes, so that they're all placed consecutively in the view layer. I added `ConsecutiveAdjustmentMetadata` to deal with this, and there are more comments in the code.
Changelog: [Internal]
Reviewed By: yungsters
Differential Revision: D23806163
fbshipit-source-id: cd9e94945034db8b840f2a806c6377034a91af61
Summary:
Just renaming, nothing more.
The idea of MountingTelemetry already grown to something bigger than just mounting telemetry, so we are renaming it.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23374947
fbshipit-source-id: f60ce38b75d1ce77498b84688e59598314c69a78
Summary:
My goal in this diff is to make LayoutAnimations more stable, and more resilient to challenging situations. Namely, LayoutAnimations already works fine with:
1) Reorders
2) Flattening/unflattening
3) Deletion and recreation of the same hierarchy
4) Updates conflicting with an existing animation
However, what if /all/ of those things are combined? Handling update conflicts with multiple ongoing animations, repeatedly flattening/unflattening the same layer of hierarchy and reordering both parents and children, etc .
This diff does not make LayoutAnimations perfect, but it does make LayoutAnimations much more resilient to situations it was not able to handle before.
My primary method of testing was to use two Playground examples: one just repeatedly queues up mutations (some animated, some not) that create, update, and delete in the same hierarchy layer. The second, more complex one, mutates between random view hierarchies that involve a lot of flattening and unflattening as well as reordering, over a depth of 5. It also exercises animations over TextInlineViews, which is more challenging.
LayoutAnimations works best with the new "Flattening Differ" for now, because the Flattening Differ produces a much smaller, nearly minimal set of instructions in cases of flattening-unflattening. I would like that to not be a hard requirement for using LayoutAnimations, but it's a good starting-point for now.
As part of this work, I also developed a lot of debugging and logging mechanisms that are handy for detecting inconsistencies and debugging crashes. Some are included in this diff behind `#define` statements that are disabled by default, and the rest will be published separately and likely cannot be landed permanently, as they're more invasive changes that are only helpful in debugging.
# Followups:
- Automate testing: write a suite of C++ tests that mutates between random diffs and guarantees that all mutations in a StubViewTree are sensible
- Construct a set of minimal repros that catalogues all remaining crashes and inconsistency issues (these seem to be extremely marginal cases and are very hard to repro - so I think it's fine to run this in prod for now, but I will follow-up as soon as I'm back to catalogue and fix all remaining issues)
- This diff focuses on *not crashing*, but it is still possible to construct a sequence of complex mutations that results in (for example) views having some opacity between 0 and 1 if animations are interrupted repeatedly. Although this is easy enough to prevent in product code - the types of scenarios I'm running in tests are very unlikely to ever happen in production - it would be nice to be *sure* that LayoutAnimations will always converge to a sensible View hierarchy with up-to-date props.
- In general, the index adjustment logic is complicated. I don't know if there's a great way around it, so I need to at least catalogue and test all edge-cases as mentioned above.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D23382975
fbshipit-source-id: f379d9aa2a4b9c33fa2ba8fa07870c9e31fad5e7
Summary:
I've used this while debugging LayoutAnimations a few times. It compiles out to nothing when the define is not set.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D22962113
fbshipit-source-id: 88b7bb1c20a07a7d804e5c81d31cf871d58bee92
Summary:
Consolidated a few places where index adjustment was happening with nearly identical code. There was also one remaining case where index adjustment should be happening, but was not previously.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D22962115
fbshipit-source-id: 732c026b5a3c60bb0eadb8d49826ffd6367f7f62
Summary:
Hook up onSuccess and onFailure callbacks to LayoutAnimations.
Note that in non-Fabric RN, onSuccess is not known to work in Android. I could not find any uses of onFailure and it's not documented, so for now, it is only called if the setup of the animation fails.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D22889352
fbshipit-source-id: 4306debb350388dd2b7a2cbfe295eb99723988e2
Summary:
This diff moves fabric C++ code from ReactCommon/fabric to ReactCommon/react/renderer
As part of this diff I also refactored components, codegen and callsites on CatalystApp, FB4A and venice
Script: P137350694
changelog: [internal] internal refactor
Reviewed By: fkgozali
Differential Revision: D22852139
fbshipit-source-id: f85310ba858b6afd81abfd9cbe6d70b28eca7415
Joshua Gross
Samuel Susla
Valentin Shergin
David Vacca