Dialog state does not change on close

[t1bb4r]

I think the Dialog component should set its state to {openDialog: false} on 'dialogclose'.

Currently if a user presses the escape key to close the dialog it wont open again since the openDialog state is still set to true.
This can be reproduced using the demo page:

• clicking show modal opens the dialog
• press escape
• clicking show modal again does not open the dialog

Unexpected state can easily be inspected using the react developer tools.

Edit: I'm using Chrome Version 48.0.2564.82 (64-bit)

[t1bb4r]

I'm very new to React so forgive my ignorance, here is what I've done to solve it. Hope this helps some one.

The following did not work (The event might be unsupported by React?)

<Dialog open={this.state.openDialog} onCancel={this.handleCloseDialog}>

So I did the following:

componentDidMount() {
    ReactDOM.findDOMNode(this.refs.dialog).addEventListener("cancel", this.handleCloseDialog);
}
render() {
  ...
     <Dialog open={this.state.openDialog} ref='dialog'>
  ...
}

Now when a user presses escape, or exits the dialog without using the button, the state is correctly set and the dialog can be opened once more.

Edit: Code works as a drop-in to the examples.

[tleunen]

Thanks for reporting the issue!
I didn't know the escape key could cancel the dialog. But indeed, it's explained on the Chrome developer page.

Using the cancel event should solve this. In your case, try with cancel instead of onCancel.

I'm thinking of cancelling the escape key by default, unless the user provide a cancel event. What do you think @Permagate?

[Permagate]

Hi @tleunen, thanks for the link. there are some new things regarding the native dialog element that I have learned.

Using the cancel event should solve this. In your case, try with cancel instead of onCancel.

I couldn't find the doc that says onCancel would be implemented natively, cmiiw. So I'm thinking if we should provide one ourselves? So that we don't need to attach a listener everytime we create a dialog, like @t1bb4r did.

I'm thinking of cancelling the escape key by default, unless the user provide a cancel event.

Yeah, I agree.

[tleunen]

I initially thought it was an attribute cancel, but in fact it's a real event.

I'll open a ticket in React to know if they plan to add the onCancel event.

[tleunen]

So I have a fix (quite easy to do), but I also wanted to add some tests to make sure we don't introduce regressions later, but I have some hard time finding how to check that the dialog closes or not when the user pressed the escape key :/

If you have some time @Permagate, could you take a look to see if there's a way?

[rudolf]

I didn't test the code now, but going by what @t1bb4r has said, if cancel isn't a supported react event, then your code would never call the onCancel prop?

I would have expected to see
this.refs.dialog.addEventListener('cancel', this.props.onCancel);

instead of

this.refs.dialog.addEventListener('cancel', prevent);

But perhaps I'm missing something.

[tleunen]

Indeed, cancel is currently not a supported react event. I created a ticket in React (facebook/react#6049) about it.

But there's a difference between what you define as props in your own components, and what's defined on the React elements.

For the prevent function, from my testing, it's called when the dialog receives the cancel event. The reason why the component catch the event by default and prevent its default behavior is because, with React, we cannot really let anyone else manage the state of a component outside of the component. So if the dialog decides to closes itself, React has to know about it. And without listing to attributes changes on the element, it's not really possible (is it?). So I decided to not allow this behavior by default.
If the end user still wants it, he can override the onCancel handler and manage the open state by himself :-)

[rudolf]

I agree that components should have as little state as possible and like the fact that you made the default for onCancel to prevent closing.

But given the following code:

<Dialog onCancel={console.log('user cancelled the dialogue'}>

Would react actually handle the cancel event and call my onCancel prop? I think the above code would still call prevent and nothing would be shown on the console. Once facebook/react#6049 is fixed, I'd expect the passed in onCancel prop to be called, but not as it stands now.

[tleunen]

Nop, if you specify the onCancel prop, it will override the default value set in the Dialog component. So the prevent function won't be called.

[Permagate]

@tleunen Heya, I was preoccupied with other things last week, so I just take a look at the fix today. I agree with @skaapgif. I think specifying onCancel props still doesn't replace the prevent function behavior, does it?

I have to change the following:

this.refs.dialog.addEventListener('cancel', prevent);

to

this.refs.dialog.addEventListener('cancel', this.props.onCancel ? this.props.onCancel : prevent);

to prevent the prevent function from being executed. Is it working as intended on your end without changing the above line of code?

[tleunen]

You don't have to manually listen the event yourself, the component does it for you.

[Permagate]

Ah, I'm talking about the component itself: https://github.com/tleunen/react-mdl/blob/master/src/Dialog/Dialog.js#L19

[tleunen]

Oh damn you're right. It should be this.props.onCancel instead.

[rudolf]

Haha, glad to see I wasn't going crazy 👍