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 Alert not showing in an app using UIScene #34562

Closed
wants to merge 1 commit into from

Conversation

tido64
Copy link
Collaborator

@tido64 tido64 commented Sep 1, 2022

Summary

In an app using UIScene, Alert no longer pops up because the window
that gets created are not attached to a scene.

Changelog

[iOS] [Fixed] - Fix Alert not showing in an app using UIScene

Test Plan

Use the test plan in #29295. This should not regress that fix.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 1, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Sep 1, 2022
@analysis-bot
Copy link

analysis-bot commented Sep 1, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 7aa203b
Branch: main

@lunaleaps
Copy link
Contributor

Sorry why does removing the hide API fix this?

@tido64
Copy link
Collaborator Author

tido64 commented Sep 2, 2022

Sorry why does removing the hide API fix this?

Sorry I wasn't clear in the description. The issue is the fact that we create a new window to present an alert from. From what I've gathered, this works only if we're using the old way of writing iOS apps because windows automatically get attached. If we're using UIScenes, they no longer do. We would have to find the appropriate scene to attach the window to. In the current state, the window is not visible, hence the alert won't show. The fix is to get the key window and present the alert from its root view. The implementation prior to the fix I mentioned in the description did the extra step of looking for the currently presenting root view, which shouldn't be necessary. As far as I can tell, the root view is sufficient. Since we no longer need the extra window, we no longer need to hide it. Hence why hide was also removed.

Summary

In an app using `UIScene`, `Alert` no longer pops up because the window
that gets created are not attached to a scene.

Changelog

[iOS] [Fixed] - Fix `Alert` not showing in an app using `UIScene`

Test Plan

Use the test plan in #29295. This should not regress that fix.
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,637,892 +0
android hermes armeabi-v7a 7,049,888 +0
android hermes x86 7,939,859 +0
android hermes x86_64 7,911,889 +0
android jsc arm64-v8a 9,513,705 +0
android jsc armeabi-v7a 8,288,992 +0
android jsc x86 9,453,249 +0
android jsc x86_64 10,044,494 +0

Base commit: 7aa203b
Branch: main

@facebook-github-bot
Copy link
Contributor

@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @tido64 in 153aedc.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 8, 2022
@tido64 tido64 deleted the tido/fix-alert-uiscene branch September 9, 2022 06:23
facebook-github-bot pushed a commit that referenced this pull request Sep 17, 2022
Summary:
Changelog: [Internal] - Revert #34562

re: [iOS] [Fixed] - Fix Alert not showing in an app using UIScene

Reviewed By: alsun2001

Differential Revision: D39591113

fbshipit-source-id: ba707c11b3fb97eb3a6fee32e57b92403aa8b3d8
@lunaleaps
Copy link
Contributor

lunaleaps commented Sep 19, 2022

@tido64 We had to revert this as this was regressing behavior of an internal use-case where an Alert was shown inside a Modal. With these changes, the Alert doesn't appear.

From a Meta engineer, they debugged it and found that [RCTKeyWindow() rootViewController] is not the same as self.alertWindow.rootViewController for the Modal case

Repro example

import {useCallback, useState} from 'react';
import {Alert, Modal, StyleSheet} from 'react-native';

export default function Playground(props: Props): React.Node {
  const [modalVisible, setModalVisible] = useState(false);

  const onPressAlert = useCallback(() => {
    Alert.alert('Alert');
  }, []);

  const onToggleModal = useCallback(() => {
    setModalVisible(v => !v);
  }, []);

  return (
    <View style={styles.container}>
      <Button
        style={{marginVertical: 20}}
        onPress={onPressAlert}
        label="Alert now"
      />
      <Button onPress={onToggleModal} label="Modal" />

      {modalVisible && (
        <Modal>
          <Button
            onPress={onPressAlert}
            label="Alert now"
          />
          <Button onPress={onToggleModal} label="Back" />
        </Modal>
      )}
    </View>
  );
}

@tido64
Copy link
Collaborator Author

tido64 commented Sep 20, 2022

@lunaleaps Thanks for letting me know. I'll find a different fix for this.

OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
In an app using `UIScene`, `Alert` no longer pops up because the window
that gets created are not attached to a scene.

## Changelog

[iOS] [Fixed] - Fix `Alert` not showing in an app using `UIScene`

Pull Request resolved: facebook#34562

Test Plan: Use the test plan in facebook#29295. This should not regress that fix.

Reviewed By: cipolleschi

Differential Revision: D39276976

Pulled By: lunaleaps

fbshipit-source-id: e48e985ed4abec77d6f01a6c17292d664ed88f13
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Changelog: [Internal] - Revert facebook#34562

re: [iOS] [Fixed] - Fix Alert not showing in an app using UIScene

Reviewed By: alsun2001

Differential Revision: D39591113

fbshipit-source-id: ba707c11b3fb97eb3a6fee32e57b92403aa8b3d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants