-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix modal and alert dismissal bugs #22666
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@shergin, can you please review/merge this pull request? BTW I don't think the failure of "test_objc" is related to my changes in this PR. |
After more testing I discovered that [3] is not fully fixed as I ran into the same bug again today. I'll try to make a repro later. While debugging this I found that after the call to |
Any progress on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff contains three things (and this complicates review process):
- Alert-related fixes. Should we decouple this component from the core? See the comment in the code.
- Modal-related fixes.
- Code-style fixes. Feel free to publish them separately.
Could you please to split this up?
@@ -30,6 +30,7 @@ @interface RCTAlertManager() | |||
@implementation RCTAlertManager | |||
{ | |||
NSHashTable *_alertControllers; | |||
UIWindow *_window; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retaining anything in ViewManager is very bad idea in general (and especially for UIKit things).
As @shergin pointed out this PR should be split into three separate PRs. Would you mind opening those? I'm going to close this one because it's too big to be merged as one. Ideally every PR only has one main objective (one bugfix, one new feature, etc.). |
Fixes #22237
Summary:
Fixed a bug with dismissing modals that caused the modal to stay on screen after being dismissed if some other view controller is presented on top of it.
Fixed an issue where dismissing a modal (by changing its
visible
property) would also hide alerts presented on top of it (i.e. shown after the modal became visible). This is achieved by presenting each alert in a separateUIWindow
instance.Fixed a bug where showing two modals rapidly and hiding them would leave an empty (invisible)
RCTModalHostViewController
hanging on screen, which made it impossible to interact with the app as the view controller is display on top of everything.These changes affect iOS only (no JS/Android changes).
Test Plan:
Need some help with filling this section...
Changelog:
[iOS] [Fixed] - Fixed various issues with dismissing alerts and modals