fix: set VCs of views from recycling pool as UIAdaptivePresentationControllerDelegate #1535
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
method We set this prop to default value here to workaround view-recycling. Let's assume the view has had _stackPresentation == <some modal stack presentation> 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.
…te, not second-to-last props.
kkafar
changed the title
kkafar
changed the title
kkafar
changed the title
7 tasks
WoLewicki
approved these changes
Jul 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1299 on Fabric. I was not able to reproduce this issue on Paper -> changes introduced in this PR do not change behaviour on Paper.
View recycling caused behaviour such that VCs of views that returned from recycle pool were not set as delegates for presentation controller. This caused
presentationControllerDidDismiss:method not to be called and in consequence we did not receive information that modal transition had finished -> we did not allow for another one to start.Changes
Set
stackPresentationto default value inprepareForRecyclemethod to workaround view-recycling & check incoming new prop value (inupdateProps:oldProps:) against actual state not old props. I pasted just below (a little bit paraphased) comment I placed in code inprepareForRecyclemethod with better explanation how the bug worked.Let's assume the view has had
_stackPresentation == <some modal stack presentation>setbefore 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 registerself.controlleras a delegate (UIAdaptivePresentationControllerDelegate) topresentation controller and this leads to buggy modal behaviour as we rely on
UIAdaptivePresentationControllerDelegatecallbacks. Restoring the default value and then comparing against it inupdateProps:oldProps:allows for setter to be called, however if there was some additional logic to execute whenstackPresentationis set topushthe setter would not be triggered.Test code and steps to reproduce
See
Test1299inFabricTestExample& description of #1299 to see what & how to test.Checklist