From 56ddace9c8ffdde5485f1dbe4c5004070cf78262 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 2898b16b385..4e0ed8458d2 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 37c551cb4bd..95cf3390657 100644 --- a/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.m +++ b/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.m @@ -41,7 +41,7 @@ } } -- (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 7027921fda7..9fe11430c8b 100644 --- a/packages/react-native/React/Modules/RCTUIManager.m +++ b/packages/react-native/React/Modules/RCTUIManager.m @@ -534,7 +534,7 @@ static NSDictionary *deviceOrientationEventBody(UIDeviceOrientation orientation) { 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 55340ad938d..8016463e145 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 a312d9abfed..aecfc4d0a6e 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 9db376d7c49..b80ab28ba85 100644 --- a/packages/react-native/React/Views/RCTRootShadowView.m +++ b/packages/react-native/React/Views/RCTRootShadowView.m @@ -23,7 +23,7 @@ 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 e0084ea7f1d..2ef25b1a200 100644 --- a/packages/react-native/React/Views/RCTShadowView.m +++ b/packages/react-native/React/Views/RCTShadowView.m @@ -304,7 +304,7 @@ static void RCTProcessMetaPropsBorder(const YGValue metaProps[META_PROP_COUNT], { 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 0bf29c2d336..f63a54b327a 100644 --- a/packages/rn-tester/RNTesterUnitTests/RCTShadowViewTests.m +++ b/packages/rn-tester/RNTesterUnitTests/RCTShadowViewTests.m @@ -84,7 +84,7 @@ [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 @@ 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),