Fix modal overlay handling.

[dtognazzini]

Addresses #162.

These changes wrap the createOverlay and removeOverlay methods with reference counts to ensure that createOverlay/removeOverlay is only called when there are no other modals managing the overlay.

I also removed the bit about removing the "show" class from the overlay in hide since the overlay is going to be removed from the DOM by removeOverlay anyway if the ref-count hits zero.

[thednp]

Interesting find, thanks ;)

[thednp]

Hey the overlay doesn't seem to close at all after opened second modal, I fixed that.

[dtognazzini]

Ah, thanks for fixing that!

[dtognazzini]

@thednp

Looking at your fix, I'm not sure if the reference counting is working correctly. When ref-counting, the count should update every time either createOverlay or removeOverlay is called. The guard should ensure that the "action" (creating or removing the overlay) only takes place once for a sequence of createOverlay-removeOverlay pairs. Effectively, the ref-count says "there are X number of users of the overlay". When the count transitions from 0-1 the overlay should be shown. When the transitions from 1-0 the overlay should be removed. All other transitions should leave the overlay as-is.

With your changes, the count is incremented after the guard:

createOverlay = function() {
  if ( modalOverlayRefCount > 1 ) { return; }

  modalOverlayRefCount += 1;

  // create the overlay
}

removeOverlay = function() {
  if ( modalOverlayRefCount === 0 ) { return; }
   
  modalOverlayRefCount -= 1;

  // remove the overlay
}

Consider the following sequence of createOverlay/removeOverlay pairs:

createOverlay - ref count ==0, resulting ref count == 1 (overlay is created)
createOverlay - ref count ==1, resulting ref count == 1 (no op)
removeOverlay - ref count ==1, resulting ref count == 0 (overlay is removed)
removeOverlay - ref count ==0, resulting ref count == 0 (no op)

What I need is this:

createOverlay - ref count ==0, resulting ref count == 1 (overlay is created)
createOverlay - ref count ==1, resulting ref count == 2 (no op)
removeOverlay - ref count ==2, resulting ref count == 1 (no op)
removeOverlay - ref count ==1, resulting ref count == 0 (overlay is removed)

This second sequence removes the overlay only when the last removeOverlay is called. This behavior is achieved by updating the count ahead of the guard:

createOverlay = function() {
  modalOverlayRefCount += 1;

  if ( modalOverlayRefCount !== 1 ) { return; }

  // this is a 0-1 transition 

  // create the overlay
}

removeOverlay = function() {
  modalOverlayRefCount -= 1;

  if ( modalOverlayRefCount !== 0 ) { return; }

  // this is a 1-0 transition 

  // remove the overlay
}

[thednp]

I don't exactly know what you need, my changes make sure it works and I tested several times, but I might consider reverting to previous code revision.

[dtognazzini]

@thednp

I'd like the backdrop to stay up while modals are on the page.

I've created some jsfiddles to demonstrate.

Here is v2.0.15 demonstrating the issue:
https://jsfiddle.net/naonhpbp/8/

Here is the same code but with my fix:
https://jsfiddle.net/fm9nmdzL/2/

Here is the same code but with your fix that is currently in v2.0.17:
https://jsfiddle.net/46hvkraL/4/

Above you said:

... the overlay doesn't seem to close at all after opened second modal

Would you mind putting together a jsFiddle to show me the issue? I can look into it.

Thanks for your time and this project - it's awesome!

[thednp]

Your script example:

firstModalEl.addEventListener('hidden.bs.modal', openSecondModal, false); 

This is not required, also you don't have to initialize modals on click, you just initialize button/modal like I wrote in the documentation:

• If you initialize a button, it should have proper attributes to target corresponding modal,
• If you initialize a modal, it can be opened like this myOtherButton.onclick = function(){ myModalElement.Modal.open(); }

All my examples here work flawless. I don't understand why you need to complicate things. That's how it is supposed to be used.

[thednp]

I think hide event would be better to be used instead of hidden, when the overlay is still present.

firstModalEl.addEventListener('hide.bs.modal', openSecondModal, false); 

[dtognazzini]

RE:

I think hide event would be better to be used instead of hidden

The hide event is when the modal is starting to be hidden. The hidden event is after the modal has been hidden.

RE: #163 (comment)
I am not trying to complicate things. The code in those jsfiddles is not my actual code.

My actual code uses React. The HTML is appended to the page as part of rendering a React component. The use case is that I have some action that pops a modal. If the user is not signed in, an authentication modal is popped first to prompt the user to sign in. After the user signs in on the authentication modal, the original modal is popped. The original modal is popped in the hidden event of the first modal.

The reason this behavior is occurring is explained in #30:

The bug happens because when the first modal closes, it removes the overlay after a short delay, and the second modal uses that same overlay element, which gets removed.

