Commit ce621bb
Fix Animated listeners on nodes derived from Animated.Value (#58037)
Summary:
Fixes #49719.
`addListener` stopped firing on any `AnimatedNode` *derived* from an `Animated.Value` — `Animated.add`/`subtract`/`multiply`/`divide`/`modulo`/`diffClamp` and `.interpolate()` — for natively driven animations. It worked in 0.77 and regressed in 0.78. The issue has since been reported for both `Animated.add` and `.interpolate()`, on both architectures, and the only workaround is to listen to the source value and recompute the derived value in JS — which observes intermediate values that never reach the screen.
There are two independent causes.
### 1. JS: derived nodes never subscribe to native updates
Before 38c46fe ("Animated: Lower `onAnimatedValueUpdate` to `AnimatedValue`", #48514), `AnimatedNode.addListener` called `startListeningToAnimatedNodeValue` for any node that had been made native. That commit moved the logic down into `AnimatedValue`, on the premise that:
> the `startListeningToAnimatedNodeValue` native module method only supports native tags for instances of `AnimatedValue` […] On Android, `startListeningToAnimatedNodeValue` throws if the node is not an instance of `ValueAnimatedNode`. On iOS, it does nothing if node is not an instance of `RCTValueAnimatedNode`.
That holds for props/style/transform/object/tracking/color nodes, but not for the operator and interpolation nodes, which *are* value nodes natively:
- **Android** — `AdditionAnimatedNode`, `SubtractionAnimatedNode`, `MultiplicationAnimatedNode`, `DivisionAnimatedNode`, `ModulusAnimatedNode`, `DiffClampAnimatedNode` and `InterpolationAnimatedNode` all extend `ValueAnimatedNode`, and `NativeAnimatedNodesManager` checks `node !is ValueAnimatedNode`.
- **iOS** — the corresponding `RCT*AnimatedNode` classes all inherit `RCTValueAnimatedNode`, and the manager checks `isKindOfClass:[RCTValueAnimatedNode class]`.
So the subscription was dropped for a set of nodes that natively support it. This restores it on `AnimatedNode`, gated on a new `__isNativeValueNode` flag that is `true` only for nodes backed by a native value node. That keeps the guarantee #48514 was after — never call `startListeningToAnimatedNodeValue` with a non-value tag — but states it explicitly instead of leaving it implicit in the class hierarchy. `AnimatedValue` now inherits that machinery rather than duplicating it.
`AnimatedNode.__detach` also drops the subscription before `dropAnimatedNode`, so it can never outlive the tag it observes.
### 2. C++: `startListeningToAnimatedNodeValue` rejects derived value nodes
Unlike Android and iOS, the C++ backend used by the New Architecture compares the node's *exact* type tag instead of testing for a `ValueAnimatedNode` subclass:
```c++
iter->second->type() == AnimatedNodeType::Value
```
so an addition or interpolation node was rejected with `"does not exist, or is not a 'value' node"`, even though `static_cast<ValueAnimatedNode*>` would have been valid. Replaced with an exhaustive `isValueNodeType` predicate, so every node type whose C++ class derives from `ValueAnimatedNode` (including `Round`) is accepted and the non-value ones are still rejected.
`AnimatedNode.__onAnimatedValueUpdateReceived` had to accept a missing `offset` as well: the C++ backend emits `onAnimatedValueUpdate` without one, which would otherwise produce `NaN` (`value + undefined`).
## Changelog:
[GENERAL] [FIXED] - `addListener` fires again for natively driven `Animated` values derived with `add`/`subtract`/`multiply`/`divide`/`modulo`/`diffClamp`/`interpolate`
Pull Request resolved: #58037
Test Plan:
### New tests
**`Libraries/Animated/__tests__/AnimatedComposition-itest.js`** — renders a derived node bound to `translateX`, attaches a listener to the *derived* node before it is made native, runs a `timing` animation on the source value, and asserts the listener observed the derived value; then asserts `removeListener` stops the updates. Parameterised over the five operators plus `interpolate`, and over both drivers.
**`ReactCommon/react/renderer/animated/tests/AnimatedNodeTests.cpp`** — `StartListeningToDerivedValueNode` asserts the C++ backend delivers updates for an `addition` node and stops on `stopListeningToAnimatedNodeValue`. `StartListeningToNonValueNodeIsIgnored` asserts registering on a `transform` node is still a no-op.
### Results
```
$ yarn fantom packages/react-native/Libraries/Animated/__tests__/AnimatedComposition-itest.js
addListener on derived value nodes
✓ Animated.add notifies its listeners on the JS driver
✓ Animated.subtract notifies its listeners on the JS driver
✓ Animated.multiply notifies its listeners on the JS driver
✓ Animated.divide notifies its listeners on the JS driver
✓ Animated.modulo notifies its listeners on the JS driver
✓ interpolate notifies its listeners on the JS driver
✓ Animated.add notifies its listeners on the native driver
✓ Animated.subtract notifies its listeners on the native driver
✓ Animated.multiply notifies its listeners on the native driver
✓ Animated.divide notifies its listeners on the native driver
✓ Animated.modulo notifies its listeners on the native driver
✓ interpolate notifies its listeners on the native driver
Tests: 26 passed, 26 total
```
Reverting only the fix (keeping the new test) fails exactly the six `native driver` cases, which is the reported bug; the `JS driver` cases were never broken:
```
✕ Animated.add notifies its listeners on the native driver
Expected 0 to be greater than or equal to 0
> 469 | expect(values.length).toBeGreaterThan(0);
…
Tests: 6 failed, 20 passed, 26 total
```
No regressions in the rest of the Animated suite:
```
$ yarn fantom packages/react-native/Libraries/Animated/__tests__/
Test Suites: 2 skipped, 16 passed, 16 of 18 total
Tests: 3 skipped, 297 passed, 300 total
$ yarn jest packages/react-native/Libraries/Animated
Test Suites: 3 passed, 3 total
Tests: 66 passed, 66 total
$ yarn flow-check
Found 0 errors
$ yarn lint # changed files
(no output)
```
`yarn build-types` regenerated `ReactNativeApi.d.ts`; the only substantive change is that `AnimatedValue`'s `removeListener`/`removeAllListeners` are now inherited from `AnimatedNode` rather than redeclared. `addListener` keeps its narrower `ValueListenerCallback` signature.
Correction to an earlier revision of this description: the C++ tests were not run locally, and they are not covered by the public CI either. `react/renderer/animated/tests` is excluded from the iOS build (`React-Fabric.podspec`: `ss.exclude_files = "react/renderer/animated/tests"`) and is not in the Android CMake glob (`react/renderer/animated/CMakeLists.txt` globs `*.cpp drivers/*.cpp event_drivers/*.cpp internal/*.cpp nodes/*.cpp`), so they build only in the internal build reached at import time. What is verified here: `NativeAnimatedNodesManager.cpp` compiles clean under `-Wall -Werror -Wpedantic` as part of the Fantom tester build, and the JS-side changes are covered by the Fantom and Jest runs above.
### Notes
For a colour `interpolate()` on the native driver the listener receives the interpolated colour as a number rather than a string, because the value is computed natively. That matches pre-0.78 behaviour and is out of scope here.
Reviewed By: christophpurrer, cipolleschi
Differential Revision: D117209945
Pulled By: zeyap
fbshipit-source-id: b50657a7b2d25e10e980b43cb1e9e1f1db5f39061 parent 8e1559a commit ce621bb
14 files changed
Lines changed: 392 additions & 98 deletions
File tree
- packages/react-native
- Libraries/Animated
- __tests__
- nodes
- ReactCommon/react/renderer/animated
- tests
- src/private/animated/__tests__
Lines changed: 121 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
379 | 379 | | |
380 | 380 | | |
381 | 381 | | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
| 23 | + | |
| 24 | + | |
23 | 25 | | |
24 | 26 | | |
25 | 27 | | |
| |||
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| 22 | + | |
| 23 | + | |
22 | 24 | | |
23 | 25 | | |
24 | 26 | | |
| |||
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
| 23 | + | |
| 24 | + | |
23 | 25 | | |
24 | 26 | | |
25 | 27 | | |
| |||
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
444 | 444 | | |
445 | 445 | | |
446 | 446 | | |
| 447 | + | |
| 448 | + | |
447 | 449 | | |
448 | 450 | | |
449 | 451 | | |
| |||
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| 22 | + | |
| 23 | + | |
22 | 24 | | |
23 | 25 | | |
24 | 26 | | |
| |||
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
| 23 | + | |
| 24 | + | |
23 | 25 | | |
24 | 26 | | |
25 | 27 | | |
| |||
Lines changed: 70 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| 11 | + | |
11 | 12 | | |
12 | 13 | | |
13 | 14 | | |
| |||
54 | 55 | | |
55 | 56 | | |
56 | 57 | | |
57 | | - | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
58 | 64 | | |
59 | 65 | | |
60 | 66 | | |
| |||
73 | 79 | | |
74 | 80 | | |
75 | 81 | | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
76 | 95 | | |
77 | 96 | | |
78 | 97 | | |
| |||
81 | 100 | | |
82 | 101 | | |
83 | 102 | | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
84 | 106 | | |
85 | 107 | | |
86 | 108 | | |
| |||
93 | 115 | | |
94 | 116 | | |
95 | 117 | | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
96 | 121 | | |
97 | 122 | | |
98 | 123 | | |
| |||
104 | 129 | | |
105 | 130 | | |
106 | 131 | | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
107 | 135 | | |
108 | 136 | | |
109 | 137 | | |
| |||
113 | 141 | | |
114 | 142 | | |
115 | 143 | | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
116 | 147 | | |
117 | 148 | | |
118 | 149 | | |
119 | 150 | | |
120 | 151 | | |
121 | 152 | | |
122 | | - | |
123 | | - | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
124 | 191 | | |
125 | 192 | | |
126 | 193 | | |
| |||
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
| 23 | + | |
| 24 | + | |
23 | 25 | | |
24 | 26 | | |
25 | 27 | | |
| |||
0 commit comments