Dirty nodes when dynamically setting config (#37207)

Summary: X-link: facebook/react-native#37207 Pull Request resolved: facebook#1274 Yoga exposes public APIs for dirtying Nodes, but will itself perform dirty marking when changing bits which invalidate layout. E.g. changing the style of a Node will invalidate it along with every parent Node. Because config setting is newly public to the C ABI, this makes a similar change so that replacing a Node's config will dirty the tree above the node if there is a layout impacting config change (I don't think children need to be invalidated since child output shouldn't change given the same owner dimensions). One quirk of this is that configs may be changed independently of the node. So someone could attach a config to a Node, then change the live config after the fact. The config does not currently have a back pointer to the Node, so we do not invalidate in that case of live config edits. The future work to rectify this would be to make configs immutable once created. There are also currently some experimental features here which should maybe be compared, but these should be moved to YGErrata anyway. Reviewed By: javache Differential Revision: D45505089 fbshipit-source-id: 7c70e2c2233165de52f856a8d3a2e9e2481028a8
NickGerleman · May 7, 2023 · 339edec · 339edec
1 parent 54d7892
commit 339edec
Show file tree

Hide file tree

Showing 4 changed files with 131 additions and 11 deletions.
diff --git a/tests/YGDirtyMarkingTest.cpp b/tests/YGDirtyMarkingTest.cpp
@@ -68,6 +68,99 @@ TEST(YogaTest, dirty_propagation_only_if_prop_changed) {
   YGNodeFreeRecursive(root);
 }
 
+TEST(YogaTest, dirty_propagation_changing_layout_config) {
+  const YGNodeRef root = YGNodeNew();
+  YGNodeStyleSetAlignItems(root, YGAlignFlexStart);
+  YGNodeStyleSetWidth(root, 100);
+  YGNodeStyleSetHeight(root, 100);
+
+  const YGNodeRef root_child0 = YGNodeNew();
+  YGNodeStyleSetWidth(root_child0, 50);
+  YGNodeStyleSetHeight(root_child0, 20);
+  YGNodeInsertChild(root, root_child0, 0);
+
+  const YGNodeRef root_child1 = YGNodeNew();
+  YGNodeStyleSetWidth(root_child1, 50);
+  YGNodeStyleSetHeight(root_child1, 20);
+  YGNodeInsertChild(root, root_child1, 1);
+
+  const YGNodeRef root_child0_child0 = YGNodeNew();
+  YGNodeStyleSetWidth(root_child0_child0, 25);
+  YGNodeStyleSetHeight(root_child0_child0, 20);
+  YGNodeInsertChild(root, root_child0_child0, 0);
+
+  YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
+
+  EXPECT_FALSE(root->isDirty());
+  EXPECT_FALSE(root_child0->isDirty());
+  EXPECT_FALSE(root_child1->isDirty());
+  EXPECT_FALSE(root_child0_child0->isDirty());
+
+  YGConfigRef newConfig = YGConfigNew();
+  YGConfigSetErrata(newConfig, YGErrataStretchFlexBasis);
+  YGNodeSetConfig(root_child0, newConfig);
+
+  EXPECT_TRUE(root->isDirty());
+  EXPECT_TRUE(root_child0->isDirty());
+  EXPECT_FALSE(root_child1->isDirty());
+  EXPECT_FALSE(root_child0_child0->isDirty());
+
+  YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
+
+  EXPECT_FALSE(root->isDirty());
+  EXPECT_FALSE(root_child0->isDirty());
+  EXPECT_FALSE(root_child1->isDirty());
+  EXPECT_FALSE(root_child0_child0->isDirty());
+
+  YGConfigFree(newConfig);
+  YGNodeFreeRecursive(root);
+}
+
+TEST(YogaTest, dirty_propagation_changing_benign_config) {
+  const YGNodeRef root = YGNodeNew();
+  YGNodeStyleSetAlignItems(root, YGAlignFlexStart);
+  YGNodeStyleSetWidth(root, 100);
+  YGNodeStyleSetHeight(root, 100);
+
+  const YGNodeRef root_child0 = YGNodeNew();
+  YGNodeStyleSetWidth(root_child0, 50);
+  YGNodeStyleSetHeight(root_child0, 20);
+  YGNodeInsertChild(root, root_child0, 0);
+
+  const YGNodeRef root_child1 = YGNodeNew();
+  YGNodeStyleSetWidth(root_child1, 50);
+  YGNodeStyleSetHeight(root_child1, 20);
+  YGNodeInsertChild(root, root_child1, 1);
+
+  const YGNodeRef root_child0_child0 = YGNodeNew();
+  YGNodeStyleSetWidth(root_child0_child0, 25);
+  YGNodeStyleSetHeight(root_child0_child0, 20);
+  YGNodeInsertChild(root, root_child0_child0, 0);
+
+  YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
+
+  EXPECT_FALSE(root->isDirty());
+  EXPECT_FALSE(root_child0->isDirty());
+  EXPECT_FALSE(root_child1->isDirty());
+  EXPECT_FALSE(root_child0_child0->isDirty());
+
+  YGConfigRef newConfig = YGConfigNew();
+  YGConfigSetLogger(
+      newConfig,
+      [](const YGConfigRef, const YGNodeRef, YGLogLevel, const char*, va_list) {
+        return 0;
+      });
+  YGNodeSetConfig(root_child0, newConfig);
+
+  EXPECT_FALSE(root->isDirty());
+  EXPECT_FALSE(root_child0->isDirty());
+  EXPECT_FALSE(root_child1->isDirty());
+  EXPECT_FALSE(root_child0_child0->isDirty());
+
+  YGConfigFree(newConfig);
+  YGNodeFreeRecursive(root);
+}
+
 TEST(YogaTest, dirty_mark_all_children_as_dirty_when_display_changes) {
   const YGNodeRef root = YGNodeNew();
   YGNodeStyleSetFlexDirection(root, YGFlexDirectionRow);

diff --git a/yoga/YGConfig.cpp b/yoga/YGConfig.cpp
@@ -7,7 +7,18 @@
 
 #include "YGConfig.h"
 
-using namespace facebook::yoga::detail;
+using namespace facebook::yoga;
+
+namespace facebook {
+namespace yoga {
+bool configUpdateInvalidatesLayout(YGConfigRef a, YGConfigRef b) {
+  return a->getErrata() != b->getErrata() ||
+      a->getEnabledExperiments() != b->getEnabledExperiments() ||
+      a->getPointScaleFactor() != b->getPointScaleFactor() ||
+      a->useWebDefaults() != b->useWebDefaults();
+}
+} // namespace yoga
+} // namespace facebook
 
 YGConfig::YGConfig(YGLogger logger) : cloneNodeCallback_{nullptr} {
   setLogger(logger);
@@ -40,6 +51,10 @@ bool YGConfig::isExperimentalFeatureEnabled(
   return experimentalFeatures_.test(feature);
 }
 
+ExperimentalFeatureSet YGConfig::getEnabledExperiments() const {
+  return experimentalFeatures_;
+}
+
 void YGConfig::setErrata(YGErrata errata) {
   errata_ = errata;
 }

diff --git a/yoga/YGConfig.h b/yoga/YGConfig.h
@@ -14,8 +14,13 @@
 
 namespace facebook {
 namespace yoga {
-namespace detail {
 
+// Whether moving a node from config "a" to config "b" should dirty previously
+// calculated layout results.
+bool configUpdateInvalidatesLayout(YGConfigRef a, YGConfigRef b);
+
+// Internal variants of log functions, currently used only by JNI bindings.
+// TODO: Reconcile this with the public API
 using LogWithContextFn = int (*)(
     YGConfigRef config,
     YGNodeRef node,
@@ -29,8 +34,12 @@ using CloneWithContextFn = YGNodeRef (*)(
     int childIndex,
     void* cloneContext);
 
+using ExperimentalFeatureSet =
+    facebook::yoga::detail::EnumBitset<YGExperimentalFeature>;
+
 #pragma pack(push)
 #pragma pack(1)
+// Packed structure of <32-bit options to miminize size per node.
 struct YGConfigFlags {
   bool useWebDefaults : 1;
   bool printTree : 1;
@@ -39,7 +48,6 @@ struct YGConfigFlags {
 };
 #pragma pack(pop)
 
-} // namespace detail
 } // namespace yoga
 } // namespace facebook
 
@@ -56,6 +64,7 @@ struct YOGA_EXPORT YGConfig {
       YGExperimentalFeature feature,
       bool enabled);
   bool isExperimentalFeatureEnabled(YGExperimentalFeature feature) const;
+  facebook::yoga::ExperimentalFeatureSet getEnabledExperiments() const;
 
   void setErrata(YGErrata errata);
   YGErrata getErrata() const;
@@ -67,13 +76,12 @@ struct YOGA_EXPORT YGConfig {
   void* getContext() const;
 
   void setLogger(YGLogger logger);
-  void setLogger(facebook::yoga::detail::LogWithContextFn logger);
+  void setLogger(facebook::yoga::LogWithContextFn logger);
   void setLogger(std::nullptr_t);
   void log(YGConfig*, YGNode*, YGLogLevel, void*, const char*, va_list) const;
 
   void setCloneNodeCallback(YGCloneNodeFunc cloneNode);
-  void setCloneNodeCallback(
-      facebook::yoga::detail::CloneWithContextFn cloneNode);
+  void setCloneNodeCallback(facebook::yoga::CloneWithContextFn cloneNode);
   void setCloneNodeCallback(std::nullptr_t);
   YGNodeRef cloneNode(
       YGNodeRef node,
@@ -83,17 +91,16 @@ struct YOGA_EXPORT YGConfig {
 
 private:
   union {
-    facebook::yoga::detail::CloneWithContextFn withContext;
+    facebook::yoga::CloneWithContextFn withContext;
     YGCloneNodeFunc noContext;
   } cloneNodeCallback_;
   union {
-    facebook::yoga::detail::LogWithContextFn withContext;
+    facebook::yoga::LogWithContextFn withContext;
     YGLogger noContext;
   } logger_;
 
-  facebook::yoga::detail::YGConfigFlags flags_{};
-  facebook::yoga::detail::EnumBitset<YGExperimentalFeature>
-      experimentalFeatures_{};
+  facebook::yoga::YGConfigFlags flags_{};
+  facebook::yoga::ExperimentalFeatureSet experimentalFeatures_{};
   YGErrata errata_ = YGErrataNone;
   float pointScaleFactor_ = 1.0f;
   void* context_ = nullptr;

diff --git a/yoga/YGNode.cpp b/yoga/YGNode.cpp
@@ -269,6 +269,11 @@ void YGNode::setConfig(YGConfigRef config) {
       config,
       config->useWebDefaults() == config_->useWebDefaults(),
       "UseWebDefaults may not be changed after constructing a YGNode");
+
+  if (yoga::configUpdateInvalidatesLayout(config_, config)) {
+    markDirtyAndPropagate();
+  }
+
   config_ = config;
 }