Send Modal onDismiss event on iOS (Fabric) and Android #42014

bernhardoj · 2023-12-20T15:57:49Z

Summary:

Modal onDismiss is not working on iOS (Fabric).
Modal onDismiss is currently only available on iOS. On Android, we don't have a way to know when exactly a modal is dismissed.

Currently, the onDismiss is emitted using a device event as a workaround to the RCTModalHostView unable to receive the component event as it's already unmounted when visible is false.

This PR removes the workaround and keeps RCTModalHostView mounted until the onDismiss event is emitted from the host and sends the onDismiss event on Android.

Changelog:

[ANDROID] [ADDED] - Added support for Modal onDismiss prop
[IOS] [FIXED] - Fix onDismiss is not working on Fabric

Test Plan:

Run rn-tester
Open the Modal example
The second example shows the counter for the show and dismiss count
Show and dismiss the modal and verify the count is incremented correctly

Screen.Recording.2023-12-20.at.23.55.29.mov

bernhardoj · 2023-12-20T16:12:35Z

This PR is identical to #38336. I closed #38336 and was planning to split it into smaller parts, but I found it's not possible to split.

I also hope the new PR will attract more attention since the old one has been stale for a few months.

(btw, the old PR fork is deleted)
(sorry in advance if this is something that I shouldn't do (closing and reopening a new PR))

analysis-bot · 2023-12-20T16:29:21Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	16,565,549	-16,499
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	19,942,644	-12,389
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: e2fb88e
Branch: main

cipolleschi · 2023-12-27T16:38:49Z

packages/rn-tester/js/examples/Modal/ModalPresentation.js

-            label="onDismiss ⚫️"
-            disabled={Platform.OS !== 'ios'}
+            label="onDismiss"
+            multiSelect={true}


why multiselect must be set to true?

It's to enable RNTOption

react-native/packages/rn-tester/js/components/RNTOption.js

Lines 40 to 44 in b507e11

disabled={

props.disabled === false || props.multiSelect === true

? false

: props.selected

}

I see that onShow passes multiSelect={true} instead of disable={false}, so I follow it.

react-native/packages/rn-tester/js/examples/Modal/ModalPresentation.js

Lines 186 to 190 in b507e11

<RNTOption

key="onShow"

style={styles.option}

label="onShow"

multiSelect={true}

Do you think I should change it to disable={false}?

yeah, probably it is more clear and accurate. I think that multiSelect is the API that allows you to select multiple choices (e.g.: "Which ice cream flavour do you prefer?" you can reply with "Chocolate", "cream" and "vanilla" and leave "strawberry" out) but in our case is is true/false and it does not make sense to select both.
WDYT?

I think that multiSelect is the API that allows you to select multiple choices

This is what I thought when saw it the first time and I totally agree using disabled makes more sense. Updated the code.

...-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostManager.java

cipolleschi

It generally looks good to me. I also love that you removed more code than you added and the overall logic looks simpler to me.
Great job! 👏

cipolleschi · 2023-12-27T16:43:15Z

/rebase - this comment automatically rebase the PR on top of main.

facebook-github-bot · 2023-12-28T17:02:56Z

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

cipolleschi · 2024-01-02T09:55:46Z

Quick update on this PR. The changes implemented here are making several internal jobs fail with our CI.

This is a change we do want, but it will require a little bit more work on our side to make it happen. How urgent is this for you? Is it blocking your workflow?

bernhardoj · 2024-01-02T10:17:10Z

@cipolleschi thanks for the update. It's not an urgent one nor blocking any workflow.

sammy-SC · 2024-01-15T10:51:21Z

packages/react-native/Libraries/Modal/Modal.js

-    if (this._eventSubscription) {
-      this._eventSubscription.remove();
+  componentDidUpdate(prevProps: Props) {
+    if (prevProps.visible !== true && this.props.visible === true) {


What is the purpose of `isRendering?

isRendering is mapped to value of visible prop.

isRendering is being used to keep the Modal rendered until the onDismiss callback is called.

What if the parent component unmounts Modal?

In that case, onDismiss won't be called.

facebook-github-bot · 2024-01-16T17:40:11Z

@cipolleschi merged this pull request in 314cfa8.

facebook-github-bot · 2024-01-20T14:27:15Z

This pull request has been reverted by a58ec07.

bernhardoj · 2024-01-22T10:45:01Z

@cipolleschi hi, I see that the PR has been reverted. May I know why it was reverted and is there anything I can do to help? Thanks!

cipolleschi · 2024-01-22T12:00:46Z

Hi there, I was about to write.
We had to revert it because we had some callsites internally that started failing over the weekend, causing an important drop in some widely used surfaces.
Sadly, the problem is that we had some Android code which was relying on the fact that the event was not fired on that platform. 🤦

One of the problems is that the change was quite big (changing both platforms, changing public APIs, ...). This kind of changes are always a little risky as the more code we change the higher the probability of something to go wrong.

What we can do to make it easier for these changes to stick would be to split this PR in smaller chunks and to try and not change the public APIs.
I'll try to figure out how to split it properly

bernhardoj · 2024-02-07T12:49:21Z

@cipolleschi hi, hope you are doing well. I would like to know if Is there any update on the split plan.

From me, I'm thinking that perhaps we can add a new props called useDismissEventFromNativeComponent that will use either the current ModalEventEmitter or the new approach from this PR.

The native component itself will fire onDismiss and ModalEventEmitter.modalDismissed (for iOS) events, so the conditional logic is only applied to the JS component (Modal.js).

For the split part,

Start adding the Android dismiss native code. We can't test the dismiss event, but we know that it works in this PR, so I think it's fine.
Next, we add the iOS dismiss native code. Same as above
Last, we apply the conditional logic in the JS file, for example

componentDidMount() {
  if (ModalEventEmitter) {
    this._eventSubscription = ModalEventEmitter.addListener(
      'modalDismissed',
      event => {
        if (!this.props.useDismissEventFromNativeComponent && event.modalID === this._identifier && this.props.onDismiss) {
          this.props.onDismiss();
        }
      },
    );
  }
}

What do you think?

bernhardoj · 2024-02-16T10:14:56Z

@cipolleschi hi, do you have any updates?

cipolleschi · 2024-02-19T15:31:44Z

Hi @bernhardoj, sorry for the late reply, but I was quite busy with other high priority stuff and I had little to no time to look into this.

For iOS, I think everything is done now:

In the Old Architecture, onDismiss relies on the old EventEmitter and it works.
In the New Architecture, onDismiss uses the proper approach with the events and it works.

The problem, as you might expect, comes with Android.
Unfortunately, I don't think we can make it work properly with that platform.
Apparently, it turns out that, internally, we have some Android code that relies on the event not being fired. Start firing it will cause major issues with the internal codebase:

it will take a lot of work and time to merge it
it will be reverted multiple times, basically, every time an internal app will fail due to this change.

There are 2 stepswe can pursue:

We create a PR in the discussion-and-proposal repo and we get the buy in from various people, including internal people, to make this happens in Android.
While we wait for the above, you can create a component based on <Modal> with the fix. Something like <EXPModal> and include that component as a dependency of Expensify. Even better if, rather than copying and pasting the code, you manage to have a component that extends the native classes of Modal so that when we push some improvements, your component benefits from them too.

This will unblock both you and the app while the discussion proceeds.

How does that sound like?

bernhardoj · 2024-02-20T15:34:37Z

In the New Architecture, onDismiss uses the proper approach with the events and it works.

Last time I tested, the onDismiss doesn't work because the JS component is unmounted before receiving the event.

In the Old Architecture, onDismiss relies on the old EventEmitter and it works.

But I think we should remove the old EventEmitter otherwise there will be double onDismiss call right?

While we wait for the above, you can create a component based on with the fix. ...

We actually use react-native-modal, so I think it's not possible to apply the suggestion (react-native-modal has onModalHide, but it's called too early). Do you think applying a patch while waiting for the proposal discussion is better?

@cipolleschi

cipolleschi · 2024-02-20T16:34:54Z

Last time I tested, the onDismiss doesn't work because the JS component is unmounted before receiving the event.

What I meant is that, after your diff landed, and got reverted, I split the iOS part and manage to get it landed. So, the iOS side is done and it works as you (almost) did with your diff.

But I think we should remove the old EventEmitter otherwise there will be double onDismiss call right?

No, because The New Architecture does not use the old event emitter and the Old Architecture don't use the event which is Fabric only.
So there would be no double events.

We actually use react-native-modal, so I think it's not possible to apply the suggestion (react-native-modal has onModalHide, but it's called too early). Do you think applying a patch while waiting for the proposal discussion is better?

Yeah, that can work too! Patch-packages is a better temporary solution than forking react-native.

bernhardoj · 2024-02-21T08:48:53Z

Ah, I see, thanks for clarifying it. I will discuss this with the team first. Thanks for your help!

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 20, 2023

cipolleschi reviewed Dec 27, 2023

View reviewed changes

...-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostManager.java Show resolved Hide resolved

cipolleschi approved these changes Dec 27, 2023

View reviewed changes

bernhardoj added 3 commits December 27, 2023 16:44

remove modal dismiss device event with identifier workaround

f5b0e54

send Modal onDismiss event on Android

5b1ea10

remove unmount code

8334aae

github-actions bot force-pushed the fix/modal-ondismiss-event branch from fe01468 to 8334aae Compare December 27, 2023 16:44

use disabled props to enable the option

2fe60a1

sammy-SC reviewed Jan 15, 2024

View reviewed changes

facebook-github-bot closed this in 314cfa8 Jan 16, 2024

facebook-github-bot added Merged This PR has been merged. Reverted labels Jan 16, 2024

adelekennedy mentioned this pull request Jan 31, 2024

[HOLD for payment 2024-08-07] [HOLD] [$1000] ReportActionContextMenu not closing properly Expensify/App#23959

Closed

6 tasks

bernhardoj mentioned this pull request Mar 4, 2024

Modal onDismiss support for Android react-native-community/discussions-and-proposals#769

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send Modal onDismiss event on iOS (Fabric) and Android #42014

Send Modal onDismiss event on iOS (Fabric) and Android #42014

bernhardoj commented Dec 20, 2023

bernhardoj commented Dec 20, 2023

analysis-bot commented Dec 20, 2023 •

edited

Loading

cipolleschi Dec 27, 2023

bernhardoj Dec 28, 2023

cipolleschi Dec 28, 2023

bernhardoj Dec 28, 2023 •

edited

Loading

cipolleschi left a comment

cipolleschi commented Dec 27, 2023

facebook-github-bot commented Dec 28, 2023

cipolleschi commented Jan 2, 2024 •

edited

Loading

bernhardoj commented Jan 2, 2024

sammy-SC Jan 15, 2024

bernhardoj Jan 15, 2024

sammy-SC Jan 15, 2024

bernhardoj Jan 16, 2024

facebook-github-bot commented Jan 16, 2024

facebook-github-bot commented Jan 20, 2024

bernhardoj commented Jan 22, 2024

cipolleschi commented Jan 22, 2024

bernhardoj commented Feb 7, 2024

bernhardoj commented Feb 16, 2024

cipolleschi commented Feb 19, 2024

bernhardoj commented Feb 20, 2024

cipolleschi commented Feb 20, 2024

bernhardoj commented Feb 21, 2024

	disabled={
	props.disabled === false \|\| props.multiSelect === true
	? false
	: props.selected
	}

	<RNTOption
	key="onShow"
	style={styles.option}
	label="onShow"
	multiSelect={true}

Send Modal onDismiss event on iOS (Fabric) and Android #42014

Send Modal onDismiss event on iOS (Fabric) and Android #42014

Conversation

bernhardoj commented Dec 20, 2023

Summary:

Changelog:

Test Plan:

bernhardoj commented Dec 20, 2023

analysis-bot commented Dec 20, 2023 • edited Loading

cipolleschi Dec 27, 2023

Choose a reason for hiding this comment

bernhardoj Dec 28, 2023

Choose a reason for hiding this comment

cipolleschi Dec 28, 2023

Choose a reason for hiding this comment

bernhardoj Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi commented Dec 27, 2023

facebook-github-bot commented Dec 28, 2023

cipolleschi commented Jan 2, 2024 • edited Loading

bernhardoj commented Jan 2, 2024

sammy-SC Jan 15, 2024

Choose a reason for hiding this comment

bernhardoj Jan 15, 2024

Choose a reason for hiding this comment

sammy-SC Jan 15, 2024

Choose a reason for hiding this comment

bernhardoj Jan 16, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Jan 16, 2024

facebook-github-bot commented Jan 20, 2024

bernhardoj commented Jan 22, 2024

cipolleschi commented Jan 22, 2024

bernhardoj commented Feb 7, 2024

bernhardoj commented Feb 16, 2024

cipolleschi commented Feb 19, 2024

bernhardoj commented Feb 20, 2024

cipolleschi commented Feb 20, 2024

bernhardoj commented Feb 21, 2024

analysis-bot commented Dec 20, 2023 •

edited

Loading

bernhardoj Dec 28, 2023 •

edited

Loading

cipolleschi commented Jan 2, 2024 •

edited

Loading