Open modal multiple times and closing it leaves behind the dark overlay permanently

[carlosperate]

So if a modal is opened multiple times, and then exited only the latest overlay is removed and all other stay there permanently.

To test you can go to http://materializecss.com/modals.html and in the javascript console just run $('#modal1').openModal(); multiple times. You can see that after clicking any of the modal options only one overlay is removed.

[pascal-de-ladurantaye]

Problem is that the lean-overlay is not reused if it exists:

Following: The generated html after opening the modal 4 times.

<div class="lean-overlay" id="materialize-lean-overlay-1" style="z-index: 1002; display: block; opacity: 0.5;"></div>
<div class="lean-overlay" id="materialize-lean-overlay-2" style="z-index: 1004; display: block; opacity: 0.5;"></div>
<div class="lean-overlay" id="materialize-lean-overlay-3" style="z-index: 1006; display: block; opacity: 0.5;"></div>
<div class="lean-overlay" id="materialize-lean-overlay-4" style="z-index: 1008; display: block; opacity: 0.5;"></div>

[carlosperate]

It might be relevant to mention that previous behaviour probably did reuse the overlay, as if you opened 2 different modals and then closed 1 one of them it would remove all the overlay completely and you would end up with the other modal still present in the page without the dark overlay.
#1512

[pascal-de-ladurantaye]

Couldn't each modal have it's own overlay reference and reuse it when it is opened a second time?

However, this would have the (probably already existing) side-effect of having a darker overlay when opening 2 different modals at the same time.

[carlosperate]

Yeah, I think we need to be able to easy track the number of overlays opened and the cleanest way could probably be to add a data- attribute to a single overlay.

The current modal add overlay code can be see in the leanModal.js - L23-61 file. As you have shown, the code will add a new div with a unique identifier each time a modal is opened. I think instead it could add a single div with a unique identifier (so try to getElementById with the unique identifier and only add the new div to the DOM if not found), and then edit an html5 data- attribute with a counter of how many modals remain opened (so openModal adds to the counter, and closeModal subtracts from the counter), which will then only remove the overlay when the counter reaches 0.

In essence having something like:

<div class="lean-overlay" id="materialize-single-lean-overlay" data-overlay-counter="3"></div>

The main reason I suggest to add the counter as an attribute is because then other components (like the side menus, date pickers, or whatever) can just reuse the same component.

Otherwise the variable _lastID could be replaced to an internal overlay counter.

[carlosperate]

Or the since the current code edits the modal div display option between none and block it could easily check if it is already block do not add an extra overlay in that case.

[pascal-de-ladurantaye]

The problem i see with this implementation is the following:

• We open the same modal 4 times : data-overlay-counter="4"
• We close the modal : data-overlay-counter="3"
• There is no modal left but the overlay is still there.

We need a way to distinguish opening the modal when it is already opened and opening it when its closed IMO.

[carlosperate]

We need a way to distinguish opening the modal when it is already opened and opening it when its closed IMO.

Check my last message. I would submit a pull request for that quick fix, but I do not have any of the build requirements to test it, so I hope somebody else could try to implement that.

[pascal-de-ladurantaye]

You are totally right. I will try to make a pull request for this tonight.

[pascal-de-ladurantaye]

Do we really want to be able to open an already opened modal?

If it is not the case, we could simply check if the modal is already opened at the beginning and return if it is.

Please tell me what you think about this.

[carlosperate]

Check how the z-index is calculated, if you have multiple modals opened and try to reopen one that is a level below, then z-index still needs to be recalculated.

[carlosperate]

hey @pascal-de-ladurantaye, were you able to make any progress on this?

[pascal-de-ladurantaye]

For the first time in years, I didn't have time to touch my computer for a full weekend 0_o
I was planning on making the setup tonight and make a first attempt to a fix.

[pascal-de-ladurantaye]

@carlosperate I did not test with bottom-sheet modals but I assume it will react the same.

[cryptoquick]

Looking forward to seeing this fix in the next release. 👍

[pascal-de-ladurantaye]

@Dogfalo @acburst Is there something wrong with the pull request i submitted? I see you are merging other PR but not this one.

[Dogfalo]

It takes time to go through PR to check code quality and whether or not
they work so we just did not get to yours yet. We will take a look soon.
On Jul 20, 2015 10:58 AM, "Pascal de Ladurantaye" notifications@github.com
wrote:

