Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two Modal Dialogs Will Hang the Browser #4781

Closed
tangrui opened this issue Aug 28, 2012 · 50 comments
Closed

Two Modal Dialogs Will Hang the Browser #4781

tangrui opened this issue Aug 28, 2012 · 50 comments
Labels

Comments

@tangrui
Copy link

tangrui commented Aug 28, 2012

http://jsfiddle.net/guyong/EUVWC/2/

  1. Click the button read 'Click Me'
  2. On the modal dialog click 'Show Another'
  3. Then click the textbox
  4. Firefox will be unresponsive
@sam6230i
Copy link

I am facing exactly same issue. Two active modals crashes firefox.

@mdo
Copy link
Member

mdo commented Aug 28, 2012

Don't stack modals—feels like a broken experience from my perspective.

@mdo mdo closed this as completed Aug 28, 2012
@ztane
Copy link

ztane commented Sep 18, 2012

This could occur for example in case an user accesses functionality that requires login; a modal login box is shown. Now, if an error occurs within the login window, we cannot show the message in a modal box because it would crash the browser

@everilae
Copy link

Broken experience or not, JS library crashing browser with rather innocent operation is funky. The problem comes up to this in bootstrap/js/bootstrap-modal.js:

, enforceFocus: function () {
    var that = this
    $(document).on('focusin.modal', function (e) {
      if (that.$element[0] !== e.target && !that.$element.has(e.target).length) {
        that.$element.focus()
      }
    })
  }

The 2 modals start competing about focus in a loop.

@everilae
Copy link

In my humble opinion the right thing to do would be to enforce focus for the most recent modal only.

@eric-brechemier
Copy link

+1 - I experience the same issue, and a fix would be most helpful. JavaScript components should not change the focus unless this is explicitly requested.

@fat
Copy link
Member

fat commented Sep 18, 2012

we don't want to start keeping track of multiple modals. That method was added for accessibility. If you want multi-modal support, you should extend the modal class.

@everilae
Copy link

The track keeping doesn't require large additions - tried doing exactly that in pull request #5181. But, as it isn't a wanted feature, would it be possible at least for the vanilla modal class not to crash the browser? Even if people (like us) are doing bad UI design :P.

@eric-brechemier
Copy link

For backstory: enforceFocus() has been introduced in SHA: a404ac3 for "modal accessibility tab control".

In the same commit, relaxFocus() was added as well:

 relaxFocus: function () {
  $(document).off('focus.modal')
}

The relaxFocus() method was then inlined in next commit (SHA: 10c6db4). The call is still present in hide():

$(document).off('focusin.modal')

@eric-brechemier
Copy link

@fat I vote to reopen this issue. Can we add an option "tabcontrol" to enable enforceFocus() explicitly?

This would allow to describe in the documentation how this behavior enhances accessibility, and the fact that it can cause the browser to crash in case multiple modals are stacked.

@eric-brechemier
Copy link

@everilae thanks for the heads up on enforceFocus(). It is kind of hard to debug an issue when the whole browser becomes unresponsive!

I am not sure that explicit support for stacked modals is needed in Bootstrap though. In my case, I could avoid the issue by disabling tab control right after opening each modal:

$(document).off('focusin.modal');

@apaleslimghost
Copy link

This issue is fucking impossible to debug. The stack overflow errors in Chrome don't keep track of line numbers, and the callback in enforceFocus is anonymous, so looking at the stack trace gives you no clue whatsoever that it's from Bootstrap. Bad design or no, don't crash the browser.

@jschr
Copy link

jschr commented Oct 10, 2012

I took a crack at extending the modal class to allow for better responsiveness, multiple modals and a few other features. You can check out a demo here: http://jschr.github.com/bootstrap-modal/

@eminos
Copy link

eminos commented Oct 11, 2012

A simple solution for Firefox crashing with multiple modals seems to only have one modal with the "fade"-class. Worked for me.
http://www.chapter31.com/2011/10/27/twitter-bootstrap-modal-not-working-in-firefox/

[edit]
..or maybe not.

@sivakolluru
Copy link

any one pls help me, i m mew for twitter bootstrap so pls tel me , how to use for web devlopment?

@sivakolluru
Copy link

eminos can u pls suggest how to use bootstrap?

@eminos
Copy link

eminos commented Oct 11, 2012

@jschr Seems like you have a solution there. What is the easiest way to implement your code with existing Boostrap? I'm only interested in the multiple stackable modals fix for now.

@jschr
Copy link

