Skip to content

Commit 5a29c68

Browse files
dennytospmeta-codesync[bot]
authored andcommitted
Animate filter without forcing a Fabric commit every frame (#58071)
Summary: `NativeAnimatedAllowlist.h` carries this instruction: ```c++ /** * Direct manipulation eligible styles allowed by the NativeAnimated JS * implementation. Keep in sync with * packages/react-native/Libraries/Animated/NativeAnimatedAllowlist.js */ ``` It has drifted by one entry. `SUPPORTED_STYLES` in the JS file lists `filter` between `opacity` and `transform`; the C++ set goes straight from `"opacity"` to `"transform"`. Every other member of the JS list's non-layout half is present, so this is a missed sync rather than a deliberate exclusion. The set is not advisory — it is what separates layout props from paint props: ```c++ // StyleAnimatedNode.cpp bool isLayoutPropsUpdated(const folly::dynamic& props) { for (const auto& styleNodeProp : props.items()) { if (getDirectManipulationAllowlist().count(styleNodeProp.first.asString()) == 0u) { return true; // absent => treated as a layout prop } } return false; } ``` and that verdict picks the transport: ```c++ // NativeAnimatedNodesManager.cpp auto& current = layoutStyleUpdated ? updateViewPropsForBackend_[viewTag] // Fabric commit : updateViewPropsDirectForBackend_[viewTag]; // direct manipulation ``` So animating `filter` runs a shadow-tree commit on every frame instead of taking the direct-manipulation path, for a property that cannot affect layout — `filter` lives in `BaseViewProps` (`std::vector<FilterFunction> filter{}`), not in `YogaStylableProps`, and nothing in `react/renderer/animated/` treats it specially. Note the C++ set is deliberately *not* a mirror of the whole JS `SUPPORTED_STYLES`: the entries the JS file adds under `useSharedAnimatedBackend()` (`width`, `height`, `margin`, `padding`, `flex`, `gap`, …) are genuine layout props and must stay out so they keep going through Fabric. Only the non-layout half has to match, and `filter` belongs to it. ## Changelog: [GENERAL] [FIXED] - Animating `filter` no longer forces a Fabric commit on every frame Pull Request resolved: #58071 Test Plan: Added `directManipulationAllowlistCoversNonLayoutStyles` to `AnimatedNodeTests`. It asserts the allowlist contains every non-layout style the JS file supports, and — so the test cannot be satisfied by simply widening the set — that the layout styles are still absent. The allowlist header is dependency-free, so the invariant can also be checked directly: ``` $ c++ -std=c++20 -Wall -Wextra -I packages/react-native/ReactCommon allowcheck.cpp -o allowcheck # before MISSING from C++ allowlist: filter checked 34 JS non-layout styles, missing=1 (exit 1) # after checked 34 JS non-layout styles, missing=0 (exit 0) ``` ``` $ yarn jest packages/react-native/Libraries/Animated Test Suites: 3 passed, 3 total Tests: 66 passed, 66 total $ node ./scripts/clang-format.js <both changed files> (no changes) ``` `getDirectManipulationAllowlist` appears in the C++ API snapshots, but only by signature — this changes the contents of the static set, not the declaration, so `scripts/cxx-api` is unaffected. The gtest itself was neither executed locally nor built by the public CI. `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 it builds only in the internal build reached at import time. What is verified here: the test body compiles clean against the real header under `-std=c++20 -Wall -Wextra`, and the standalone parity check above exercises the same assertions and fails without the one-line change. Reviewed By: zeyap Differential Revision: D117174629 Pulled By: javache fbshipit-source-id: 5cfab73b61773df1b10ebc38bfb0a6c365e64fc0
1 parent a2f0a43 commit 5a29c68

2 files changed

Lines changed: 59 additions & 0 deletions

File tree

‎packages/react-native/ReactCommon/react/renderer/animated/internal/NativeAnimatedAllowlist.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ inline const std::unordered_set<std::string> &getDirectManipulationAllowlist()
4848
"borderStartStartRadius",
4949
"elevation",
5050
"opacity",
51+
"filter",
5152
"transform",
5253
"zIndex",
5354
/* ios styles */

‎packages/react-native/ReactCommon/react/renderer/animated/tests/AnimatedNodeTests.cpp‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
#include "AnimationTestsBase.h"
99

10+
#include <react/renderer/animated/internal/NativeAnimatedAllowlist.h>
1011
#include <react/renderer/animated/nodes/ColorAnimatedNode.h>
1112
#include <react/renderer/animated/nodes/ObjectAnimatedNode.h>
1213
#include <react/renderer/core/ReactRootViewTagGenerator.h>
@@ -332,4 +333,61 @@ TEST_F(AnimatedNodeTests, ObjectAnimatedNode) {
332333
EXPECT_EQ(collectedProps["test"][2]["scale3d"], 4);
333334
}
334335

336+
// Styles that Animated supports and that do not participate in layout must be
337+
// direct-manipulation eligible; anything absent from this set is treated as a
338+
// layout update by StyleAnimatedNode and forced through a Fabric commit.
339+
// Keep in sync with SUPPORTED_STYLES in
340+
// packages/react-native/Libraries/Animated/NativeAnimatedAllowlist.js.
341+
TEST_F(AnimatedNodeTests, directManipulationAllowlistCoversNonLayoutStyles) {
342+
const auto& allowlist = getDirectManipulationAllowlist();
343+
344+
for (const auto& style :
345+
{"backgroundColor",
346+
"borderBottomColor",
347+
"borderColor",
348+
"borderEndColor",
349+
"borderLeftColor",
350+
"borderRightColor",
351+
"borderStartColor",
352+
"borderTopColor",
353+
"color",
354+
"tintColor",
355+
"borderBottomEndRadius",
356+
"borderBottomLeftRadius",
357+
"borderBottomRightRadius",
358+
"borderBottomStartRadius",
359+
"borderEndEndRadius",
360+
"borderEndStartRadius",
361+
"borderRadius",
362+
"borderTopEndRadius",
363+
"borderTopLeftRadius",
364+
"borderTopRightRadius",
365+
"borderTopStartRadius",
366+
"borderStartEndRadius",
367+
"borderStartStartRadius",
368+
"elevation",
369+
"opacity",
370+
"filter",
371+
"transform",
372+
"zIndex",
373+
"shadowOpacity",
374+
"shadowRadius",
375+
"scaleX",
376+
"scaleY",
377+
"translateX",
378+
"translateY"}) {
379+
EXPECT_EQ(allowlist.count(style), 1u)
380+
<< style
381+
<< " is animatable and does not affect layout, so it must be "
382+
"direct-manipulation eligible";
383+
}
384+
385+
// Layout styles must stay out, so they keep going through Fabric.
386+
for (const auto& style :
387+
{"width", "height", "margin", "padding", "flex", "top", "gap"}) {
388+
EXPECT_EQ(allowlist.count(style), 0u)
389+
<< style << " affects layout and must not be direct-manipulated";
390+
}
391+
}
392+
335393
} // namespace facebook::react

0 commit comments

Comments
 (0)