@Dogfalo https://github.com/Dogfalo @acburst
https://github.com/acburst Is there something wrong with the pull
request i submitted^ I see you are merging other PR but not this one.

—
Reply to this email directly or view it on GitHub
#1647 (comment)
.

[alexofob]

This issue still persist. When are we getting a fix.
Thanks

[alexofob]

I am not not sure whether this issue is specific to meteor. But below is a simple fix I did:

$('.modal-trigger').leanModal(
dismissible: true
complete: ->
$('.lean-overlay').remove()

[JonathanGuo]

I actually submitted a pull request on Jun 25. But no response at all. I have just merged latest master branch and hopefully this time someone would look at it.

Anyway I still implement the code in my project to fix the issue. If you guys have some issue you can simple use my code. #1590

Copy the code and use grunt to recompile the materialize.js and materialize.min.js.

[gg4u]

Is this issue solved?
I keep to see modal _IDs duplicated.

Also, how to set dimissable without calling a closeModal() or openModal() actions?

Suppose you want to block a modal for a call to action.
As a callback, modal become updated.
You don't want to close it, and allow user read update.

Then, I simply would like to click on the lean-modal background, but it stays blocked.

I tried to override dimissable click on the layer but it did not work.

    $modal = $('#modal-call-action'),
    $overlay = $('<div class="lean-overlay"></div>'),
    $overlay.click(function() {
          $modal.closeModal({dismissible : true, complete: function() { $('.lean-overlay').remove() }});
        });

I currently overrided it like:

  function updateDismissable() {

    $('.lean-overlay').on('click',function() { 
        $('.lean-overlay').remove(); // fix for multiple overlayers

        $('#modal-share-login').closeModal({ dismissible: true, complete: function() { $('.lean-overlay').remove() }} );

    })
  }

but something cleaner would be better.

Also, could you explain the difference for calling $overlay = $('<div class="lean-overlay"></div>') VS $('.lean-overlay') ?

The first return me one object only; the latter the array of all overlapping lean-overlays div.

Thank you!

[romant]

confirming that @alexofob's workaround of adding the following to initialisation works.

complete: function () {
      $('.lean-overlay').remove();
    }

look forward to this not being necessary.

[pascal-de-ladurantaye]

This has been fixed for over 8 months.... If only they merged the PR.

Pascal de Ladurantaye
Génie Logiciel 4e année
VP-Technologie du CEGE
École Polytechnique de Montréal

On Thu, Jan 21, 2016 at 11:41 AM, Bobbie Stump notifications@github.com
wrote:

What the code above does is check if there's already a "lean-overlay" on
the page and then, instead of appending another one, it will just "show"
the one that's already there.

—
Reply to this email directly or view it on GitHub
#1647 (comment)
.

[bstump]

I had to do a LOT more than "remove lean-overlay". Looks like it's an issue when you use a jQuery multi-selector and/or when you add content to the modal through AJAX. I have attached a file with the fix.
leanModal.js.zip

[bstump]

@pascal-de-ladurantaye I just grabbed leanModal.js from the master repo and it's still broken. My zip file above fixes it. What branch is this issue resolved in?

[pascal-de-ladurantaye]

#1691

[isaachinman]

Why has this not been merged? Adding the removal of lean-overlay to complete is such an easy fix, cmon guys.

[carlosperate]

While I do agree this should have been fixed a while ago it's not as simple as just removing the overlay. You can have multiple modals opened, each overlaying the previous modal, what happens then when you reopen a modal that was hidden behind a newer one? Also, if you close any of those, only it's relevant overlay should be removed and the rest left behind.

All that said, this behaviour has been taken in consideration and fixed in #1691 ...

[isaachinman]

Ah yes, wasn't aware of those complications. At any rate, cheers, I see #1691 is due to be merged imminently.

[JonathanGuo]

To anyone who wants to solve this problem by using my code and recompiling the package. please checkout the file https://github.com/JonathanGuo/materialize/blob/Fixed-duplicated-leanModal-overlays/js/leanModal.js

I created a pull request #1590 ages ago and never been merged. This piece of code perfectly solved this issue. We already used this in our product. I am not going to resubmit my code and hope the code to be merged.

[samudranb]

@JonathanGuo could you please help me understand how I could apply your fix?

I'm using the materialize:materialize package on meteor, and I can't find the leanModal.js file anywhere. Do I have to remove the package, download the materialize, and your file, and add it back? That might cause other packages which have dependency on materialize to break. 😢

[samudranb]

Honestly though, I do wish this issue was closed if the pull request is valid!

[JonathanGuo]

@samudranb I know it's a pain. So in our project I folked the Materializecss and applied the changes. Then I have my own gruntfile.js to build the js file. In this way there is no conflict but every time you update the Materializecss package, you need to run your own gruntfile.js to generate the file.

For your gruntfile, you can simply copy https://github.com/Dogfalo/materialize/blob/master/Gruntfile.js and change the leanModel.js route to yours.

[samudranb]

Thanks @JonathanGuo ! I'll check it out

[Metisse]

hi ! i found a solution for me maybe it's work for you :

      $(document).ready(function(){
            $('#modal').click(function() {
                 $('#modal1').openModal();
            });
            if ($('body').css(''))
            {
                $('.lean-overlay').remove();
            }
        });

[Sc07713]

Hi there, I thought I would just add my fix that I placed in materialize.js within the openModal extension.

If there is more than 2 layers, it will just delete them, its not the most efficient method but it seems to operate better for me than the fix to leanModal.js.

$('.lean-overlay').each(function (i, obj) { if (i > 1) { $('.lean-overlay').slice(i).remove(); } });

[carlosperate]

So what happens if you have one modal opened, and then a different one opens on top?

[Sc07713]

It only ever has two overlays max. 0 and 1 in the .each statement.

If there was a 3rd one I would bump it up to 2. However I think it may struggle if there were anymore than 2.

It seems to work better than the leanModal fix for the time being. (well atleast for my implementation)

The lean modal fix was buggy as, and for some weird reason was shutting down my modals and overlays for no reason that I could see.

The above needs to be in a function that operates everytime a modal opens. (or if you prefer every second modal).

I did forget to mention you still need:

$('.modal-trigger').leanModal({ complete: function () { $('.lean-overlay').remove(); } });

However when it comes to duplicate overlays appearing, my code helps prevent the background from going all black by removing the "too many" overlays factor.

So if you are getting duplicate overlays use my code above, and to remove the overlays use the above.

It looks way smoother.

[gubi]

Above patches doesn't fix for me.
I have this environment:

• an ever-present button that trigger
  • a modal with button that launch
    • an async call that on success close modal and change page contents.

So, opening and closing recursively modals, it remembers the last ".lean-overlay" ID and then uses another new one. This cause the issue!
I think that a simple static "lean-overlay", switched on and off, could be a better lightweight approach...
Anyway, I used this simple patch and on Firefox 44.0 seems to work:

$('.modal-trigger').leanModal({
    dismissible: true,
    opacity: 0.5,
    in_duration: 300,
    out_duration: 200,
    ready: function() {
        if($(".lean-overlay").length > 1) {
            $(".lean-overlay:not(:first)").each(function() {
                $(this).remove();
            });
        }
    },
    complete: function() {
        $(".lean-overlay").each(function() {
            $(this).remove();
        });
    }
});

[Krissams]

Try this one guys,
$(document).ready(function(){ $('.modal-trigger').leanModal(); });

[ceeeff]

Not sure if this is the same problem I was having (sounds the same). I resolved by using unbind() before calling.

[jumbosushi]

@gubi's example did the trick for me (thanks!). Hope they will patch this at some point tho.

[gdoron]

@Dogfalo @acburst Are you still actively maintaing your awesome library?
If yes, please merge the pull request, it's more than a year since it was opened.
If no, please add new collaborates to the project, I'm sure there're many users who can and want to make this project continue.

Thoughts? @pascal-de-ladurantaye ?

[jhillhouse92]

@Dogfalo What's the status with this issue?

[tomscholz]

@gdoron The PR was merged. And here something about the state of this project
Does the issue still occur to you?
@jhillhouse92 Thats the question ;)

[jhillhouse92]

@tomscholz Indeed it is, glad traction is starting to take place :)... with 25k+ stars that's a pretty important library to lots of people. Lets keep it that way.

[tomscholz]

Closed due inactivity and being solved.