Wrong behaviour of modal.dismiss in bootstrap.native.js

[oliseviche]

Wrong condition for removing event listeners exists in Modal.dismiss method.

if (!/\bin/.test(this.modal.className)) {
 this.modal.addEventListener('click', dismissHandler, false);
} else {
 this.modal.removeEventListener('click', dismissHandler, false);
}

After closing dialog 'in' class is removed before the code above is executed, thus leading to behaviour that every close event new click listeners are attached.

[thednp]

Do you have an example to showcase the behavior is wrong? What do you mean by wrong: is it different than original jQuery script or it's not fast enough?

[oliseviche]

Well, I will try to explain with pseudo-example, and if it wouldn't be understandable, will provide a fiddle.

The main problem is with dismiss and keydown methods. They both create handlers within them, which are subscribed to click event of modal element through closures.

The first problem is that when both dismiss and keydown methods are called, the modal element doesn't contains 'in' style class on it - it is removed right before in caller. So it doesn't matter, if you are showing or closing the window - event handlers are always attached on both operations, and it leads to memory leaks.

But, even if we will subscribe or unsubscribe from events according to correct 'in' style class, another error will occur - because handlers are created every time inside methods - they are new objects, and they are not detached from event. This handlers should be a members of this.

If you put a debug statements inside handlers now - you will see that the calls are accumulated as much, as you opened the dialogs.

I don't know if the problem exists in jquery plugins, because I don't use it. But if so, this behavior is still incorrect in its nature, and I think that this is a good practice to make this good project even better.

[thednp]

In my mind the scripts generally try to add listeners when needed and removed when not needed so that the CPU can carry less burden and really manage less memory.

I don't know if the problem exists in jquery plugins, because I don't use it. But if so, this behavior is still incorrect in its nature, and I think that this is a good practice to make this good project even better.

I perfectly agree with you here. In this case, I am open to any code suggestion you may have. I was thinking of some new options to customize what happens with certain components' handlers like dropdown but I didn't expect modal to be a memory leak problem.

[Jeff17Robbins]

Here's a jsfiddle showing the accumulation of previous event listeners.

After launching the Modal the first time, <div id="myModal1"...> has one click eventlistener.

Then close the Modal. Now, two click eventlisteners.

Launch the Modal again, now three click eventlisteners.

It keeps accumulating a click eventlistener for each .open(...) or .close(...).

[thednp]

Does this happen with Modal only or is it happening to other components?

I am now sure this has to be improved ASAP, and I also am sure this should not happen with Carousel.

So in my mind now, I am confident we should have something like this:

• when .open() is used, remove listeners that trigger the .open() method
• when .close() is used, add listeners back

Please confirm that this is the case.

[Jeff17Robbins]

I'm sorry, but I am new to both Bootstrap and Bootstrap-native.,so I am not quite sure what you are asking.

When you say "remove listeners that trigger the .open() method", I am confused. In the jsfiddle example, there is a <button> separate from the dialog which has a listener to then find via the data-target attribute its corresponding dialog and call that dialog's .open() method. Which works, but adds a click listener` to the dialog (not the launching button.)

Then, when you .close() the dialog, presumably via the listener added, there is yet another click added to the now-invisible dialog. It is this compounding of listeners on the dialog that is the problem.

What is the purpose of the click listeners on the dialog itself? Is it to handle the button(s) on the dialog, or some other purpose?

[thednp]

Ok, please make another test with Carousel, check if it multiplies any click event listeners and let me know, it should not.

If that is true, then Modal and other components have to be checked asap, and I mean ASAP.

[Jeff17Robbins]

I will try. I have never used Carousel, so it won't be fast turnaround.

[thednp]

You can simply test on the bootstrap.native demo site, I am curious of your finding.

UPDATE

I have already tested and I think you make a very good point with your finding. I might have to rework all handlers from all components. I think the future version is going to be even more simple code and perhaps lighter.

[Jeff17Robbins]

The problem in the Modal code is in its use of a CSS class named in as a flag.

All three routines which are meant to add an event listener in .open() and remove the listener in .close()

• .resize()

• .dismiss()

• .keydown()

