-
-
Notifications
You must be signed in to change notification settings - Fork 78.8k
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
Comments
I am facing exactly same issue. Two active modals crashes firefox. |
Don't stack modals—feels like a broken experience from my perspective. |
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 |
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:
The 2 modals start competing about focus in a loop. |
In my humble opinion the right thing to do would be to enforce focus for the most recent modal only. |
+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. |
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. |
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. |
For backstory: enforceFocus() has been introduced in SHA: a404ac3 for "modal accessibility tab control". In the same commit, relaxFocus() was added as well:
The relaxFocus() method was then inlined in next commit (SHA: 10c6db4). The call is still present in hide():
|
@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. |
@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:
|
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. |
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/ |
A simple solution for Firefox crashing with multiple modals seems to only have one modal with the "fade"-class. Worked for me. [edit] |
any one pls help me, i m mew for twitter bootstrap so pls tel me , how to use for web devlopment? |
eminos can u pls suggest how to use bootstrap? |
@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. |
@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.
Hope that helps. |
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. |
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. |
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. |
I've found my fix for bootstrap-modal v2.2.0 here: #5022 (comment) |
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:
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. |
We solved this without using any hacks by doing the following:
// restore enforceFocus functionality 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. |
Thanks for the workaround! Why is the issue closed? The bug still exists in Bootstrap 2.3. |
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. |
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:
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. |
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. |
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. |
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.
|
didn't crash my firefox… am i crazy. im probably crazy. @mdo o_O |
@fat Dunno bro, this was over a month ago :). Apparently some folks saw this in Firefox, IE8, and Chrome. |
does it crash your shit in chrome O_o |
However, none of these browsers crashed when viewing that jsfiddle in the opening comment. |
i actually think its pretty funny, i say leave it 👯 |
that's us |
dancing |
…i'll fix it… |
Hooray! |
k fixed. |
this wont crash a browser anymore but it's an awful user experience. truly awful. |
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. |
@fat It's awful, but...... But sometimes it is impossible to explain this stupid customers =) |
Sample fixed http://jsfiddle.net/EUVWC/29/ |
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
http://jsfiddle.net/guyong/EUVWC/2/
The text was updated successfully, but these errors were encountered: