Hiding in Modal-events breaks Modal

[kolbma]

There is some problem when calling Modal.hide() in the event-listener at least for shown.bs.modal and the Modal is already hidden.

var overwritten = {};
function showModal(modal, notimer) {
    overwritten[modal] = notimer || false;
    if (!modal.Modal) {
        new Modal(modal);
        modal.addEventListener('hide.bs.modal', function (evt) {
            overwritten[modal] = true;
        });
        modal.addEventListener('shown.bs.modal', function (evt) {
            setTimeout(function () {
                if (!overwritten[modal]) {
                    modal.Modal.hide();
                }
            }, 1500);
        });
    }
    modal.Modal.show();
}

Without the workaround with overwritten and the additional event listener for hide.bs.modal, at the next Modal.show(), the dialog shows up very short and will disappear immediately and the body-class stays set. So the UI is broken, because half of the Modal is left and you can not close the Modal anymore.
This happens when the Modal got hidden by some other call (e.g. escape key) before and the timeout-function is running and calling Modal.hide() afterwards.
There are no JS errors in console.
I've no tested if the problem is with hiding a hidden Modal or with the timeout-event or both in combination.

[thednp]

Give me a pen or fiddle so I can understand what you mean ;)

[Vuurvlieg]

I have the same problem; When trying to hide a openend modal, the backdrop is still active while the modal is not.

[jhunschejones]

@kolbma and @Vuurvlieg, what version of the library are you using? I had what I believe is the same problem on v2.0.19 but upgrading to v2.0.26 resolved the issue.

[kolbma]

I'm at v2.0.26
https://cdnjs.cloudflare.com/ajax/libs/bootstrap.native/2.0.26/bootstrap-native-v4.js

[Vuurvlieg]

@thednp @jhunschejones I am using the latest release, which is version 2.0.26.
I created a example and also video recorded. It is important to note that closing the modal often works perfectly, but in like 30% of the times the Modal bug appears.

[kolbma]

With this modified pen https://codepen.io/anon/pen/QPVPZo I've it always on the 2nd button click.

Ah, and simply wait, don't close the modals with the mouse. Just open it with mouse.

[thednp]

This seems to be the perfect test case to make it bulletproof. I will have a look at it asap.

[thednp]

@Vuurvlieg @kolbma your examples are very very hard to make it work. It almost feels like an unfair challenge when in my mind this component was meant to be a good alternative to the original jQuery plugin with light and fast code execution.

In the long run this would eventually get fixed as I know something like a setImediate or setTimeout alternative will come forward to come in handy solving this. Right now, a "gated" setup on each step of the execution could probably solve the issue, but I'm also open to any solution you might come with.

I will try and find more time and investigate this issue, but I will need your help in testing and validating the proposed code changed to come.

[Vuurvlieg]

@thednp

Some previous versions ago this bug did not occur. I think this bug was introduced in
#266. I believe before this we were using SetTimeout which was working fine for me.

but I will need your help in testing and validating the proposed code changed to come.

No problem. Just let me know when you are ready and I will test as much as needed.

[thednp]

Currently working on something that seems to make sense.

[thednp]

@Vuurvlieg @kolbma @jhunschejones and anyone else, please test the latest master with your hardest test and let me know how it goes asap.

Keep in mind:

• wait for animations to complete
• or force actions while animation is running
• observe the behavior
• want to know any suggestion on how to improve the code and the best on how to manage this case you've showcased above (a case I've tweaked and shown below)
• I want to mark this as "improved" and not fixed as your example is, as I said, very harsh and a bit unreasonable, we need to move on

UPDATE:

As for your example, this is a tweak for your example scripting

let modal = document.getElementById('modal1');
var opened = new Modal(modal, {backdrop: 'static', keyboard: false});

let timer = null;

function openModal1(el, ev) {
  ev.preventDefault();
  let modalHtml = modal.innerHTML;

  var initModal = (() => {
      modal.innerHTML = modalHtml.replace('Modal closed', 'Modal opened');
  });

  var resetModal = (() => {
      modal.innerHTML = modalHtml;
  });

  modal.addEventListener('shown.bs.modal', function() {
    clearTimeout(timer);
    timer = setTimeout(function() {
      modal.Modal.hide();
      console.log('.hide() at 2000ms');
    }, 2000);
  });
  modal.addEventListener('shown.bs.modal', function() {
    clearTimeout(timer);
    timer = setTimeout(function() {
      modal.Modal.hide();
      console.log('.hide() at 3000ms');
    }, 3000);
  });

  modal.addEventListener('shown.bs.modal', initModal, {once: true});
  modal.addEventListener('hide.bs.modal', resetModal, {once: true});

  opened.show();
}

Note:

• it's unnecessary to re-initialize the same element on click, it's a change I've made to improve performance, since you only need to show the modal on click, simply moved the modal initialization outside the click handler,
• re-initializing the modal on click is not a problem, as it allows you to reset options and change behavior on every instance, but you have to understand there is a new memory allocation for each new instance, the code can break because these different addresses can sometimes not link each other and allow for perfect logic flow, so before you re-initialize, you should consider a destroy of the previous initialization via modal.Modal = null
• keep in mind the code is meant to be fast, memory efficient, secure, all via private methods/constants and expose only public methods that are strictly tied to UI, something that's quite different from the original plugin and is tied directly to the above note,
• the new code on master is heavily in control for the timing of show/hide methods, which would require a similar handling for external scripting, so I added some timeouts and clear timeouts to help avoid code breaks, since setTimeout behaves totally random without clearance,
• IMHO the component does what is supposed to do: handle user events, transitionEnd, original events, keeps everything tight and efficient, the rest is your responsibility on design scripting for the UI