jschr commented Oct 11, 2012

@eminos All you should need to do is include the additional css/js files from https://github.com/jschr/bootstrap-modal after the bootstrap files.

bootstrap-modal.js is dependent on bootstrap-modalmanager.js so make sure to include it after .

Hope that helps.

@Slogo
Copy link

Slogo commented Oct 18, 2012

It's absolutely ridiculous to close issues like this, this should be re-opened. Regardless of good design, bad design, or merely 'bad design because I say so' crashing a browser is unacceptable under any circumstances. If multiple modals are an issue, or displaying multiple modals in a certain way is not going to be supported, the issue should be handled gracefully or blocked from occurring.

It's fine to tell users to extend modal if they want multiple modals on a page. It's not fine to force them to work around a browser crashing bug when they do extend the modal class. You're asking everyone with a legitimate reason for multiple modals (and there ARE plenty) to work around a browser crashing bug on their own.

@Slogo
Copy link

Slogo commented Oct 18, 2012

Furthermore this is a relatively easy fix if you only want to support 1 modal at a time. enforceFocus can be updated to call $(document).off('focusin.modal');. The whole problem seems to stem from the fact that 2 modals can both have event handlers active for the focusin.modal event. That way you won't crash the browser.

@elwayman02
Copy link

Yea, this doesn't make any sense. There are plenty of valid reasons to open modals on top of modals...it's really quite unexpected that Bootstrap doesn't support this in the first place. But, even if for some strange reason you think it's actually preferable to NOT have this capability (o.O), you shouldn't be crashing people's browsers just because a second modal opened on their screen.

@onassar
Copy link

onassar commented Nov 6, 2012

I've found my fix for bootstrap-modal v2.2.0 here: #5022 (comment)
Not elegant, and removes the focusing of modal functionality, but helps me get passed this challenge without spending more time :)

@stephankaag
Copy link

I created a new pull request based on @everilae 's work #5833

@tylerjames
Copy link

I previously had a problem when stacking multiple modals where the z-index of the backdrop was the same for each modal, which meant you could interact with the modal behind the top modal. Someone, I can't remember who but the fates smile upon him, posted a hack to increase the z-index of the topmost modal.

Adding this hack and completely commenting out the enforceFocus() method has fixed the problem for me. This might not be a solution for people who don't want a backdrop, but right now it's better than crashing. This is not a general-case solution for multiple modals but it could probably be extended to work with multiple.

This was the solution:

  • add class 'level2' to a modal that appears on top of another modal
  • in your css add:
    .modal.level2 {
    z-index:1052
    }

That raises the z-index of the second modal. Now the backdrop for the top modal will obscure the lower modal and you won't have focus issues or any interaction with the lower modal.

@dcasadevall
Copy link

