Skip to content

Commit e22e64c

Browse files
safaiyehkligarski
andauthored
fix(iOS): unregister tabs accessory observer from observed wrapper (#3948)
## Description Fixes a possible iOS 26 bottom accessory crash during invalidation. The helper registers KVO for `center` on `self.nativeWrapperView`, but invalidation removed the observer from `_bottomAccessoryView.superview.superview` at teardown time. UIKit can detach or swap the bottom accessory wrapper before invalidation runs, so the teardown path may attempt to remove the observer from a different wrapper view and raise an `NSRangeException` because that object was never observed. I do not see an existing issue for this exact stack, but we observed it in production on `react-native-screens` 4.23.0, and the same registration/removal pattern is present on current `main`. ## Changes - Stores the exact native wrapper view that was registered for KVO. - Makes `registerForAccessoryFrameChanges` idempotent for the same wrapper and unregisters the previous wrapper if the wrapper changes. - Removes the observer from the stored observed wrapper during invalidation instead of resolving the current superview chain again. - Uses a dedicated KVO context and forwards unrelated observations to `super`. ## Test plan - `yarn install --immutable` - `yarn prepare` - `yarn check-types` - `xcrun clang-format -i ios/tabs/bottom-accessory/RNSTabsBottomAccessoryHelper.mm` - `git diff --check` Existing examples that exercise bottom accessory behavior: - `apps/src/tests/issue-tests/Test3288.tsx` - `apps/src/tests/single-feature-tests/tabs/bottom-accessory-layout.tsx` I did not add a JS unit test for this because the failure is caused by UIKit/KVO object identity during native view invalidation. A meaningful automated regression test would need a native iOS XCTest harness or an iOS integration/e2e test that drives bottom accessory mount, detach, and invalidation lifecycle. This repository does not appear to have an iOS native unit-test target for library internals today. I attempted `yarn test:unit` after initializing the `react-navigation` submodule, but the root Jest command also collects the submodule's own test suites and fails on unrelated submodule Jest setup / React version issues before exercising this native change. ## Checklist - [ ] Included code example that can be used to test this change. Existing bottom accessory examples are listed above; no new JS example was added for this native KVO lifecycle fix. - [x] For visual changes, included screenshots / GIFs / recordings documenting the change. N/A, no visual changes. - [x] For API changes, updated relevant public types. N/A, no API changes. - [x] Ensured that CI passes --------- Co-authored-by: Krzysztof Ligarski <[email protected]>
1 parent d3abbe7 commit e22e64c

1 file changed

Lines changed: 34 additions & 5 deletions

File tree

ios/tabs/bottom-accessory/RNSTabsBottomAccessoryHelper.mm

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,11 @@
88

99
namespace react = facebook::react;
1010

11+
static void *RNSTabsBottomAccessoryNativeWrapperViewContext = &RNSTabsBottomAccessoryNativeWrapperViewContext;
12+
1113
@implementation RNSTabsBottomAccessoryHelper {
1214
RNSTabsBottomAccessoryComponentView *__weak _bottomAccessoryView;
15+
UIView *__weak _observedNativeWrapperView;
1316

1417
#if REACT_NATIVE_VERSION_MINOR < 82
1518
BOOL _initialStateUpdateSent;
@@ -35,6 +38,7 @@ - (instancetype)initWithBottomAccessoryView:(RNSTabsBottomAccessoryComponentView
3538

3639
- (void)initState
3740
{
41+
_observedNativeWrapperView = nil;
3842
#if REACT_NATIVE_VERSION_MINOR < 82
3943
_initialStateUpdateSent = NO;
4044
_displayLink = nil;
@@ -110,17 +114,44 @@ - (void)handleContentViewVisibilityForEnvironmentIfNeeded
110114

111115
#pragma mark - Observing frame changes
112116

117+
- (void)unregisterForAccessoryFrameChanges
118+
{
119+
UIView *observedNativeWrapperView = _observedNativeWrapperView;
120+
if (observedNativeWrapperView == nil) {
121+
return;
122+
}
123+
124+
[observedNativeWrapperView removeObserver:self
125+
forKeyPath:@"center"
126+
context:RNSTabsBottomAccessoryNativeWrapperViewContext];
127+
_observedNativeWrapperView = nil;
128+
}
129+
113130
- (void)registerForAccessoryFrameChanges
114131
{
115-
[self.nativeWrapperView addObserver:self forKeyPath:@"center" options:NSKeyValueObservingOptionInitial context:nil];
132+
UIView *nativeWrapperView = self.nativeWrapperView;
133+
if (_observedNativeWrapperView == nativeWrapperView) {
134+
return;
135+
}
136+
137+
[self unregisterForAccessoryFrameChanges];
138+
[nativeWrapperView addObserver:self
139+
forKeyPath:@"center"
140+
options:NSKeyValueObservingOptionInitial
141+
context:RNSTabsBottomAccessoryNativeWrapperViewContext];
142+
_observedNativeWrapperView = nativeWrapperView;
116143
}
117144

118145
- (void)observeValueForKeyPath:(NSString *)keyPath
119146
ofObject:(id)object
120147
change:(NSDictionary *)change
121148
context:(void *)context
122149
{
123-
[self notifyWrapperViewFrameHasChanged];
150+
if (context == RNSTabsBottomAccessoryNativeWrapperViewContext) {
151+
[self notifyWrapperViewFrameHasChanged];
152+
} else {
153+
[super observeValueForKeyPath:keyPath ofObject:object change:change context:context];
154+
}
124155
}
125156

126157
- (UIView *)nativeWrapperView
@@ -192,9 +223,7 @@ - (void)invalidate
192223
{
193224
[_bottomAccessoryView unregisterForTraitChanges:_traitChangeRegistration];
194225
_traitChangeRegistration = nil;
195-
// Using nativeWrapperView directly here to avoid failing RCTAssert in self.nativeWrapperView.
196-
// If we're called from didMoveToWindow, it's not a problem, but I'm not sure if this will always be the case.
197-
[_bottomAccessoryView.superview.superview removeObserver:self forKeyPath:@"center"];
226+
[self unregisterForAccessoryFrameChanges];
198227
_bottomAccessoryView = nil;
199228
#if REACT_NATIVE_VERSION_MINOR < 82
200229
[self invalidateDisplayLink];

0 commit comments

Comments
 (0)