Fix merge options leaks to next screen in stack (#5859)

* Fix merge options leaks to next screen in stack * Rename mergeChild:options to mergeChildOptions:options: * Rename mergeChildOptions:options: to mergeChildOptions:child:
wix · Jan 21, 2020 · 386bf65 · 386bf65
1 parent 6521177
commit 386bf65
Show file tree

Hide file tree

Showing 14 changed files with 89 additions and 21 deletions.
diff --git a/e2e/Options.test.js b/e2e/Options.test.js
@@ -63,6 +63,19 @@ describe('Options', () => {
     await expect(elementByLabel('Styling Options')).toBeVisible();
   });
 
+  it('Merging options to invisible component in stack should not affect current component', async () => {
+    await elementById(TestIDs.PUSH_BTN).tap();
+    await elementById(TestIDs.HIDE_PREVIOUS_SCREEN_TOP_BAR).tap();
+    await expect(elementByLabel('Pushed Screen')).toBeVisible();
+  });
+
+  it('Merging options to invisible component should affect the invisible component', async () => {
+    await elementById(TestIDs.PUSH_BTN).tap();
+    await elementById(TestIDs.HIDE_PREVIOUS_SCREEN_TOP_BAR).tap();
+    await elementById(TestIDs.POP_BTN).tap();
+    await expect(elementByLabel('Styling Options')).toBeNotVisible();
+  });
+
   xit('hides topBar onScroll down and shows it on scroll up', async () => {
     await elementById(TestIDs.PUSH_OPTIONS_BUTTON).tap();
     await elementById(TestIDs.SCROLLVIEW_SCREEN_BUTTON).tap();

diff --git a/lib/ios/RNNLayoutProtocol.h b/lib/ios/RNNLayoutProtocol.h
@@ -29,6 +29,8 @@ typedef void (^RNNReactViewReadyCompletionBlock)(void);
 
 - (void)mergeOptions:(RNNNavigationOptions *)options;
 
+- (void)mergeChildOptions:(RNNNavigationOptions *)options child:(UIViewController *)child;
+
 - (RNNNavigationOptions *)resolveOptions;
 
 - (void)setDefaultOptions:(RNNNavigationOptions *)defaultOptions;

diff --git a/lib/ios/RNNStackController.m b/lib/ios/RNNStackController.m
@@ -1,5 +1,6 @@
 #import "RNNStackController.h"
 #import "RNNComponentViewController.h"
+#import "UIViewController+Utils.h"
 
 @implementation RNNStackController {
     UIViewController* _presentedViewController;
@@ -21,6 +22,13 @@ - (void)viewDidLayoutSubviews {
 	[self.presenter applyOptionsOnViewDidLayoutSubviews:self.resolveOptions];
 }
 
+- (void)mergeChildOptions:(RNNNavigationOptions *)options child:(UIViewController *)child {
+    if (child.isLastInStack) {
+        [self.presenter mergeOptions:options resolvedOptions:self.resolveOptions];
+    }
+    [self.parentViewController mergeChildOptions:options child:child];
+}
+
 - (UINavigationController *)navigationController {
 	return self;
 }

diff --git a/lib/ios/UIViewController+LayoutProtocol.h b/lib/ios/UIViewController+LayoutProtocol.h
@@ -16,7 +16,7 @@ typedef void (^RNNReactViewReadyCompletionBlock)(void);
 
 - (void)mergeOptions:(RNNNavigationOptions *)options;
 
-- (void)mergeChildOptions:(RNNNavigationOptions *)options;
+- (void)mergeChildOptions:(RNNNavigationOptions *)options child:(UIViewController *)child;
 
 - (RNNNavigationOptions *)resolveOptions;
 

diff --git a/lib/ios/UIViewController+LayoutProtocol.m b/lib/ios/UIViewController+LayoutProtocol.m
@@ -31,12 +31,12 @@ - (instancetype)initWithLayoutInfo:(RNNLayoutInfo *)layoutInfo
 - (void)mergeOptions:(RNNNavigationOptions *)options {
     [self.options overrideOptions:options];
     [self.presenter mergeOptions:options resolvedOptions:self.resolveOptions];
-    [self.parentViewController mergeChildOptions:options];
+    [self.parentViewController mergeChildOptions:options child:self];
 }
 
-- (void)mergeChildOptions:(RNNNavigationOptions *)options {
+- (void)mergeChildOptions:(RNNNavigationOptions *)options child:(UIViewController *)child {
     [self.presenter mergeOptions:options resolvedOptions:self.resolveOptions];
-	[self.parentViewController mergeChildOptions:options];
+    [self.parentViewController mergeChildOptions:options child:child];
 }
 
 - (RNNNavigationOptions *)resolveOptions {

diff --git a/lib/ios/Utils/UIViewController+Utils.h b/lib/ios/Utils/UIViewController+Utils.h
@@ -2,5 +2,8 @@
 #import <UIKit/UIKit.h>
 
 @interface UIViewController (Utils)
+
 - (void)forEachChild:(void (^)(UIViewController *child))perform;
-@end
+- (BOOL)isLastInStack;
+
+@end
diff --git a/lib/ios/Utils/UIViewController+Utils.m b/lib/ios/Utils/UIViewController+Utils.m
@@ -1,10 +1,15 @@
 #import "UIViewController+Utils.h"
 
 @implementation UIViewController (Utils)
+
 - (void)forEachChild:(void (^)(UIViewController *child))perform {
     for (UIViewController *child in [self childViewControllers]) {
         perform(child);
     }
 }
 
-@end
+- (BOOL)isLastInStack {
+    return self.navigationController.childViewControllers.lastObject == self;
+}
+
+@end
diff --git a/...gationTests/RNNNavigationControllerTest.m → .../NavigationTests/RNNStackControllerTest.m b/...gationTests/RNNNavigationControllerTest.m → .../NavigationTests/RNNStackControllerTest.m
@@ -4,32 +4,34 @@
 #import <ReactNativeNavigation/RNNComponentViewController.h>
 #import "RNNTestRootViewCreator.h"
 
-@interface RNNNavigationControllerTest : XCTestCase
+@interface RNNStackControllerTest : XCTestCase
 
 @property (nonatomic, strong) RNNStackController *uut;
 
 @end
 
-@implementation RNNNavigationControllerTest {
+@implementation RNNStackControllerTest {
 	RNNComponentViewController* _vc1;
 	id _vc2Mock;
 	RNNComponentViewController* _vc2;
 	UIViewController* _vc3;
 	RNNNavigationOptions* _options;
 	RNNTestRootViewCreator* _creator;
 	RNNEventEmitter* _eventEmitter;
+	id _presenter;
 }
 
 - (void)setUp {
     [super setUp];
+	_presenter = [OCMockObject partialMockForObject:[[RNNStackPresenter alloc] init]];
 	_eventEmitter = [OCMockObject niceMockForClass:[RNNEventEmitter class]];
 	_creator = [[RNNTestRootViewCreator alloc] init];
 	_vc1 = [[RNNComponentViewController alloc] initWithLayoutInfo:[RNNLayoutInfo new] rootViewCreator:nil eventEmitter:nil presenter:[OCMockObject partialMockForObject:[[RNNComponentPresenter alloc] init]] options:[[RNNNavigationOptions alloc] initEmptyOptions] defaultOptions:[[RNNNavigationOptions alloc] initEmptyOptions]];
 	_vc2 = [[RNNComponentViewController alloc] initWithLayoutInfo:[RNNLayoutInfo new] rootViewCreator:nil eventEmitter:nil presenter:[[RNNComponentPresenter alloc] init] options:[[RNNNavigationOptions alloc] initEmptyOptions] defaultOptions:[[RNNNavigationOptions alloc] initEmptyOptions]];
 	_vc2Mock = [OCMockObject partialMockForObject:_vc2];
 	_vc3 = [UIViewController new];
 	_options = [OCMockObject partialMockForObject:[[RNNNavigationOptions alloc] initEmptyOptions]];
-	self.uut = [[RNNStackController alloc] initWithLayoutInfo:nil creator:_creator options:_options defaultOptions:nil presenter:[OCMockObject partialMockForObject:[[RNNStackPresenter alloc] init]] eventEmitter:_eventEmitter childViewControllers:@[_vc1, _vc2]];
+	self.uut = [[RNNStackController alloc] initWithLayoutInfo:nil creator:_creator options:_options defaultOptions:nil presenter:_presenter eventEmitter:_eventEmitter childViewControllers:@[_vc1, _vc2]];
 }
 
 - (void)testInitWithLayoutInfo_shouldBindPresenter {
@@ -167,6 +169,22 @@ - (void)testOverrideOptionsShouldOverrideOptionsState {
 	[(id)self.uut.options verify];
 }
 
+- (void)testMergeChildOptionsShouldUpdatePresenterForVisibleChild {
+	RNNNavigationOptions* options = [[RNNNavigationOptions alloc] initEmptyOptions];
+
+	[[_presenter expect] mergeOptions:options resolvedOptions:[OCMArg any]];
+	[self.uut mergeChildOptions:options child:self.uut.childViewControllers.lastObject];
+	[_presenter verify];
+}
+
+- (void)testMergeChildOptionsShouldNotUpdatePresenterForInvisibleChild {
+	RNNNavigationOptions* options = [[RNNNavigationOptions alloc] initEmptyOptions];
+
+	[[_presenter reject] mergeOptions:options resolvedOptions:self.uut.resolveOptions];
+	[self.uut mergeChildOptions:options child:self.uut.childViewControllers.firstObject];
+	[_presenter verify];
+}
+
 - (RNNStackController *)createNavigationControllerWithOptions:(RNNNavigationOptions *)options {
 	RNNStackController* nav = [[RNNStackController alloc] initWithLayoutInfo:nil creator:_creator options:options defaultOptions:nil presenter:[[RNNStackPresenter alloc] init] eventEmitter:nil childViewControllers:@[_vc1]];
 	return nav;

diff --git a/playground/ios/NavigationTests/RNNTabBarControllerTest.m b/playground/ios/NavigationTests/RNNTabBarControllerTest.m
@@ -7,6 +7,7 @@
 
 @interface RNNTabBarControllerTest : XCTestCase
 
+@property(nonatomic, strong) RNNBottomTabsController * originalUut;
 @property(nonatomic, strong) RNNBottomTabsController * uut;
 @property(nonatomic, strong) id mockChildViewController;
 @property(nonatomic, strong) id mockEventEmitter;
@@ -25,7 +26,8 @@ - (void)setUp {
     self.mockTabBarPresenter = [OCMockObject partialMockForObject:[[RNNBottomTabsPresenter alloc] init]];
     self.mockChildViewController = [OCMockObject partialMockForObject:[RNNComponentViewController new]];
     self.mockEventEmitter = [OCMockObject partialMockForObject:[RNNEventEmitter new]];
-    self.uut = [OCMockObject partialMockForObject:[[RNNBottomTabsController alloc] initWithLayoutInfo:nil creator:nil options:[[RNNNavigationOptions alloc] initWithDict:@{}] defaultOptions:nil presenter:self.mockTabBarPresenter eventEmitter:self.mockEventEmitter childViewControllers:@[[[UIViewController alloc] init]]]];
+	self.originalUut = [[RNNBottomTabsController alloc] initWithLayoutInfo:nil creator:nil options:[[RNNNavigationOptions alloc] initWithDict:@{}] defaultOptions:nil presenter:self.mockTabBarPresenter eventEmitter:self.mockEventEmitter childViewControllers:@[[[UIViewController alloc] init]]];
+    self.uut = [OCMockObject partialMockForObject:self.originalUut];
     OCMStub([self.uut selectedViewController]).andReturn(self.mockChildViewController);
 }
 
@@ -104,11 +106,11 @@ - (void)testMergeOptions_shouldInvokePresenterMergeOptions {
 }
 
 - (void)testMergeOptions_shouldInvokeParentMergeOptions {
-    id parentMock = [OCMockObject partialMockForObject:[RNNComponentViewController new]];
+    id parentMock = [OCMockObject niceMockForClass:[RNNComponentViewController class]];
     RNNNavigationOptions *options = [[RNNNavigationOptions alloc] initWithDict:@{}];
 
     OCMStub([self.uut parentViewController]).andReturn(parentMock);
-    [((RNNComponentViewController *) [parentMock expect]) mergeChildOptions:options];
+    [((RNNComponentViewController *) [parentMock expect]) mergeChildOptions:options child:self.originalUut];
     [self.uut mergeOptions:options];
     [parentMock verify];
 }

diff --git a/playground/ios/NavigationTests/UIViewController+LayoutProtocolTest.m b/playground/ios/NavigationTests/UIViewController+LayoutProtocolTest.m
@@ -63,10 +63,11 @@ - (void)testResolveOptions {
 
 - (void)testMergeOptions_invokedOnParentViewController {
     id parent = [OCMockObject partialMockForObject:[RNNStackController new]];
+	RNNStackController* uut = [[RNNStackController alloc] initWithLayoutInfo:nil creator:nil options:nil defaultOptions:nil presenter:nil eventEmitter:nil childViewControllers:nil];
+
     RNNNavigationOptions * toMerge = [[RNNNavigationOptions alloc] initEmptyOptions];
-    [(UIViewController *) [parent expect] mergeChildOptions:toMerge];
-
-    RNNStackController* uut = [[RNNStackController alloc] initWithLayoutInfo:nil creator:nil options:nil defaultOptions:nil presenter:nil eventEmitter:nil childViewControllers:nil];
+	[(UIViewController *) [parent expect] mergeChildOptions:toMerge child:uut];
+
     [parent addChildViewController:uut];
 
     [uut mergeOptions:toMerge];

diff --git a/playground/ios/playground.xcodeproj/project.pbxproj b/playground/ios/playground.xcodeproj/project.pbxproj
@@ -39,7 +39,7 @@
 		E58D26592385888C003F36BA /* RNNNavigationStackManagerTest.m in Sources */ = {isa = PBXBuildFile; fileRef = E58D263C2385888C003F36BA /* RNNNavigationStackManagerTest.m */; };
 		E58D265A2385888C003F36BA /* RNNTestRootViewCreator.m in Sources */ = {isa = PBXBuildFile; fileRef = E58D263D2385888C003F36BA /* RNNTestRootViewCreator.m */; };
 		E58D265B2385888C003F36BA /* UIViewController+RNNOptionsTest.m in Sources */ = {isa = PBXBuildFile; fileRef = E58D263E2385888C003F36BA /* UIViewController+RNNOptionsTest.m */; };
-		E58D265C2385888C003F36BA /* RNNNavigationControllerTest.m in Sources */ = {isa = PBXBuildFile; fileRef = E58D263F2385888C003F36BA /* RNNNavigationControllerTest.m */; };
+		E58D265C2385888C003F36BA /* RNNStackControllerTest.m in Sources */ = {isa = PBXBuildFile; fileRef = E58D263F2385888C003F36BA /* RNNStackControllerTest.m */; };
 		E58D265D2385888C003F36BA /* RNNControllerFactoryTest.m in Sources */ = {isa = PBXBuildFile; fileRef = E58D26402385888C003F36BA /* RNNControllerFactoryTest.m */; };
 		E58D265E2385888C003F36BA /* RNNCommandsHandlerTest.m in Sources */ = {isa = PBXBuildFile; fileRef = E58D26412385888C003F36BA /* RNNCommandsHandlerTest.m */; };
 		E58D265F2385888C003F36BA /* RNNBasePresenterTest.m in Sources */ = {isa = PBXBuildFile; fileRef = E58D26422385888C003F36BA /* RNNBasePresenterTest.m */; };
@@ -119,7 +119,7 @@
 		E58D263C2385888C003F36BA /* RNNNavigationStackManagerTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RNNNavigationStackManagerTest.m; sourceTree = "<group>"; };
 		E58D263D2385888C003F36BA /* RNNTestRootViewCreator.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RNNTestRootViewCreator.m; sourceTree = "<group>"; };
 		E58D263E2385888C003F36BA /* UIViewController+RNNOptionsTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = "UIViewController+RNNOptionsTest.m"; sourceTree = "<group>"; };
-		E58D263F2385888C003F36BA /* RNNNavigationControllerTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RNNNavigationControllerTest.m; sourceTree = "<group>"; };
+		E58D263F2385888C003F36BA /* RNNStackControllerTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RNNStackControllerTest.m; sourceTree = "<group>"; };
 		E58D26402385888C003F36BA /* RNNControllerFactoryTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RNNControllerFactoryTest.m; sourceTree = "<group>"; };
 		E58D26412385888C003F36BA /* RNNCommandsHandlerTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RNNCommandsHandlerTest.m; sourceTree = "<group>"; };
 		E58D26422385888C003F36BA /* RNNBasePresenterTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RNNBasePresenterTest.m; sourceTree = "<group>"; };
@@ -253,7 +253,7 @@
 				E58D26282385888B003F36BA /* RNNFontAttributesCreatorTest.m */,
 				E58D262A2385888B003F36BA /* RNNLayoutManagerTest.m */,
 				E58D26432385888C003F36BA /* RNNModalManagerTest.m */,
-				E58D263F2385888C003F36BA /* RNNNavigationControllerTest.m */,
+				E58D263F2385888C003F36BA /* RNNStackControllerTest.m */,
 				E58D26322385888B003F36BA /* RNNNavigationOptionsTest.m */,
 				E58D263C2385888C003F36BA /* RNNNavigationStackManagerTest.m */,
 				E58D262C2385888B003F36BA /* RNNOptionsTest.h */,
@@ -755,7 +755,7 @@
 				E58D264E2385888C003F36BA /* RNNTestBase.m in Sources */,
 				E58D26612385888C003F36BA /* RNNTestNoColor.m in Sources */,
 				E58D265D2385888C003F36BA /* RNNControllerFactoryTest.m in Sources */,
-				E58D265C2385888C003F36BA /* RNNNavigationControllerTest.m in Sources */,
+				E58D265C2385888C003F36BA /* RNNStackControllerTest.m in Sources */,
 				E58D26482385888C003F36BA /* RNNDotIndicatorPresenterTest.m in Sources */,
 				E58D26522385888C003F36BA /* RNNComponentPresenterTest.m in Sources */,
 				E58D26572385888C003F36BA /* RNNTabBarPresenterTest.m in Sources */,

diff --git a/playground/src/screens/OptionsScreen.js b/playground/src/screens/OptionsScreen.js
@@ -65,7 +65,14 @@ class Options extends Component {
     }
   });
 
-  push = () => Navigation.push(this, Screens.Pushed);
+  push = () => Navigation.push(this, {
+    component: {
+      name: Screens.Pushed,
+      passProps: {
+        previousScreenIds: [this.props.componentId]
+      }
+    }
+  });
 
   hideTopBarInDefaultOptions = () => {
     Navigation.setDefaultOptions({

diff --git a/playground/src/screens/PushedScreen.js b/playground/src/screens/PushedScreen.js
@@ -15,7 +15,8 @@ const {
   ADD_BACK_HANDLER,
   REMOVE_BACK_HANDLER,
   SET_STACK_ROOT_BUTTON,
-  PUSH_OPTIONS_BUTTON
+  PUSH_OPTIONS_BUTTON,
+  HIDE_PREVIOUS_SCREEN_TOP_BAR
 } = require('../testIDs');
 const Screens = require('./Screens');
 
@@ -58,6 +59,7 @@ class PushedScreen extends React.Component {
         <Button label='Remove BackHandler' testID={REMOVE_BACK_HANDLER} onPress={this.removeBackHandler} />
         <Button label='Set Stack Root' testID={SET_STACK_ROOT_BUTTON} onPress={this.setStackRoot} />
         <Button label='Push Options Screen' testID={PUSH_OPTIONS_BUTTON} onPress={this.pushOptionsScreen} />
+        <Button label='Hide previous screen top bar' testID={HIDE_PREVIOUS_SCREEN_TOP_BAR} onPress={this.hidePreviousScreenTopBar} />
       </Root>
     );
   }
@@ -101,6 +103,12 @@ class PushedScreen extends React.Component {
 
   popToRoot = () => Navigation.popToRoot(this);
 
+  hidePreviousScreenTopBar = () => Navigation.mergeOptions(this.props.previousScreenIds[this.getStackPosition() - 1], {
+    topBar: {
+      visible: false
+    }
+  });
+
   setStackRoot = () => Navigation.setStackRoot(this, [
     {
       component: {

diff --git a/playground/src/testIDs.js b/playground/src/testIDs.js
@@ -135,6 +135,7 @@ module.exports = {
   SHOW_LIFECYCLE_BTN: 'SHOW_LIFECYCLE_BTN',
   CHANGE_BUTTON_PROPS: 'CHANGE_BUTTON_PROPS',
   GOTO_BUTTONS_SCREEN: 'GOTO_BUTTONS_SCREEN',
+  HIDE_PREVIOUS_SCREEN_TOP_BAR: 'HIDE_PREVIOUS_SCREEN_TOP_BAR',
 
   // Elements
   SCROLLVIEW_ELEMENT: `SCROLLVIEW_ELEMENT`,