Make Modal's `onDismiss` work on Fabric. #42601

cipolleschi · 2024-01-22T16:29:43Z

Summary:
After S390064, the OnDismiss event for Modal from D52445670 was reverted.
The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything.

The most important thing for our short term goal is to make the OnDismiss work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless).

We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs.

The Problem:

The reason why the onDismiss event does not work is because, as soon as the visible property is turned to false, the component is removed by the React tree.
When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired.

The Solution:

We made this work by "delaying" when the component need to be removed from the reacat Tree.

Rather then rendering or node or not based on the visible props, we are introducing a State object that keeps track when the Modal is rendered or not.

The state.isRendering property is set to true when the visible prop is set to true.
For iOS, when visible prop is set to false, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the state.isRendering property to false and the Modal can be considered dismissed.

Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented.

Changelog:

[iOS][Fixed] - onDismiss now work on iOS with Fabric, in both Bridge and Bridgeless mode.

Differential Revision: D52959996

facebook-github-bot · 2024-01-22T16:30:07Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Differential Revision: D52959996

facebook-github-bot · 2024-01-22T16:52:02Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Differential Revision: D52959996

facebook-github-bot · 2024-01-22T17:19:33Z

This pull request was exported from Phabricator. Differential Revision: D52959996

analysis-bot · 2024-01-22T17:48:04Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	16,951,905	+88
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	20,335,624	+35
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: b8778ab
Branch: main

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Differential Revision: D52959996

facebook-github-bot · 2024-01-23T10:57:18Z

This pull request was exported from Phabricator. Differential Revision: D52959996

okwasniewski · 2024-01-23T12:55:42Z

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

+      if (Platform.OS === 'ios') {
+        this.setState({isRendering: false}, () => {
+          if (this.props.onDismiss) {
+            this.props.onDismiss();
+          }
+        });


Question: Wouldn't it be better from an OOT platform standpoint to use early return? If we would check Platform.OS === 'android' and return. Then no changes would be needed for macOS to support it.

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Differential Revision: D52959996

facebook-github-bot · 2024-01-23T13:43:54Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Differential Revision: D52959996

facebook-github-bot · 2024-01-23T13:45:14Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Differential Revision: D52959996

facebook-github-bot · 2024-01-23T14:59:37Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Differential Revision: D52959996

facebook-github-bot · 2024-01-23T15:26:34Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Differential Revision: D52959996

facebook-github-bot · 2024-01-23T16:03:12Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Differential Revision: D52959996

facebook-github-bot · 2024-01-24T10:05:53Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Reviewed By: sammy-SC Differential Revision: D52959996

facebook-github-bot · 2024-01-24T13:42:54Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Reviewed By: sammy-SC Differential Revision: D52959996

facebook-github-bot · 2024-01-25T10:05:18Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Reviewed By: sammy-SC Differential Revision: D52959996

facebook-github-bot · 2024-01-25T11:50:16Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Reviewed By: sammy-SC Differential Revision: D52959996

facebook-github-bot · 2024-01-31T18:24:05Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Reviewed By: sammy-SC Differential Revision: D52959996

facebook-github-bot · 2024-02-07T14:32:19Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Reviewed By: sammy-SC Differential Revision: D52959996

facebook-github-bot · 2024-02-07T14:33:45Z

This pull request was exported from Phabricator. Differential Revision: D52959996

Summary: After S390064, the OnDismiss event for Modal from D52445670 was reverted. The diff was too big and caused the SEV, so we are trying to reimplement it gradually to make sure we don't brake anything. The most important thing for our short term goal is to make the `OnDismiss` work only for iOS (following the official docs, Android never supported it) for Fabric (Bridge and Bridgeless). We also want to minimize the changes t the JS infrastructure, so we are trying not to alter the JS APIs. ## The Problem: The reason why the onDismiss event does not work is because, as soon as the `visible` property is turned to `false`, the component is removed by the React tree. When this happens, Fabric deallocate the ShadowNode and the EventEmitter. Therefore, the event is not fired. ## The Solution: We made this work by "delaying" when the component need to be removed from the reacat Tree. Rather then rendering or node or not based on the `visible` props, we are introducing a `State` object that keeps track when the Modal is rendered or not. The `state.isRendering` property is set to `true` when the `visible` prop is set to `true`. For iOS, when `visible` prop is set to `false`, instead, we wait for the Native side to actually dismiss the View and to invoke the event. When the event is fired, we manually set the `state.isRendering` property to false and the Modal can be considered dismissed. Notice that this makes also useless to have the Modal Native's snapshot to simulate that the modal is still presented. ## Changelog: [iOS][Fixed] - `onDismiss` now work on iOS with Fabric, in both Bridge and Bridgeless mode. Reviewed By: sammy-SC Differential Revision: D52959996

facebook-github-bot · 2024-02-07T14:34:48Z

This pull request was exported from Phabricator. Differential Revision: D52959996

facebook-github-bot · 2024-02-08T10:26:42Z

This pull request has been merged in c7a0dff.

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: Facebook Partner: Facebook Partner fb-exported labels Jan 22, 2024

cipolleschi force-pushed the export-D52959996 branch from 2425376 to bc7a7af Compare January 22, 2024 16:51

cipolleschi force-pushed the export-D52959996 branch from bc7a7af to e86038c Compare January 22, 2024 17:19

cipolleschi force-pushed the export-D52959996 branch from e86038c to 5dee28b Compare January 23, 2024 10:57

okwasniewski reviewed Jan 23, 2024

View reviewed changes

cipolleschi force-pushed the export-D52959996 branch from 5dee28b to c7f3b0c Compare January 23, 2024 13:43

cipolleschi force-pushed the export-D52959996 branch from c7f3b0c to aae1891 Compare January 23, 2024 13:45

cipolleschi force-pushed the export-D52959996 branch from aae1891 to 9e849ca Compare January 23, 2024 14:59

cipolleschi force-pushed the export-D52959996 branch from 9e849ca to 1a45258 Compare January 23, 2024 15:26

cipolleschi force-pushed the export-D52959996 branch from 1a45258 to 14e8da9 Compare January 23, 2024 15:26

cipolleschi force-pushed the export-D52959996 branch from befed28 to cbaeffb Compare January 24, 2024 10:05

cipolleschi force-pushed the export-D52959996 branch from cbaeffb to 7b20f9d Compare January 24, 2024 13:42

cipolleschi force-pushed the export-D52959996 branch from 7b20f9d to cc01266 Compare January 25, 2024 10:05

cipolleschi force-pushed the export-D52959996 branch from cc01266 to bdb85f3 Compare January 25, 2024 11:50

cipolleschi force-pushed the export-D52959996 branch from bdb85f3 to 4ec1a95 Compare January 31, 2024 18:23

cipolleschi force-pushed the export-D52959996 branch from 4ec1a95 to ba96111 Compare February 7, 2024 14:32

cipolleschi force-pushed the export-D52959996 branch from ba96111 to 44a45f9 Compare February 7, 2024 14:33

cipolleschi force-pushed the export-D52959996 branch from 44a45f9 to 7369719 Compare February 7, 2024 14:34

facebook-github-bot closed this in c7a0dff Feb 8, 2024

facebook-github-bot added the Merged This PR has been merged. label Feb 8, 2024

TatianaKapos mentioned this pull request Nov 19, 2024

[Fabric] Implement onDismiss for Modal and remove titlebar microsoft/react-native-windows#14126

Merged

Make Modal's onDismiss work on Fabric. #42601

Make Modal's onDismiss work on Fabric. #42601

Uh oh!

Conversation

cipolleschi commented Jan 22, 2024

The Problem:

The Solution:

Changelog:

Uh oh!

facebook-github-bot commented Jan 22, 2024

Uh oh!

facebook-github-bot commented Jan 22, 2024

Uh oh!

facebook-github-bot commented Jan 22, 2024

Uh oh!

analysis-bot commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jan 23, 2024

Uh oh!

okwasniewski Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 23, 2024

Uh oh!

facebook-github-bot commented Jan 23, 2024

Uh oh!

facebook-github-bot commented Jan 23, 2024

Uh oh!

facebook-github-bot commented Jan 23, 2024

Uh oh!

facebook-github-bot commented Jan 23, 2024

Uh oh!

facebook-github-bot commented Jan 24, 2024

Uh oh!

facebook-github-bot commented Jan 24, 2024

Uh oh!

facebook-github-bot commented Jan 25, 2024

Uh oh!

facebook-github-bot commented Jan 25, 2024

Uh oh!

facebook-github-bot commented Jan 31, 2024

Uh oh!

facebook-github-bot commented Feb 7, 2024

Uh oh!

facebook-github-bot commented Feb 7, 2024

Uh oh!

facebook-github-bot commented Feb 7, 2024

Uh oh!

facebook-github-bot commented Feb 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Make Modal's `onDismiss` work on Fabric. #42601

Make Modal's `onDismiss` work on Fabric. #42601

analysis-bot commented Jan 22, 2024 •

edited

Loading