From 31680559b4e8b882614b1c11837f90aa38639790 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Tue, 10 Oct 2023 11:43:14 -0700 Subject: [PATCH] Deterministic onLayout event ordering for iOS Paper (#40748) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/40748 The ordering of `onLayout` events is non-deterministic on iOS Paper, due to nodes being added to an `NSHashTable` before iteration, instead of an ordered collection. We don't do any lookups on the collection, so I think this was chosen over `NSMutableArray` for the sake of `[NSHashTable weakObjectsHashTable]`, to avoid retain/release. Using a collection which does retain/release seems to cause a crash due to double release or similar, so those semantics seem intentional (though I'm not super familiar with the model here). We can replicate the memory semantics with ordering by using `NSPointerArray` (which is unfortunately not parameterized). This change does that, so we get consistently top-down layout events (matching Fabric, and Android Paper as of D49627996). This lets us use multiple layout events to calculate right/bottom edge insets deterministically. Changelog: [iOS][Changed] - Deterministic onLayout event ordering for iOS Paper Reviewed By: luluwu2032 Differential Revision: D50093411 fbshipit-source-id: f6a9d5c973b97aede879baa8b952cc1be2447f28 --- .../React/Base/Surface/RCTSurfaceRootShadowView.h | 2 +- .../React/Base/Surface/RCTSurfaceRootShadowView.m | 2 +- packages/react-native/React/Modules/RCTUIManager.m | 2 +- packages/react-native/React/Views/RCTLayout.h | 2 +- packages/react-native/React/Views/RCTRootShadowView.h | 2 +- packages/react-native/React/Views/RCTRootShadowView.m | 2 +- packages/react-native/React/Views/RCTShadowView.m | 2 +- packages/rn-tester/RNTesterUnitTests/RCTShadowViewTests.m | 4 ++-- 8 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.h b/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.h index 2898b16b385d15..4e0ed8458d2ce1 100644 --- a/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.h +++ b/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.h @@ -27,6 +27,6 @@ */ @property (nonatomic, assign) YGDirection baseDirection; -- (void)layoutWithAffectedShadowViews:(NSHashTable *)affectedShadowViews; +- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews; @end diff --git a/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.m b/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.m index 37c551cb4bd318..95cf3390657f6c 100644 --- a/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.m +++ b/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.m @@ -41,7 +41,7 @@ - (void)insertReactSubview:(RCTShadowView *)subview atIndex:(NSInteger)atIndex } } -- (void)layoutWithAffectedShadowViews:(NSHashTable *)affectedShadowViews +- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews { NSHashTable *other = [NSHashTable new]; diff --git a/packages/react-native/React/Modules/RCTUIManager.m b/packages/react-native/React/Modules/RCTUIManager.m index 7027921fda706b..9fe11430c8b7f0 100644 --- a/packages/react-native/React/Modules/RCTUIManager.m +++ b/packages/react-native/React/Modules/RCTUIManager.m @@ -534,7 +534,7 @@ - (RCTViewManagerUIBlock)uiBlockWithLayoutUpdateForRootView:(RCTRootShadowView * { RCTAssertUIManagerQueue(); - NSHashTable *affectedShadowViews = [NSHashTable weakObjectsHashTable]; + NSPointerArray *affectedShadowViews = [NSPointerArray weakObjectsPointerArray]; [rootShadowView layoutWithAffectedShadowViews:affectedShadowViews]; if (!affectedShadowViews.count) { diff --git a/packages/react-native/React/Views/RCTLayout.h b/packages/react-native/React/Views/RCTLayout.h index 55340ad938d1bf..8016463e145cb6 100644 --- a/packages/react-native/React/Views/RCTLayout.h +++ b/packages/react-native/React/Views/RCTLayout.h @@ -31,7 +31,7 @@ typedef struct CG_BOXABLE RCTLayoutMetrics RCTLayoutMetrics; struct RCTLayoutContext { CGPoint absolutePosition; - __unsafe_unretained NSHashTable *_Nonnull affectedShadowViews; + __unsafe_unretained NSPointerArray *_Nonnull affectedShadowViews; __unsafe_unretained NSHashTable *_Nonnull other; }; typedef struct CG_BOXABLE RCTLayoutContext RCTLayoutContext; diff --git a/packages/react-native/React/Views/RCTRootShadowView.h b/packages/react-native/React/Views/RCTRootShadowView.h index a312d9abfede82..aecfc4d0a6ecef 100644 --- a/packages/react-native/React/Views/RCTRootShadowView.h +++ b/packages/react-native/React/Views/RCTRootShadowView.h @@ -29,6 +29,6 @@ */ @property (nonatomic, assign) YGDirection baseDirection; -- (void)layoutWithAffectedShadowViews:(NSHashTable *)affectedShadowViews; +- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews; @end diff --git a/packages/react-native/React/Views/RCTRootShadowView.m b/packages/react-native/React/Views/RCTRootShadowView.m index 9db376d7c49fd9..b80ab28ba8531e 100644 --- a/packages/react-native/React/Views/RCTRootShadowView.m +++ b/packages/react-native/React/Views/RCTRootShadowView.m @@ -23,7 +23,7 @@ - (instancetype)init return self; } -- (void)layoutWithAffectedShadowViews:(NSHashTable *)affectedShadowViews +- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews { NSHashTable *other = [NSHashTable new]; diff --git a/packages/react-native/React/Views/RCTShadowView.m b/packages/react-native/React/Views/RCTShadowView.m index e0084ea7f1d656..2ef25b1a200aef 100644 --- a/packages/react-native/React/Views/RCTShadowView.m +++ b/packages/react-native/React/Views/RCTShadowView.m @@ -304,7 +304,7 @@ - (void)layoutWithMetrics:(RCTLayoutMetrics)layoutMetrics layoutContext:(RCTLayo { if (!RCTLayoutMetricsEqualToLayoutMetrics(self.layoutMetrics, layoutMetrics)) { self.layoutMetrics = layoutMetrics; - [layoutContext.affectedShadowViews addObject:self]; + [layoutContext.affectedShadowViews addPointer:((__bridge void *)self)]; } } diff --git a/packages/rn-tester/RNTesterUnitTests/RCTShadowViewTests.m b/packages/rn-tester/RNTesterUnitTests/RCTShadowViewTests.m index 0bf29c2d336fe5..f63a54b327a8b5 100644 --- a/packages/rn-tester/RNTesterUnitTests/RCTShadowViewTests.m +++ b/packages/rn-tester/RNTesterUnitTests/RCTShadowViewTests.m @@ -84,7 +84,7 @@ - (void)testApplyingLayoutRecursivelyToShadowView [self.parentView insertReactSubview:mainView atIndex:1]; [self.parentView insertReactSubview:footerView atIndex:2]; - [self.parentView layoutWithAffectedShadowViews:[NSHashTable weakObjectsHashTable]]; + [self.parentView layoutWithAffectedShadowViews:[NSPointerArray weakObjectsPointerArray]]; XCTAssertTrue( CGRectEqualToRect([self.parentView measureLayoutRelativeToAncestor:self.parentView], CGRectMake(0, 0, 440, 440))); @@ -187,7 +187,7 @@ - (void)_withShadowViewWithStyle:(void (^)(YGNodeRef node))configBlock RCTShadowView *view = [self _shadowViewWithConfig:configBlock]; [self.parentView insertReactSubview:view atIndex:0]; view.intrinsicContentSize = contentSize; - [self.parentView layoutWithAffectedShadowViews:[NSHashTable weakObjectsHashTable]]; + [self.parentView layoutWithAffectedShadowViews:[NSPointerArray weakObjectsPointerArray]]; CGRect actualRect = [view measureLayoutRelativeToAncestor:self.parentView]; XCTAssertTrue( CGRectEqualToRect(expectedRect, actualRect),