From 451d13a846e3ba85ce6c24f576bec5cf61bcd2c5 Mon Sep 17 00:00:00 2001 From: David Vacca Date: Fri, 29 Jul 2022 17:32:35 -0700 Subject: [PATCH] Disable calculation of TransformedFrames in Layoutable ShadowNodex Summary: Calculation of TransformedFrames was a method introduced by D37994809 (https://github.com/facebook/react-native/commit/64528e5faa445907b8287b412c344f30c20fca61) to fix rendering of inverted flat lists. We found that this operation is crashing in fb4a causing the UBN T127619309 This diff creates a feature flag to disable the calculation of TransformedFrames in Layoutable ShadowNodes. **The goal of this diff is to revert the behavior introduced by D37994809 (https://github.com/facebook/react-native/commit/64528e5faa445907b8287b412c344f30c20fca61)** The featureFlag is disabled in fb4a and enabled in react AR (because ReactAr apps relies on the calculation of TransformedFrames and these apps are not affected) The root cause of the bug will be fixed by D38280674 (which still requires more testing and investigation) changelog: [internal] internal Reviewed By: JoshuaGross Differential Revision: D38286857 fbshipit-source-id: 721cd0554ae6a6b369b3f8dbb584160a270d0f18 --- .../react/config/ReactFeatureFlags.java | 3 ++ .../com/facebook/react/fabric/jni/Binding.cpp | 4 +++ .../renderer/core/LayoutableShadowNode.cpp | 25 +++++++++++++++-- .../react/renderer/core/ShadowNode.cpp | 4 +++ ReactCommon/react/renderer/core/ShadowNode.h | 5 ++++ .../core/tests/LayoutableShadowNodeTest.cpp | 28 +++++++++++++++++++ 6 files changed, 66 insertions(+), 3 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index a8a5264042ba50..4be9e5bc100189 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -83,6 +83,9 @@ public class ReactFeatureFlags { /** Enables or disables MapBuffer Serialization */ public static boolean mapBufferSerializationEnabled = false; + /** Enables or disables calculation of Transformed Frames */ + public static boolean calculateTransformedFramesEnabled = false; + /** Feature Flag to use overflowInset values provided by Yoga */ public static boolean useOverflowInset = false; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index be2a0f1e749381..83f578955af0f6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -434,6 +434,10 @@ void Binding::installFabricUIManager( "MapBufferSerializationEnabled", getFeatureFlagValue("mapBufferSerializationEnabled")); + contextContainer->insert( + "CalculateTransformedFramesEnabled", + getFeatureFlagValue("calculateTransformedFramesEnabled")); + disablePreallocateViews_ = reactNativeConfig_->getBool( "react_fabric:disabled_view_preallocation_android"); diff --git a/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp b/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp index 7f1802906df70f..7f3c1f0382aba5 100644 --- a/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp +++ b/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp @@ -151,8 +151,21 @@ LayoutMetrics LayoutableShadowNode::computeRelativeLayoutMetrics( } // ------------------------------ - - auto transformedFrames = calculateTransformedFrames(shadowNodeList, policy); + // TODO: T127619309 remove after validating that T127619309 is fixed + auto optionalCalculateTransformedFrames = + descendantNode->getContextContainer() + ? descendantNode->getContextContainer()->find( + "CalculateTransformedFramesEnabled") + : std::optional(false); + + bool shouldCalculateTransformedFrames = + optionalCalculateTransformedFrames.has_value() + ? optionalCalculateTransformedFrames.value() + : false; + + auto transformedFrames = shouldCalculateTransformedFrames + ? calculateTransformedFrames(shadowNodeList, policy) + : LayoutableSmallVector(); auto layoutMetrics = descendantLayoutableNode->getLayoutMetrics(); auto &resultFrame = layoutMetrics.frame; resultFrame.origin = {0, 0}; @@ -168,7 +181,9 @@ LayoutMetrics LayoutableShadowNode::computeRelativeLayoutMetrics( return EmptyLayoutMetrics; } - auto currentFrame = transformedFrames[i]; + auto currentFrame = shouldCalculateTransformedFrames + ? transformedFrames[i] + : currentShadowNode->getLayoutMetrics().frame; if (i == size - 1) { // If it's the last element, its origin is irrelevant. currentFrame.origin = {0, 0}; @@ -185,6 +200,10 @@ LayoutMetrics LayoutableShadowNode::computeRelativeLayoutMetrics( } resultFrame.origin += currentFrame.origin; + if (!shouldCalculateTransformedFrames && i != 0 && + policy.includeTransform) { + resultFrame.origin += currentShadowNode->getContentOriginOffset(); + } } // ------------------------------ diff --git a/ReactCommon/react/renderer/core/ShadowNode.cpp b/ReactCommon/react/renderer/core/ShadowNode.cpp index 203d86e95b0be9..eaab746ac5c5b8 100644 --- a/ReactCommon/react/renderer/core/ShadowNode.cpp +++ b/ReactCommon/react/renderer/core/ShadowNode.cpp @@ -121,6 +121,10 @@ ShadowNode::Unshared ShadowNode::clone( return family_->componentDescriptor_.cloneShadowNode(*this, fragment); } +ContextContainer::Shared ShadowNode::getContextContainer() const { + return family_->componentDescriptor_.getContextContainer(); +} + #pragma mark - Getters ComponentName ShadowNode::getComponentName() const { diff --git a/ReactCommon/react/renderer/core/ShadowNode.h b/ReactCommon/react/renderer/core/ShadowNode.h index 8264065ffb2ee6..a190a0906c6c7d 100644 --- a/ReactCommon/react/renderer/core/ShadowNode.h +++ b/ReactCommon/react/renderer/core/ShadowNode.h @@ -129,6 +129,11 @@ class ShadowNode : public Sealable, public DebugStringConvertible { */ const ComponentDescriptor &getComponentDescriptor() const; + /* + * Returns the `ContextContainer` used by this ShadowNode. + */ + ContextContainer::Shared getContextContainer() const; + /* * Returns a state associated with the particular node. */ diff --git a/ReactCommon/react/renderer/core/tests/LayoutableShadowNodeTest.cpp b/ReactCommon/react/renderer/core/tests/LayoutableShadowNodeTest.cpp index 8fde7fd7122701..f8b0789e48b97d 100644 --- a/ReactCommon/react/renderer/core/tests/LayoutableShadowNodeTest.cpp +++ b/ReactCommon/react/renderer/core/tests/LayoutableShadowNodeTest.cpp @@ -13,6 +13,9 @@ using namespace facebook::react; +// TODO: T127619309 re-enable CalculateTransformedFrames +bool enableCalculateTransformedFrames = false; + /* * ┌────────┐ * │ │ @@ -450,6 +453,11 @@ TEST(LayoutableShadowNodeTest, invertedVerticalView) { auto builder = simpleComponentBuilder(); auto childShadowNode1 = std::shared_ptr{}; auto childShadowNode2 = std::shared_ptr{}; + + if (!enableCalculateTransformedFrames) { + return; + } + // clang-format off auto element = Element() @@ -525,6 +533,11 @@ TEST(LayoutableShadowNodeTest, nestedInvertedVerticalView) { auto builder = simpleComponentBuilder(); auto childShadowNode1 = std::shared_ptr{}; auto childShadowNode2 = std::shared_ptr{}; + + if (!enableCalculateTransformedFrames) { + return; + } + // clang-format off auto element = Element() @@ -601,6 +614,11 @@ TEST(LayoutableShadowNodeTest, invertedHorizontalView) { auto builder = simpleComponentBuilder(); auto childShadowNode1 = std::shared_ptr{}; auto childShadowNode2 = std::shared_ptr{}; + + if (!enableCalculateTransformedFrames) { + return; + } + // clang-format off auto element = Element() @@ -672,6 +690,11 @@ TEST(LayoutableShadowNodeTest, nestedInvertedHorizontalView) { auto builder = simpleComponentBuilder(); auto childShadowNode1 = std::shared_ptr{}; auto childShadowNode2 = std::shared_ptr{}; + + if (!enableCalculateTransformedFrames) { + return; + } + // clang-format off auto element = Element() @@ -738,6 +761,11 @@ TEST(LayoutableShadowNodeTest, inversedContentOriginOffset) { auto builder = simpleComponentBuilder(); auto childShadowNode = std::shared_ptr{}; + + if (!enableCalculateTransformedFrames) { + return; + } + // clang-format off auto element = Element()