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

onCancel not being called for modals #5203

Closed
elios264 opened this issue Mar 7, 2017 · 5 comments
Closed

onCancel not being called for modals #5203

elios264 opened this issue Mar 7, 2017 · 5 comments
Labels
🐛 Bug Ant Design Team had proved that this is a bug. help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request.

Comments

@elios264
Copy link

elios264 commented Mar 7, 2017

Environment(required)

  • antd version: 2.7.4
  • OS and its version: Windows 10
  • Browser and its version: chrome 56.0.2924.87

What did you do? Please provide steps to re-produce your problem.

    Modal.confirm({
      title: 'Sample title',
      content: `Sample content`,
      onCancel: () => console.log('cancelling') });

What do you expected?

a console log when pressing ESC key

What happens?

The modal closes but the log is not printed

Re-producible online demo

http://codepen.io/elios264/pen/RpoVeE

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Mar 8, 2017

Hi elios.I think it's not a bug,because the doc say that onCancel don't care about esc,it just handle cancel button and x button.And I think maybe it's brower's behavior(i.e.,remove all nodes which are masks ),you can use alert() or confirm() on the browser console and then press "Esc" key,you will see that they will disappear.

Maybe these help?

Five unexpected uses for the Esc key

bootstrap modal handle esc

bootstrap modal doc

relevant issue1

relevant issue2

relevant issue3

Chrome keyboard shortcuts

@elios264
Copy link
Author

elios264 commented Mar 8, 2017

Hi @NE-SmallTown,

It is not browser behaviour, I checked the underline source code but I couldn't figure out because the library uses rc-dialog underneath, but the rc-dialog does calls onCancel when pressing ESC,

I read "cancel button and x button" too, I just think they missed that case.

@benjycui benjycui added the 🐛 Bug Ant Design Team had proved that this is a bug. label Mar 8, 2017
@benjycui
Copy link
Contributor

benjycui commented Mar 8, 2017

We need to trigger props.onCancel and pass event as argument here

Could you try to PR to fix this? @elios264 @NE-SmallTown

@benjycui benjycui added the help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request. label Mar 8, 2017
@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Mar 8, 2017

@elios264 Sorry for that I don't check rc-dialog src code,it does handle the "ESC" key.

 if (props.keyboard && e.keyCode === KeyCode.ESC) {
      this.close(e);
 }

It does is a bug like @benjycui says,when you press "ESC" key,the execute order is:

react-component-dialog: onKeyDown ->> 
react-component-dialog: close ->>
react-component-dialog.props.onClose(e) ->>
ant-design-Modal: handleCancel ->>
ant-design-Modal.props.onCancel(e) ->>
ant-design-confirm: onCancel ->> 
function close() {
    const unmountResult = ReactDOM.unmountComponentAtNode(div);
    if (unmountResult && div.parentNode) {
      div.parentNode.removeChild(div);
    }
}

So,the final handle function is close function above,but it doesn't call props.onCancel(e) which you define at your component like @benjycui says.

@lock
Copy link

lock bot commented May 1, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐛 Bug Ant Design Team had proved that this is a bug. help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request.
Projects
None yet
Development

No branches or pull requests

4 participants