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

Modal's onDismiss callback is wrongly implemented in 0.61.0-rc.3 #26473

Closed
wddwycc opened this issue Sep 17, 2019 · 9 comments
Closed

Modal's onDismiss callback is wrongly implemented in 0.61.0-rc.3 #26473

wddwycc opened this issue Sep 17, 2019 · 9 comments
Labels
Bug Component: Modal Resolution: Locked This issue was locked by the bot.

Comments

@wddwycc
Copy link
Contributor

wddwycc commented Sep 17, 2019

Issue:

With This commit, onDismiss on Modal will never be called when dismissed, code below as an example.

const App = () => {
  const [visible, setVisible] = useState(true)
  const onClosePress = useCallback(() => setVisible(false), [])
  const onDismiss = useCallback(() => { console.log('dismiss') }, [])
  return (
    <View>
      <TouchableOpacity onPress={onClosePress}>
        <Text>Close</Text>
      </TouchableOpacity>
      <Modal visible={visible} onDismiss={onDismiss} />
    </View>
  )
}

This is not the intended behavior of this callback according to the document


React Native version:

System:
OS: macOS 10.14.4
CPU: (8) x64 Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
Memory: 1.32 GB / 16.00 GB
Shell: 5.3 - /bin/zsh
Binaries:
Node: 10.9.0 - /usr/local/bin/node
npm: 6.2.0 - /usr/local/bin/npm
Watchman: 4.9.0 - /usr/local/bin/watchman
SDKs:
iOS SDK:
Platforms: iOS 13.0, DriverKit 19.0, macOS 10.15, tvOS 13.0, watchOS 6.0
IDEs:
Xcode: 11.0/11A419c - /usr/bin/xcodebuild
npmPackages:
react: 16.9.0 => 16.9.0
react-native: 0.61.0-rc.3 => 0.61.0-rc.3

@xiao99xiao
Copy link

xiao99xiao commented Sep 17, 2019

The commit bd2b7d6 is a disaster. It actually introduced the problem which it claims to solve in its summary. Would be great to see it reverted asap.

# Problem:
`onDismiss` prop isn't being called once the modal is dismissed, this diff fixes it.

@wddwycc wddwycc changed the title Modal's onDismiss callback is wrongly implemented in rc3 Modal's onDismiss callback is wrongly implemented in 0.61.0-rc.3 Sep 17, 2019
@bartolkaruza
Copy link

Fix for this issue is on the way, I'll try to get it cherry-picked in the next RC for 0.61 as well. #26490
Thanks for reporting this!

@wddwycc
Copy link
Contributor Author

wddwycc commented Sep 19, 2019

This issue has been resolved with a5353c0

@wddwycc wddwycc closed this as completed Sep 19, 2019
@annjawn
Copy link

annjawn commented Mar 16, 2020

This still seems to be broken as of 0.61.5

@lidonghua
Copy link

The same issue confirmed in 0.62 stable release. It seems the Fix is not included in the new release.

alloy pushed a commit to alloy/react-native that referenced this issue Apr 3, 2020
…r [an issue](facebook#26473) with `Modal`’s `onDismiss` prop.

This reverts commit bd2b7d6.
@exotexot
Copy link

exotexot commented Apr 8, 2020

I was sort of hoping, that the onDismiss callback would also fire, if I swipe it away on iOS. It doenst do that for me on 0.62.1

@xiao99xiao
Copy link

@farbexot You should open a new ticket.

@fedex995
Copy link

@farbexot did you open the new ticket? I'm expecting the same fix

@gadaubac339
Copy link

Open Modal.js in folder node_modules/react-native/Libraries/Modal, you can see:
`componentDidMount() {
if (ModalEventEmitter) {
this._eventSubscription = ModalEventEmitter.addListener(
'modalDismissed',
event => {
if (event.modalID === this._identifier && this.props.onDismiss) {
this.props.onDismiss();
}
},
);
}
}

componentWillUnmount() {
if (this._eventSubscription) {
this._eventSubscription.remove();
}
}
`
The root cause is iOS call "dismiss event" AFTER componentWillUnmount is called.
And there is no subscription exists for "onDismiss".
Try modify "componentWillUnmount":

componentWillUnmount() {
if (Platform.OS === 'ios') {
//do nothing
}
else {
if (this._eventSubscription) {
this._eventSubscription.remove();
}
}
}

And remove subscription:
componentDidMount() {
if (ModalEventEmitter) {
this._eventSubscription = ModalEventEmitter.addListener(
'modalDismissed',
(event) => {
if (event.modalID === this._identifier && this.props.onDismiss) {
if (Platform.OS === 'ios') {
if (this._eventSubscription) {
this._eventSubscription.remove();
}
}
this.props.onDismiss();
}
},
);
}
}
Good luck.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Component: Modal Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants