Fix crash and side effects on double rotation with modal

[cameronvoell]

Fixes wordpress-mobile/WordPress-Android#10227

Context

This PR borrows ideas from this older PR from @mkevins: #1288

Related PRs

Gutenberg: WordPress/gutenberg#20860
WordPress-Android: wordpress-mobile/WordPress-Android#10383

Description

The changes is this PR can be broken into

• Part 1: simple fix for Crash in block editor on double rotation when inserter is open WordPress-Android#10227 and
• Part 2: a fix for a side effect of Part 1 that was causing modals to not reopen after rotate.

Part 1: Identical to #1288 (see the Description section there). Basically since we are creating a new Activity on orientation change we need to make a call to ReactInstanceManager.onHostDestroy()which will perform any required cleanup by our ReactContext's ActivityEventListeners. This includes removing references from our Modal to the old Activity, which fixes wordpress-mobile/WordPress-Android#10227.

Part 2: of this PR is to fix some issues where modals would no longer respond after we performed the required cleanup mentioned above (see discussion here: wordpress-mobile/WordPress-Android#10383 (comment)). The simplest way I found to do this was to emit a message to our React Native Bridge that we are closing any open modals. Our React Native BottomSheet component that contains the react-native-modal will listen for these events on Android and perform a call to its onClose method which notifies the ReactNative state that the modal is ready to be opened again (see Gutenberg PR: WordPress/gutenberg#20860).

Alternatively, we could test an update to the ReactModalHostView class in the facebook react-native repo that adds a call to the mOnRequestCloseListener before Dialog cleanup occurs in its onHostDestroy method. I have not tested this yet, but I think proposing the change to Open Source library would be complicated because the way we utilize our React Native instance inside a "headless" fragment is non standard, so this use case may not apply to most people.

To test:

Tested in WordPress Android app, as well as in the Android/iOS example apps. See steps and screenshot below.

Testing steps (same apply for opening the inserter Modal, or clicking insert Image/Video to Open a modal)

1. Add an Image block and add an image from device or media library.
2. While the image block is selected, press the "gear" button to open up the Image Settings.
3. Rotate the Device.
4. Click the gear button again, the Modal should reopen as normal.
5. Rotate the Device again => the app should behave normally without crashing.

Screenshot:

PR submission checklist:

• I have considered adding unit tests where possible.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[cameronvoell]

👋 @mkevins, gave this solution a go and seems to work OK. Thing is, I am not sure what's the full set of assumptions ReactInstanceManager.onHostDestroy() assumes, and it seems that we need to dive into the implementation to understand what's going on.

@hypest I wanted to attempt to address your question from #1288 (comment), since I was able to dig into this issue this week and resolve some side effects (pending review) that we were seeing from @mkevins fix for wordpress-mobile/WordPress-Android#10227.

In particular, the question to answer would be: does the onHostDestroy() lead to destroying the React context that holds the app and the data? If yes, under which circumstances?

So ReactInstanceManager.onHostDestroy() definitely does not destroy ReactContext that holds app data. You can see that ReactInstanceManager was created with a clear distinction between its destroy method which specifically "Destroy[s] this react instance and the attached JS context" and it's onHostDestroy method which calls onHostDestroy methods of our ReactContext's ActivityEventListeners, and then sets our Activity reference to null (code link).

Those same ActivityEventListeners instances will have their onHostResume functions called from our WPAndroidGlueCode onResume function:

gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java

Line 461 in 7500e0e

mReactInstanceManager.onHostResume(activity,

In short, ReactInstanceManager seems to make assumptions that Activity references can be updated via its onHostDestroy, onHostResume lifecycle, while preserving its ReactContext. Since the activity reference passed into our ReactInstanceManager.onHostDestroy() is in fact destroyed, and the main responsibility of onHostDestroy is to clean up ActivityEventListeners which may rely on that destroyed Activity, I think this change should be relatively harmless, (and beneficial!).

[hypest]

👍 @cameronvoell , thanks for having a deep look.

[mkevins]

Thanks Cameron for summarizing the lifecycle methods behavior, and for finding a solution to the side effects exposed by the draft fix. Very nice work on part 2 👍 .

The simplest way I found to do this was to emit a message to our React Native Bridge that we are closing any open modals.

This makes sense to me, and works well. I've tested via the steps in the description, and everything is now working as expected.

I think proposing the change to Open Source library would be complicated because the way we utilize our React Native instance inside a "headless" fragment is non standard, so this use case may not apply to most people.

I agree on this point too. Since our app architecture requires us to "color outside the lines" a bit, I'm unsure how readily our proposed changes would be accepted upstream.

[mkevins]

Tested via steps in description, on Android 10 - Pixel 3a, and everything is working as expected. LGTM!