BsModal is closed regarding of open property which causes strange edge cases

[jelhan]

Current interaction of <BsModal>'s open property and closing functionality is confusing in my opinion.

1. The modal could be closed by clicking backdrop/close button even if open property stays true. Seeing <BsModal @open={{true}}>...</BsModal> I would expect that these modal can't be closed at all. But actually it could by closed by a click on the backdrop. One must return false from onHide action to prevent a modal from being closeable or disable all possibilities to close that modal.

2. If a modal is closed but open property is still true the modal could not be reopened by setting it to true again. It must be changed to some other value before. This is likely to cause bugs that are very difficult to debug.
   E.g. imagine a modal being used to show an error message if some action fails. This is most likely been implemented by setting a property bound to <BsModal>'s @open to true in that action. If the developer isn't aware of the behavior described here, he will likely not set the property to false again when user closes the modal (using <BsModal>'s onHide or onHidden properties). In that case the modal (and with it the error message) won't be shown again if the error reoccurs...

It would be much easier to reason about <BsModal> and less error prune if a modal can't be closed / will always be shown if it's @open property is true. A typical usage would then be <BsModal @open={{showModal}} @onHide={{action (mut showModal) false}}>, which is much more inline with the other APIs of Ember Bootstrap and the rest of the ecosystem.

This would be a breaking change. So we might want to consider it for v3.

[simonihmig]

This stems from the idea that the component should support both an uncontrolled as well as a controlled mode of operation. With that I basically mean who is responsible for owning the open-state of the component, see e.g. https://stackoverflow.com/questions/42522515/what-are-controlled-components-and-uncontrolled-components/42522792#42522792.

I think in most cases (like 99%+) you want the uncontrolled version, where the modal shows up by default, and will be closed when user does the corresponding interaction (close button, backdrop click). So no need the prevent the closing. In that case I would like <BsModal> to just work(TM), without the boilerplate of the controlled version.

In the usually rare case that you want to control the state, the component should allow you to do that, which is the <BsModal @open={{showModal}} @onHide={{action (mut showModal) false}}> example.

I think there is some value in what you would expect to be the default behaviour in 99%+ of the cases to just work. Take a dropdown as another example. There could be use cases where you actually want to control the opening programmatically, but in most cases the open-state can be hidden inside the component and default behaviour to show the menu when the dropdown button is clicked does not need to be controlled from the outside.

Do you have ideas for a better API to unite these two modes?

[simonihmig]

@jelhan would like to do the v3 release sooner than later. Still think this 👆 should be sorted out before the release?

[jelhan]

@jelhan would like to do the v3 release sooner than later. Still think this should be sorted out before the release?

Lets not block v3 for this one as it seems to need more discussion. I'm quite sure there will be a v4 some time in the future. 😄

---

I like the distinction between controlled and uncontrolled mode of operation. Very helpful terms to reason about the issue. Actually I think we already support both modes but having a strange API for the controlled one:

1. The uncontrolled version is the default one, which just works using <BsModal>. I wouldn't touch that one.
2. A consumer could opt in the controlled version by passing open property. In that case the modal is only shown if open is true on initial render or changed to true later. The value is not considered if a user interaction might close the modal. In that case only return value of onHide action is considered. In that sense it's working like a trigger - but a broken one as it's not shown again if resetting true.

Let me give a quick example, what I would consider broken about current implementation of controlled mode:

<button onclick={{action (mut showModal) true}}>show modal</button>
<BsModal @open={{showModal}}>...</BsModal>

This looks quite straight forward isn't it? But actually it's not working as expected as the modal can't be opened more than one time. You could find an Ember Twiddle to play around here: https://ember-twiddle.com/6f545b142fa78f23c9b9e3cdd419f18a?openFiles=templates.application.hbs%2C

Please note that it's not that easy to fix this. Altering showModal property in the same runloop isn't fixing it. An action that enforces the modal even if showModal is true when being called must do something like this:

import { next } from '@ember/runloop';

export default Controller.extend({
  actions: {
    enforceModal() {
      this.set('showModal', false);

      next(() => {
        this.set('showModal', true)
      });
    }
  }
});

That's awful and therefore it should be considered best practice to set showModal property to false in onHide or onHidden actions in my opinion: <BsModal @open={{showModal}} @onHide={{action (mut showModal) false}}> I'm suggesting to enforce that if open property is used.

Speaking more technically I recommend these changes:

• Use the open property of <BsModal> to distinguish between controlled and uncontrolled mode. The component is in controlled mode if and only if open is a Boolean value.
• Change the default value of @open to null so that it's still in uncontrolled mode by default.
• Show modal on initial render if open is null or true.
• Show modal after initial render if open changes to true.
• Close modal on user interaction unless open is true after onHide action (or onHide action returns false).

[simonihmig]

Lets not block v3 for this one as it seems to need more discussion. I'm quite sure there will be a v4 some time in the future.

Agree 😁

it should be considered best practice to set showModal property to false in onHide or onHidden actions in my opinion

Also agree here. In controlled mode you should actively prevent showModal and the actual visibility to diverge, as in your example: <BsModal @open={{showModal}} @onHide={{action (mut showModal) false}}>

Close modal on user interaction unless open is true after onHide action

Given the user is responsible as said before to manage the @open state, I think we don't have to do anything here other than call the action - in controlled mode (e.g. this.isControlled('open') === true). If the user wants to close it, (s)he will need to set @open to false explicitly.

I think we can apply the same pattern to other components as well, which currently do not supported a controlled mode, or very limited: Dropdown, Popover, Tooltip, more? So leaving this for v4 would make even more sense, so we can introduce a coherent API pattern.

[jelhan]

Sounds good. Will try to pick that one up after v3 has landed.

Should also add a note to documentation why @open is needed at all. A user might also try using an if block like this:

{{#if showModal}}
  <BsModal @onHide={{action (mut showModal) false}}> ... </BsModal>
{{/if}}

But this is breaking close animation, isn't it? Would be great if we could have async willDestroy hook to await the close animation before element is removed from DOM...

[simonihmig]

@jelhan regarding the controlled vs uncontrolled discussion (which just kind of popped up again), I just remembered that this API choice had its root in this discussion: https://gist.github.com/simonihmig/f4cada751b89b192f608bcb83ed1c22a

[tmlrd42]

For some reason this worked as expected before the v4 glimmer rewrite. I just updated to v4.4 and now there is definitely no sane way to use @OPEN anymore.
The only option is to use an #if block around the BsModel and thus to kill the hide animation.
Looks like this is not whats intended.
I would be grateful, if the original behavour could be restored somehow.

[jelhan]

@tmlrd42 Could you please open a new issue for that bug? Would be helpful if you could include the code needed to reproduce the issue in a newly created application.