[Vuurvlieg]

@thednp Please dont close this issue as its not fixed yet.

I'd love to test your solution but unfortunately I ran into a new issue, which is a HTML form can not be submitted inside a Modal. Before may 28 did this work perfectly.
See https://codepen.io/anon/pen/qGvWGY: open the modal and try to submit the form. Nothing happend.

[thednp]

It was closed automatically as referenced in my commit.

[thednp]

Let's take this step by step. First I need to know if our original issue is fixed. Then we can handle the new one ok?

[thednp]

@Vuurvlieg solved, check master code.

[Vuurvlieg]

@thednp Thanks, submitting the form does work again!

So I've tested your solution from #281 (comment) and it seems to work for the example given by @kolbma. However, it does not work for the codepen example given by me earlier. Once I open a second model when the first modal is still openend, the backdrop is being removed completely.

Im not sure but my suggestion is to get rid of all SetTimeOut events and implement the transitionend_event functions. So I mean, when the Modal uses (fade) animation, the bootstrap events such as shown.bs.modal & hide.bs.modal should be fired, only if the transitionend_event does occur.

Let me clarify this by some code. (Again I'm not sure if this ever would work but it might be a solution to put inside the core of the Bootstrap native Modal component with some tweaking)

const el = document.querySelector('.modal.fade');

el.addEventListener('transitionstart', function() {
  // fire the "show.bs.modal" event
});`

el.addEventListener('transitioncancel', function() {
  // fire the "hide.bs.modal" event
});

el.addEventListener('transitionend', function() {
  // fire the "shown.bs.modal" or "hidden.bs.modal" event, depending on modal state
});

[thednp]

@Vuurvlieg why use a native script as if it's jQuery? Why initialize modal on button click? Why add even more event handlers on every click? You are creating more and more memory addresses, meaning you use more memory, you mess up the component private constants and variables, you are constantly bloating the DOM with new handlers (yes you set to once: true, but GC happens when it happens), overall it's inconsistent, it's not good development practice and I rarely see this kind of need, if ever.

On the question to why not add transitionend into the mix, we've had several versions with full implementation, however it didn't work with opening a new modal from a currently opened modal for some reason. I know it's not impossible, it may require some time to develop, but to be frank I'm happy with the current state.

The main difference with the original plugin is that many of the constants/variables and methods that are public in the original version are made private with our version, to make the best possible GC work. The major drawback of our implementation is that it won't allow us to do jQuery style init/access/re-innit without any performance penalty or code breaks.

So. Why not initialize modal, attach events, whatever, all before outside the click handler to only do the show() method?

Try and brake this re-written example in less than 100 clicks.

[Vuurvlieg]

@thednp

Why add even more event handlers on every click? You are creating more and more memory addresses, meaning you use more memory, you mess up the component private constants and variables, you are constantly bloating the DOM with new handlers (yes you set to once: true, but GC happens when it happens),

I'm sorry but I do not agree with this. Indeed I do use once: true, and if Bootstrap Native does work correctly and follow the correct standards, every related handler should be removed (from DOM) when closing the Modal. I have tested this in my website and indeed every handler is removed when the Modal is closed. It does not have any performance issues doing it this way. Everything runs very smooth and fast as long the handlers are being removed from DOM when needed.

Anyway, the only 2 problems I have right now with the latest master is that the backdrop is removed when I open the second modal while the first modal is still being open.

Also closing the second modal results in opening the first modal.

Check this example.

[thednp]

I put a lot of effort in solving this, tried everything I know, even if it doesn't affect me or a larger portion of the community, I was expecting you would confirm.

As I said, this library doesn't do jQuery, it doesn't replace original library functionality word for word, if you want to use it, you use it the way I show you working on my modified version of your example because unfortunately I don't have anything else to help you.

Meanwhile I'm going to close this until we have a solution or at least someone else struggling with the same situation.

[Vuurvlieg]

For the missing backdrop there is a very easy fix.

Inside the this.show = function() {} of the Modal component

if (document.querySelector('.modal-backdrop')) {
   removeOverlay();
}

Working example, adjustment is done in JS at line #1244

[thednp]

Will check that asap.

[thednp]

That's not a good resolution for some reasons:

• we already have removeOverlay() queued for the above part closing currently open modal;
• the demo already showcases opening a modal from another modal on V3 and main site, working perfect, and I tested it successfully with and without the fade class.

I suggest you fork the project and use your "fix".

[Vuurvlieg]

the demo already showcases opening a modal from another modal on V3 and main site, working perfect, and I tested it successfully with and without the fade class.

Well, then you should test it again carefully because the showcase doesn't work either. Same problem at other level.

I digged into the core code and its very logical that the backdrop is not shown. Its all about the timeouts with its intervals: When a second modal is being opened while the first one is already open, the current code executes the this.show() function first, then the this.hide() function, which hides the first model AND sets too late modalOverlay to zero, which results in the backdrop of the second modal is never created.

Simply adding a setInterval in the this.show() function of the Modal component solves this problem. For anybody with the same problem, take a look at this quick crafted working example, at JS line #1245.

[thednp]

@kolbma is your original issue solved in the master please?

[thednp]

@kolbma and @Vuurvlieg I've updated the wiki in regards to this issue.

https://github.com/thednp/bootstrap.native/wiki/FAQs#does-it-support-jquery-style-instance-hooks

[tamb]

I'm getting this issue too.

[thednp]

@tamb what is your use case?