We solved this without using any hacks by doing the following:

  1. Before calling any of the modals:
    var oldFocus = jQuery().modal.Constructor.prototype.enforceFocus;
    jQuery().modal.Constructor.prototype.enforceFocus = function(){};
  2. After BOTH conflicting modals have been created (they can be shown or not, doesn't matter).

// restore enforceFocus functionality
jQuery().modal.Constructor.prototype.enforceFocus = oldFocus;

This might be ugly , but this way you don't have to modify any source and won't have any problems when they do update the issue.

@winterbe
Copy link

Thanks for the workaround!

Why is the issue closed? The bug still exists in Bootstrap 2.3.

@ztane
Copy link

ztane commented Feb 11, 2013

The bug was closed because the Bootstrap authors seem to think that PEBKAC - the programmer is stupid if at any time 2 modals are open at the same time.

@Saturate
Copy link

I tried to debug this, it was a nightmare due to the complexity of the app that I am building, I had no easy way to know what was going on without a stack-trace. Chrome just crashed and the log printed:

<error>
v.event.dispatch
o.handle.u
v.event.trigger
(anonymous function)
v.extend.each
v.fn.v.each
v.fn.extend.trigger
v.fn.(anonymous function)
(anonymous function)
v.event.dispatch
o.handle.u
v.event.trigger

Now after some time I found the reason; it was the modals. Now I never intended there to be more than one, but a bug made it possible, and I could not see this due to the browser crashing.

I really hope that you will at least throw an error, this is the right thing to do. Crashing the browser is not an option.

I do not want to see this:
chrome-error

I would rather like something like this:
chrome-error-iwant

@nosachamos
Copy link

Wow, closing this really??

At the place I work devs can't close bugs, only QA can. That's FOR A REASON. I honestly had a higher idea of the authors before they closed this ridiculously severe bug. Not even newbie would do that. Not sure I want to use anything from them anymore.

@0plus1
Copy link

0plus1 commented Apr 10, 2013

Devs seems to miss the point of this issue.

If you don't want people to use two modals in the same page (which is a totally understandable development position) you should throw an exception, log an error or whatever you want that it's not the current: "crash the whole browser".

Good software return an error upon an unexpected behaviour, bad software crash, very bad software ignore a real issue that, to be clear, is not something so uncommon to encounter as you might notice from this list of people reporting this error.

@jbenet
Copy link

jbenet commented Apr 16, 2013

I agree with the previously voiced comments.

Regardless of support for multiple modals, bootstrap SHOULD NOT crash browsers if two are thrown.

@mdo @fat please reopen this.

@vcfgvcfg
Copy link

I know stack modal is not supported by bootstrap.

For anyone who is facing this issue: a quick work around would be binding a focusin handle to the child modal and stop the focus stopPropagation. The hasClass('modal') check is in case that we have tooltip or calendar in modal.

        // hack to make bootstrap modal stacking work.
        $childModal.on('shown', function (e) {
            var $t = $(e.target);
            if ($t.hasClass('modal')) {
                $t.data('modal').$backdrop.zIndex(1047);
            }
        }).on('hidden', function (e) {
            // add modal open class since we still have opened modal.
            var $t = $(e.target);
            if ($t.hasClass('modal')) {
                $('body').addClass('modal-open');
            }
        }).on('focusin', function (e) {
            // stop focusin competition war.
            e.stopPropagation();
        });

@fat
Copy link
Member

fat commented May 25, 2013

didn't crash my firefox… am i crazy. im probably crazy. @mdo o_O

@fat
Copy link
Member

fat commented May 25, 2013

@nosachamos 👯

@mdo
Copy link
Member

mdo commented May 25, 2013

@fat Dunno bro, this was over a month ago :). Apparently some folks saw this in Firefox, IE8, and Chrome.

@fat
Copy link
Member

fat commented May 25, 2013

does it crash your shit in chrome O_o

@mdo
Copy link
Member

mdo commented May 25, 2013

Seeing two issues, one in IE8 (prompts an error_ and one in latest Firefox (console only).

screen shot 2013-05-24 at 10 20 49 pm

screen shot 2013-05-24 at 10 22 40 pm

@mdo
Copy link
Member

mdo commented May 25, 2013

Uncaught RangeError: Maximum call stack size exceeded in latest Chrome console as well.

However, none of these browsers crashed when viewing that jsfiddle in the opening comment.

@fat
Copy link
Member

fat commented May 25, 2013

i actually think its pretty funny, i say leave it 👯

@fat
Copy link
Member

fat commented May 25, 2013

that's us

@fat
Copy link
Member

fat commented May 25, 2013

dancing

@fat
Copy link
Member

fat commented May 25, 2013

…i'll fix it…

@jbenet
Copy link

jbenet commented May 25, 2013

Hooray!

fat added a commit that referenced this issue May 25, 2013
@fat
Copy link
Member

fat commented May 25, 2013

k fixed.

@fat fat closed this as completed May 25, 2013
@fat
Copy link
Member

fat commented May 25, 2013

this wont crash a browser anymore but it's an awful user experience. truly awful.

@grokys
Copy link

grokys commented Jun 25, 2013

Appreciated. I think we all agree it's an awful user experience, but when users tell me their browser is crashing and I have no idea why... Well that's much worse.

@Odrin
Copy link

Odrin commented Jun 26, 2013

@fat It's awful, but...... But sometimes it is impossible to explain this stupid customers =)

@pascalmartin
Copy link

Sample fixed http://jsfiddle.net/EUVWC/29/

benkomalo added a commit to Khan/bootstrap that referenced this issue Sep 3, 2013
Summary:
This is a hand-cherry-picked of a change included in bootstrap upstream (v3.0)
that fixes an infinite loop when double modals are introduced. Sadly, we use
double modals for related videos, as we haven't fleshed out the design for
that experience yet. In the meantime, IE users are crashing hard on this.

Test Plan:
make compiled_and_readable in parent webapp project
load dashboard, open a practice task with related video
click on related video, made sure it opened!

Reviewers: marcos, marcia

Reviewed By: marcos

Differential Revision: http://phabricator.khanacademy.org/D3920
@twbs twbs locked and limited conversation to collaborators Jun 9, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests