Modal 'onOpen' and 'onClose' not firing when they should

[AlvMF1]

Looks like onOpen and onClose are fired when open changes (and when mounted?).

Testcase

codepen.io
Check console.

[levithomason]

Thanks for the codepen! Actually, the console functions are called on every render because they are not wrapped in a function:

onOpen={console.warn('onOpen')} 
// vs
onOpen={() => console.warn('onOpen')} 

The first example is assigning the return value of console.warn() to the onOpen handler, which returns undefined, so it is the same as having no onOpen/onClose handlers at all.

The second example gives a function, that when called, logs the messages. Seems to be working correct with the fix. Here's a fork:

http://codepen.io/levithomason/pen/aBJvZR

[AlvMF1]

Got it. But doing that, I don't see console messages in your fork when opening or closing...

[levithomason]

There might be a design flaw here. It looks like successful auto controlled open/close events do not call the handlers, which they should. Whereas, unsuccessful auto open/close events do call the handlers.

Example, clicking the button successfully opens the Modal, so no onOpen is called. Clicking the "close" button successfully closes the Modal, so no onClose is called. However, I see onClose when I press escape or click on the dimmer which is trying to close the Modal but is unsuccessful.

I've reopened and renamed this issue to investigate this.

[Arthelon]

It seems to be an issue with delegation to the Portal component. Some of the handlers aren't being registered if the "trigger" Portal prop isn't passed to the Modal.

[luski]

As far as I understood the current code, modals can be closed only when the trigger prop is passed or the onClose prop is a function setting the "open" state to false. When the user clicks outside the modal or presses ESC the this.close() inner method of the Portal component is called and within it:
this.prop.onClose() is called and
this.trySetState({ open: false }) (Portal.js:334)
is called.
The problem is the "trySetState" doesn't do anything. It tries to generate a new state, but always generates just an empty object. I don't understand what this function is supposed to do and why don't use just
this.setState({ open: false })
which solves this part of the issue?

The onClose and onOpen callbacks are not called when the user clicks the trigger because they are called in this.open and this.close inner methods and these methods are not called in some cases.
I think it would be better to call this.open/this.close (or at least onOpen/onClose callbacks) only in the componentDidUpdate method when the "open" state is changed. It assures the open/close methods are called always when the "open" state is really changed and not when the user only tries to change this state.

But I don't know the code and concepts good enough so treat it more like questions than suggestions 😄

[micahbule]

I'm currently having a problem with this one since we're heavily relying on proper callback functionalities for firing certain events in our app.

According to my analysis of the internal code, everything is fine when events are propagated from the trigger prop, as well as anything within the Modal's scope (Portal scope if we dive deeper) in a sense that the node who fired the event is either passed as a trigger prop or a child component of the underlying Portal component, as well as the dimmer.

The problem exists when the trigger node (in most cases a button) lies outside the scope, meaning the Modal nor the Portal component doesn't have an idea which node caused the change, it just know that the value of the open prop changed.

A quick clarification to @levithomason: Since you're mounting/unmounting the modal node when we open/close the modal (respectively), what's the ideal sequence of callbacks firing?

[micahbule]

I've found a much graceful "hack" for now until a better solution comes up.

Instead of controlling the Modal component's open prop and setting it to true or false to show or hide the modal, you can basically get a reference of the Modal component and manually trigger the open/close handlers via handleOpen()/handleClose() methods, respectively.

The advantages of this hack:

1. By leaving the control of the open prop to the component (via AutoControl), you're sure that the underlying mechanisms (ex. underlying Portal component) would still work as is.
2. Using this approach, you can now bind the visibility handlers to triggers outside the scope of the Modal component. Refer to my fork of your Codepen here. You can even manually trigger the handlers on other events such as a props or state changes (ex. a Redux or Flux state tree change).

The only caveat of this hack is that whenever you trigger the handlers from events that does not provide React's SyntheticEvent, your onOpen()'s and onClose()'s first argument would either be undefined or anything that you pass as the first argument of your handleOpen() and handleClose calls.