Opening a modal after another modal doesn't always show backdrop

[lekoala]

I've discovered one issue when opening a modal after another. I have is that sometimes the backdrop doesn't get properly the show class. It happens a bit randomly, so i guess it's due to the transition from the previous modal that sometimes overlaps the transition of the new modal.

From what I can see, sometimes getCurrentOpen(element) returns the new modal as expected, but sometimes not, and therefore hides the backdrop even if there is a new modal being shown.

Actually, this is even more obvious if you do a setTimeout, then it almost always happen

modal.addEventListener("hidden.bs.modal", () => {
  setTimeout(() => {
    // open modal here
  }, 10)
});

[lekoala]

example here https://codepen.io/lekoalabe/pen/wvPygrg click "bsn modal"

[thednp]

@lekoala can you please test the issues with latest master, I've changed where the hidden.bs.modal even is triggered in the last commit.

[lekoala]

@thednp i don't see any update on master about this ?
https://thednp.github.io/bootstrap.native/ actually it's also the case in the docs, if you go to the next modal, the backdrop is gone

[thednp]

f627a06#diff-1f315e5b1783cf3ac760b25752e346391f35462f8faba29918f78e33701306b7R3019-R3020

  hiddenModalEvent.relatedTarget = relatedTarget;
  dispatchEvent(element, hiddenModalEvent);

[thednp]

@lekoala ?

[lekoala]

@thednp it's not fixed as far as I can tell. The backdrop element is removed and therefore not visible on the second modal. It plays like this

beforeModalShow
modal-native.js:170 afterModalShow
modal-native.js:153 afterModalHide
modal-native.js:400 show => backdrop static
modal-native.js:184 beforeModalShow
modal-native.js:170 afterModalShow
modal-native.js:400 show => backdrop true
modal-native.js:153 afterModalHide
modal-native.js:184 beforeModalShow
modal-native.js:170 afterModalShow
modal-native.js:153 afterModalHide

the show call checks if the backdrop should be displayed (it's still there) but afterModalHide kills it.
I think it should be safe to move at least partially the backdrop to beforeModalShow or check again if it's still there

[lekoala]

doing this

function beforeModalShow(self) {
  const { element, hasFade, options, container } = self;
  setElementStyle(element, { display: 'block' });

  if(options.backdrop) {
     if (!container.contains(overlay)) {
        appendOverlay(container, hasFade, true);
        showOverlay();
      }
  }

kind of fixes the issue, but there is a glitch, it would be better if the overlay wasn't removed at all. so probably this needs a flag to be set in show() to say hey don't remove the overlay

[lekoala]

ok i found a solution.

i had a global let modalIsShowing = false; flag. you set it to true or false in onbeforeshow/onaftershow and you don't remove the overlay is the flag is true.
i can make a PR if that works for you

[thednp]

Hold on, I have to check things.

[thednp]

We already have that code in the show() method. The order of execution on show:

• show overlay if hidden or change its type otherwise
• show modal itself

This order cannot be changed.

The thing is: as you can see in the demo: modals open other modals or offcanvas open other modals, with different overlay options, it all works as configured. Perhaps you can try and only use show for the next modal without using hide() of the current open modal/offcanvas by yourself?

[lekoala]

Well my use case is this:

• I have a modal with an input field
• I collect all the data from the field when confirming the modal (=> closing) => this is calling hide()
• Listening to the hidden event, I may (or may not) choose to display another modal

So while I could do something else than hiding the first modal (basically I could make a button that triggers properly the next modal without hiding the first one), it's less convenient as you need to build custom stuff instead.
For reference, what I do works with regular bs5 javascript (i know i know, it's not the same, but still)

[lekoala]

My solution with the global flag is not too bad and only adds a couple of lines. I don't see any issue with offcanvas, it's just preventing a drop of the backdrop element between modals

[thednp]

I agree we have to make it work the same, but a global flag will not help. I will download your codepen and check myself, will provide a fix asap.

[thednp]

@lekoala there is an important thing for you to learn: BSN does not try to emulate what jQuery does in terms of event listener memory leaks, which has a complicated event delegation system that's very good at handling this issue.

So let me explain: you add new Modal instances on click and remove them once hidden.bs.hidden is triggered, however you must also remove the hidden.bs.modal listener, so for your examples 3 and 4 this should be the code for you to use:

let button3 = document.querySelector(".bsn-modal");
button3.addEventListener("click", (ev) => {
  console.log("click");
  var myModalInstance = new BSN.Modal("#myModal", {
    backdrop: "static",
    keyboard: false
  });
  myModalInstance.show();
  document.querySelector("#myModal").addEventListener("hidden.bs.modal", (ev) => {
    myModalInstance.show();
  }, { once: true });  // NOTICE THE EVENT LISTENER OPTIONS HERE
});

let button4 = document.querySelector(".bsn-modal-dispose");
button4.addEventListener("click", (ev) => {
  console.log("click");
  var myModalInstance = new BSN.Modal("#myModal", {
    backdrop: "static",
    keyboard: false
  });
  document.querySelector("#myModal").addEventListener("hidden.bs.modal", (ev) => {
    myModalInstance.dispose();
    console.log(BSN.Modal.getInstance("#myModal"))
  }, {once: true}); // NOTICE THE EVENT LISTENER OPTIONS HERE
  myModalInstance.show();
});

Alternatively you can do the following:

let button3 = document.querySelector(".bsn-modal");
button3.addEventListener("click", (ev) => {
  console.log("click");
  var myModalInstance = new BSN.Modal("#myModal", {
    backdrop: "static",
    keyboard: false
  });
  const eventListener3 = (ev) => {
    myModalInstance.show();
   document.querySelector("#myModal").removeEventListener("hidden.bs.modal", eventListener3);
  }
  myModalInstance.show();
  document.querySelector("#myModal").addEventListener("hidden.bs.modal", eventListener3 );
});

let button4 = document.querySelector(".bsn-modal-dispose");
button4.addEventListener("click", (ev) => {
  console.log("click");
  var myModalInstance = new BSN.Modal("#myModal", {
    backdrop: "static",
    keyboard: false
  });
  const eventListener4 = (ev) => {
    myModalInstance.dispose();
    document.querySelector("#myModal").removeEventListener("hidden.bs.modal", eventListener4)
  }
  document.querySelector("#myModal").addEventListener("hidden.bs.modal", eventListener4);
  myModalInstance.show();
});

The memory leak is gone, as well as ALL errors resulted in the broken relation between disposed instances and their original event listeners still available in the memory which still add up and trigger, with every click.

With this issue cleared I think these last two issues are practically solved.

Let me know if you spot any other issue.

[lekoala]

@thednp adding the listener option doesn't solve the missing backdrop part, no? (it doesn't for me) I was reusing the same modal out of laziness, but the point here is the missing backdrop.
I've updated the codepen https://codepen.io/lekoalabe/pen/wvPygrg
Indeed, the example with dispose is fixed in the current master so I removed that one

[thednp]

That issue doesn't occur for me on my local machine, it always works fine and as I said, you need to test with the latest github.com/thednp/bootstrap.native/tree/master and confirm back to me. Then we can publish a new version on NPM.

[thednp]

@lekoala can you confirm?

[lekoala]

@thednp yes works fine on master indeed!