I have not looked at your examples. I will look to see if you have that case. I will also pull together a jsfiddle using normal bootstrap JS to see if it's an issue there. I'll also look at the examples with my changes and see if I can see the issue you pointed above.

If there's a better way to pop a modal after another modal has been hidden, let me know.

Thanks

[thednp]

I will try to recreate your use case with the fiddle you've made, I want to have it fixed for everybody not just me, but the way you do things aren't (at least to my mind) proper JavaScript.

I mean every time you click the button you create a new instance of Modal, over and over, it's gonna blow up somewhere. I have little to no experience with React, but that's what I understand from your example.

[dtognazzini]

RE: #163 (comment)
I'm not sure what you mean by "proper Javascript". Creating new instances of objects to encapsulate state is not only "proper Javascript", but also good OOP and is exactly what bootstrap is doing:
https:/twbs/bootstrap/blob/bab3246a1d7219e93ee12e09ff7608c5dd6bcbc2/js/src/modal.js#L517

I looked at your examples with my changes. I see that you are using data-toggle on the "Next" button in the first modal, here:

bootstrap.native/index.html

Line 500 in 168c77c

<button type="button" class="btn btn-success" data-toggle="modal" data-target="#anotherModal">Next</button>

Clicking this button causes clickHandler to be called with the new modal (here) and dismissHandler to be called with the old modal (here). Here's dismissHandler:

    dismissHandler = function(e) {
      var clickTarget = e[target];
      if ( open && (clickTarget[parentNode][getAttribute](dataDismiss) === component 
          || clickTarget[getAttribute](dataDismiss) === component
          || (clickTarget === modal && self[backdrop] !== staticString) ) ) {
        self.hide(); relatedTarget = null;
        e.preventDefault();
      }
    };

self.hide() is not being called for the old modal here. open is true, but one of the other expressions in the if-statement is evaluating to false. This means that removeOverlay is not being called for the old modal. That means that the createOverlay-removeOverlay calls pairing is not actually a pair. This is why my changes to use ref-counting don't work: removeOverlay is only called once, when the last modal is dismissed instead of twice. (once for each created modal).

I'm not sure what the behavior would be for normal bootstrap JS. I'll pull together a jsfiddle for that. On a cursory look at normal bootstrap v4 JS, I do see that the hidden event isn't triggered until after the overlay is hidden: https:/twbs/bootstrap/blob/bab3246a1d7219e93ee12e09ff7608c5dd6bcbc2/js/src/modal.js#L322

This is different than bootstrap.native where, as explained in #30, the hidden event is triggered before the overlay is managed:
https:/thednp/bootstrap.native/blob/2.0.15/lib/V3/modal-native.js#L142-L143

Changing bootstrap.native to match normal bootstrap's behavior to fire the hidden event after the overlay is managed would obviate the need for my fix.

[dtognazzini]

Here's an example using normal bootstrap JS:
https://jsfiddle.net/0sjwoaaa/10/

It works. That is, the overlay animates away and back again between the modals and finally away after dismissing the last modal.

[thednp]

Our code is supposed to do better in this case. I don't want to hide the overlay while closing visible modal and opening new one. That's what I had in mind from the start.

The thing is, your modalOverlayRefCount doesn't count between multiple instances of Modal because it's closed within the UMD closure and not in the global scope, so I will think of a better way to handle this case.

[dtognazzini]

modalOverlayRefCount is in the globals.js file which makes it global to the bootstrap.native closure, which is effectively global, barring pulling in more than one instance of bootstrap.native. Is that a use case you want to support?

RE:

Our code is supposed to do better in this case.

Why do different than bootstrap? I thought this library was suppose to match the behavior of standard bootstrap. Is that not the case?

[dtognazzini]

Lastly, the issue here is that the code in dismissHandler is breaking the create/remove pairing. That should be fixed regardless of my changes. If it were fixed, it seems you would accept my changes, no?

[dtognazzini]

Just stumbled upon this:
twbs/bootstrap#23586

I'll likely switch to this as soon as it's available. In the meantime, I'll stick with my fork of this library barring a better fix that is consistent with normal bootstrap behavior.

[thednp]

Why do different than bootstrap? I thought this library was suppose to match the behavior of standard bootstrap. Is that not the case?

In my mind doing things exactly like original plugins was a bit of a tinkering. I always wanted to make it better in some ways, because with plain JavaScript you can, still I think some of our functionality here was later added to the original plugins because it was a better way IMO.

[thednp]

OK I got this thing to work with the events as in your example, it works exactly the same as with jQuery version. But it also works the way I've set the first example.

Be prepared to test in a couple of hours.

[thednp]

@dtognazzini go check the master code.

[dtognazzini]

@thednp Finally getting around to this now. NPM can't find it version 2.0.18. Did you push?

[dtognazzini]

@thednp master is working well. Thanks!