use in as their flag via this test: if (!/\bin/.test(this.modal.className)) {

But this.close removes in too soon. So those 3 functions add a new listener on .close(), rather than removing the listener they are meant to remove.

    this.close = function() {
  
      if ( this.overlay ) {
        removeClass(this.overlay,'in');
      }
      removeClass(this.modal,'in'); // <----- 'in' removed before calling .resize(), .keydown(), and .dismiss()
      this.modal.setAttribute('aria-hidden', true);
  
      clearTimeout(timer);
      timer = setTimeout( function() {
        removeClass(document.body,'modal-open');
        self.resize();
        self.resetAdjustments();
        resetScrollbar();
  
        self.dismiss();
        self.keydown();
        self.modal.style.display = '';
      }, this.options.duration/2);

[Jeff17Robbins]

A couple other code problems in Modal:

• if (!/\bin/.test(this.modal.className)) {

I think you meant to put the "word boundary" on both sides of in:

if (!/\bin\b/.test(this.modal.className)) {

• clearTimeout(currentOpen.getAttribute('data-timer')); in this.open looks for an attribute data-timer that is never set in the code. So this line of code does nothing useful.

And two problems with removeClass() itself in Utils:

  removeClass = function(el,c) {
    if (el.classList) { el.classList.remove(c); } else { el.className = el.className.replace(c,'').replace(/^\s+|\s+$/g,''); }
  },

• Specifically, el.className.replace(c,'') will remove the first occurrence, e.g., of 'in', which, if any other class included the substring 'in' would lead to unintended consequences. For example, a class named 'raining' would become 'raing'. So you probably need a regexp there, too. Such as:

el.className.replace(new RegExp('\\b'+c+'\\b'),'')

• Also, replace(/^\s+|\s+$/g,'') only trims whitespace from the beginning or end of className, not the middle, where c could have been.

[thednp]

Your findings are truly interesting. I am super curious what would you suggest about your previous comment.

[Jeff17Robbins]

You could try moving these lines to the bottom of the function.

      if ( this.overlay ) {
        removeClass(this.overlay,'in');
      }
      removeClass(this.modal,'in'); // <----- 'in' removed before calling .resize(), .keydown(), and .dismiss()

[oliseviche]

Assumptions made by Jeff17Ribbinns are right, but even this changes would not prevent accumulating, because component doesn't save original handler to unsubscribe.
I will provide my local corrected version a bit later to show how in my case I've managed the problem. It isn't in pull request because I think that it somehow violates the main code and style flow of original library, but anyway it could give an idea about an improvments.

[Jeff17Robbins]

Good point. The handlers are local to a closure, so the removeEventListener isn't passed the same function as the addEventListener had been passed.

[oliseviche]

Let's go!

First changes includes adding a special toggled subscription routines for DOM events. The main purpose of it to generate a toggled subscribe/unsubscribe methods. First call of getSubscriptionToggler(...) returns a toggler, which when called subscribes/unsubscribes from listeners in a chainable manner;

function subscribe(element, event, handler) {
    element.addEventListener(event, handler, false);
    return function() {
        return unsubscribe(element, event, handler);
    };
};
function unsubscribe(element, event, handler) {
    element.removeEventListener(element, event, handler);
    return function() {
        return subscribe(element, event, handler);
    };
};
function getSubscriptionToggler(element, event, handler) {
    return function() {
        return subscribe(element, event, handler);
    };
};

Next, I've removed handlers generation from keydown and dismiss methods, and place them in common space of instance. Right after:

var self = this, timer, ..., checkScrollbar = function () {...}

goes our new handlers:

keyHandler = function(e) {
    if (self.options.keyboard && e.which == 27) {
        self.close();
    }
},
dismissHandler = function (e) {
    if ( e.target.parentNode.getAttribute('data-dismiss') === 'modal' 
        || e.target.getAttribute('data-dismiss') === 'modal' 
        || e.target === self.modal ) {
            e.preventDefault(); self.close()
    }
},

And after that we declare a scoped variables keyListenerToggler and clickListenerToggler:

keyListenerToggler = getSubscriptionToggler(document, 'keydown', keyHandler),
clickListenerToggler = getSubscriptionToggler(this.modal, 'click', dismissHandler);

This will hold our togglers inside modal instance object for future use.

And we are almost finished. The last step is to refactor this.keydown and this.dismiss. It's very simple, just use out listener togglers appropriate:

this.keydown = function() {
    keyListenerToggler = keyListenerToggler();
};
this.dismiss = function() {
    clickListenerToggler = clickListenerToggler();
};

[thednp]

@oliseviche I was looking for a fork in your profile to see this code working but I couldn't find one.

Please consider making a PR as soon as you see fit.

[thednp]

@oliseviche I think we can export this script to UTILS and use it in all components for the event handling.

[thewisenerd]

sorry for hijacking this thread, but the \bin regex is broken. I've commented about it in issue #54 ( #54 (comment) ).

@Jeff17Robbins suggested \bin\b ( #98 (comment) )which is broken as well.

[thednp]

@oliseviche I am considering to change the way events are handled all round, no more removing and re-adding events, we will simply toggle a local variable, for instance isSliding = true/false for carousel and check against it instead of adding/removing handlers.

BUT I will look into your code as well.

[thednp]

@oliseviche @thewisenerd @Jeff17Robbins please test the new commit

2a079b0