From cd205dc743bff9615fe8eaf129aab3c31132939e Mon Sep 17 00:00:00 2001 From: Kacper Kafara Date: Mon, 18 Jul 2022 18:21:39 +0200 Subject: [PATCH 1/4] bookmark: @kkafar/fix-1299 INIT From 94fd0dff5696240f3053a44fed8b7db099b33706 Mon Sep 17 00:00:00 2001 From: Kacper Kafara Date: Tue, 19 Jul 2022 14:28:42 +0200 Subject: [PATCH 2/4] chore: restore default value for stackPresentation in prepareForRecycle method We set this prop to default value here to workaround view-recycling. Let's assume the view has had _stackPresentation == set before below line was executed. Then, when instantiated again (with the same modal presentation) updateProps:oldProps: method would be called and setter for stack presentation would not be called. This is crucial as in that setter we register `self` as a delegate (UIAdaptivePresentationControllerDelegate) to presentation controller and this leads to buggy modal behaviour as we rely on UIAdaptivePresentationControllerDelegate callbacks. Restoring the default value and then comparing against it in updateProps:oldProps: allows for setter to be called, however if there was some additional logic to execute when stackPresentation is set to "push" the setter would not be triggered. --- ios/RNSScreen.mm | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ios/RNSScreen.mm b/ios/RNSScreen.mm index cd8b60fcbd..560838bdb2 100644 --- a/ios/RNSScreen.mm +++ b/ios/RNSScreen.mm @@ -550,6 +550,17 @@ - (void)prepareForRecycle _dismissed = NO; _state.reset(); _touchHandler = nil; + + // We set this prop to default value here to workaround view-recycling. + // Let's assume the view has had _stackPresentation == set + // before below line was executed. Then, when instantiated again (with the same modal presentation) + // updateProps:oldProps: method would be called and setter for stack presentation would not be called. + // This is crucial as in that setter we register `self` as a delegate (UIAdaptivePresentationControllerDelegate) to + // presentation controller and this leads to buggy modal behaviour as we rely on + // UIAdaptivePresentationControllerDelegate callbacks. Restoring the default value and then comparing against it in + // updateProps:oldProps: allows for setter to be called, however if there was some additional logic to execute when + // stackPresentation is set to "push" the setter would not be triggered. + _stackPresentation = RNSScreenStackPresentationPush; } - (void)updateProps:(facebook::react::Props::Shared const &)props From 65f19b6e07e5399a97ddac9643fb71af4981ffa9 Mon Sep 17 00:00:00 2001 From: Kacper Kafara Date: Tue, 19 Jul 2022 14:33:39 +0200 Subject: [PATCH 3/4] fix: compare incoming stackPresentation prop value against actual state, not second-to-last props. --- ios/RNSScreen.mm | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ios/RNSScreen.mm b/ios/RNSScreen.mm index 560838bdb2..3f37ce25b7 100644 --- a/ios/RNSScreen.mm +++ b/ios/RNSScreen.mm @@ -614,9 +614,12 @@ - (void)updateProps:(facebook::react::Props::Shared const &)props } #endif - if (newScreenProps.stackPresentation != oldScreenProps.stackPresentation) { - [self - setStackPresentation:[RNSConvert RNSScreenStackPresentationFromCppEquivalent:newScreenProps.stackPresentation]]; + // Notice that we compare against _stackPresentation, not oldScreenProps.stackPresentation. + // See comment in prepareForRecycle method for explanation. + RNSScreenStackPresentation newStackPresentation = + [RNSConvert RNSScreenStackPresentationFromCppEquivalent:newScreenProps.stackPresentation]; + if (newStackPresentation != _stackPresentation) { + [self setStackPresentation:newStackPresentation]; } if (newScreenProps.stackAnimation != oldScreenProps.stackAnimation) { From c16acb4331690f25dcd84814f65743e44c140848 Mon Sep 17 00:00:00 2001 From: Kacper Kafara Date: Tue, 19 Jul 2022 15:10:54 +0200 Subject: [PATCH 4/4] fix: mistake in comment --- ios/RNSScreen.mm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ios/RNSScreen.mm b/ios/RNSScreen.mm index 3f37ce25b7..1673de6e3b 100644 --- a/ios/RNSScreen.mm +++ b/ios/RNSScreen.mm @@ -555,11 +555,11 @@ - (void)prepareForRecycle // Let's assume the view has had _stackPresentation == set // before below line was executed. Then, when instantiated again (with the same modal presentation) // updateProps:oldProps: method would be called and setter for stack presentation would not be called. - // This is crucial as in that setter we register `self` as a delegate (UIAdaptivePresentationControllerDelegate) to - // presentation controller and this leads to buggy modal behaviour as we rely on - // UIAdaptivePresentationControllerDelegate callbacks. Restoring the default value and then comparing against it in - // updateProps:oldProps: allows for setter to be called, however if there was some additional logic to execute when - // stackPresentation is set to "push" the setter would not be triggered. + // This is crucial as in that setter we register `self.controller` as a delegate + // (UIAdaptivePresentationControllerDelegate) to presentation controller and this leads to buggy modal behaviour as we + // rely on UIAdaptivePresentationControllerDelegate callbacks. Restoring the default value and then comparing against + // it in updateProps:oldProps: allows for setter to be called, however if there was some additional logic to execute + // when stackPresentation is set to "push" the setter would not be triggered. _stackPresentation = RNSScreenStackPresentationPush; }