Skip to content
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 disappearing alert bug on iOS #26839

Closed
wants to merge 3 commits into from

Conversation

sryze
Copy link
Contributor

@sryze sryze commented Oct 13, 2019

Summary

Present each alert (UIAlertController) in a separate window so that they are not affected by modals or other presented view controlelrs. This also allows one to display mulitple alerts on top of each other (it is sometimes useful).

I basically copied a bunch of code from RCTDevLoadingView to achieve this.

This patch fixes issue #22237.

My previous pull request related to this issue: #22666 (closed).

Changelog

[iOS] [Fixed] - Fixed a bug with alerts disappearing when a modal gets hidden

Test Plan

I have a test project that demonstrates the bug and allows one to verify this fix (bug no. 1 in the bug list). The project is made with React 0.61.2.

Present each alert (UIAlertController) in a separate window so that they are
not affected by modals or other presented view controlelrs. This also allows
one to display mulitple alerts on top of each other (it is sometimes useful).

I basically copied a bunch of code from RCTDevLoadingView to chieve this.

This patch fixes issue facebook#22237.
@sryze sryze requested a review from shergin as a code owner October 13, 2019 13:14
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 13, 2019
@zaguiini
Copy link

zaguiini commented Dec 20, 2019

This fixes it.

@@ -30,6 +30,7 @@ @interface RCTAlertManager()
@implementation RCTAlertManager
{
NSHashTable *_alertControllers;
UIWindow *_window;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not store UIWindow as an ivar (because it's not shared among instances of view controllers), right?
Instead, we should just remove a window instance from Screen instance on alert being dismissed. We can geet a particular window instance from presented view controller (or any mounted view) instance.

Copy link
Contributor Author

@sryze sryze May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shergin You are right, it doesn't need to be an ivar, it can be a local variable here. In that case it also must be strongly captured by the action handler block of UIAlertController, otherwise the window gets destroyed while we are showing the alert controller. It looks like UIAlertController (or UIViewController in general) doesn't keep a strong reference to its window.

UIViewController *presentingController = [UIViewController new];
self->_window.rootViewController = presentingController;
self->_window.hidden = NO;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hidden must be set to NO explicitly, otherwise the window will remain invisible

@GoMino
Copy link

GoMino commented May 12, 2020

Thanks @sryze !
I confirm this fix works, I have patched my version of react-native waiting for this to be release.

@bartzy
Copy link

bartzy commented Jun 13, 2020

@sryze Do you know when this fix is expected to land? Are there any workarounds from the JS-side in the meantime?

@sryze
Copy link
Contributor Author

sryze commented Jun 20, 2020

I have no idea, probably not soon.

As a workaround you can create a patch for the react-native package and apply it during post-install step using patch-package. This is what I did in my project.

@mpm
Copy link

mpm commented Jun 22, 2020

@bartzy: A work around I use is to delay displaying the Alert for half a second, to make sure closing the modal has been dealt with. Ex. setTimeout(Alert.alert(...), 500)

This is far from a legitimate approach- but this issue was blocking our app approval in iTunes and I can't easily patch react myself as I'm using Expo.

In my case the Modal was to be closed immediately before the Alert appears. My work around will not work if you require to close the Modal at some arbitrary time while the Alert is visible and continues to be shown.

@adam-ashored
Copy link

adam-ashored commented Oct 21, 2020

An alternative to using the setTimeout is to await a promisified alert before you do a state update.

I tried to do something like this:

const handleSave = () => {
  try {
    setSaving(true);
    await someApiRequest();
  } catch(err) {
    Alert.alert(err);
  } finally {
    setSaving(false);
  }

}

And instead transformed it into this:

const handleSave = () => {
  try {
    setSaving(true);
    await someApiRequest();
  } catch(err) {
    await new Promise(res => Alert.alert(err,null,null,{onDismiss:res}))
  } finally {
    setSaving(false);
  }

}

@RenderCoder
Copy link

This change not work for me, may need to try another solution

@sryze
Copy link
Contributor Author

sryze commented Jul 13, 2021

This bug still exists in RN 0.62.3

@github-actions
Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Mar 21, 2023
@github-actions
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Alert Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants