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

Modal Close On Escape keyboard = true Does Not Work #4663

Closed
nodesocket opened this issue Aug 23, 2012 · 32 comments
Closed

Modal Close On Escape keyboard = true Does Not Work #4663

nodesocket opened this issue Aug 23, 2012 · 32 comments
Labels

Comments

@nodesocket
Copy link

The JavaScript modal according to the doc should close when escape is pressed by default:

    keyboard        boolean    true Closes the modal when escape key is pressed

Even setting keyboard = true still does not close the modal when escape key is pressed.

@thezoggy
Copy link

works fine here...

@nodesocket
Copy link
Author

Are you using the latest version 2.1.0? This functionality used to work for me as well in 2.0.4 but seems to have broke in the newest version.

@thezoggy
Copy link

latest is 2.1.1-wip, you tried that ?

@nodesocket
Copy link
Author

Only want to use releases. Can you confirm that in 2.1.0 is was broken though?

@nodesocket
Copy link
Author

Confirmed, does not work properly in 2.1.0 see the following jsFiddle:

http://jsfiddle.net/4EUS6/

Click the button, then hit escape. The modal should close, but it does not.

@fat
Copy link
Member

fat commented Aug 27, 2012

Hey @nodesocket,

Thanks for opening this issue! Unfortunately, it looks like it fails to pass the criteria neccessary for submitting to bootstrap. The following things are currently failing:

  • should include a jsfiddle/jsbin illustrating the problem if tagged with js but not a feature

For a full list of issue filing guidelines, please refer to the bootstrap issue filing guidelines.

thanks!

@fat fat closed this as completed Aug 27, 2012
@nodesocket
Copy link
Author

The fiddle is right here: http://jsfiddle.net/4EUS6/

@fat fat reopened this Aug 28, 2012
@alterphp
Copy link

alterphp commented Sep 2, 2012

Here is the Firebug console error message I get for this issue :

Exposing chrome JS objects to content without exposedProps is insecure and deprecated. See https://developer.mozilla.org/en/XPConnect_wrappers for more information.

src = target[ name ]; jquery.js (ligne 349)

jQuery line involved is inthis block of code :

jQuery.extend = jQuery.fn.extend = function() {
    var options, name, src, copy, copyIsArray, clone,
        target = arguments[0] || {},
        i = 1,
        length = arguments.length,
        deep = false;

    // Handle a deep copy situation
    if ( typeof target === "boolean" ) {
        deep = target;
        target = arguments[1] || {};
        // skip the boolean and the target
        i = 2;
    }

    // Handle case when target is a string or something (possible in deep copy)
    if ( typeof target !== "object" && !jQuery.isFunction(target) ) {
        target = {};
    }

    // extend jQuery itself if only one argument is passed
    if ( length === i ) {
        target = this;
        --i;
    }

    for ( ; i < length; i++ ) {
        // Only deal with non-null/undefined values
        if ( (options = arguments[ i ]) != null ) {
            // Extend the base object
            for ( name in options ) {
                src = target[ name ];
                copy = options[ name ];

                // Prevent never-ending loop
                if ( target === copy ) {
                    continue;
                }

                // Recurse if we're merging plain objects or arrays
                if ( deep && copy && ( jQuery.isPlainObject(copy) || (copyIsArray = jQuery.isArray(copy)) ) ) {
                    if ( copyIsArray ) {
                        copyIsArray = false;
                        clone = src && jQuery.isArray(src) ? src : [];

                    } else {
                        clone = src && jQuery.isPlainObject(src) ? src : {};
                    }

                    // Never move original objects, clone them
                    target[ name ] = jQuery.extend( deep, clone, copy );

                // Don't bring in undefined values
                } else if ( copy !== undefined ) {
                    target[ name ] = copy;
                }
            }
        }
    }

    // Return the modified object
    return target;
};

@nodesocket
Copy link
Author

Any ideas? Is this confirmed?

Thanks.

@sieppl
Copy link

sieppl commented Sep 6, 2012

I have the same problem with FF15. After closing the modal dialog (with a mouse click) I get the exposedProps error message. Any concurrent javascript action breaks then. However the example on twitter bootstrap is working fine. Will do further research..

EDIT:

After disabling the FireQuery Plugin and restarting Firefox the issue is solved!

@billsaysthis
Copy link

I am upgrading one of our apps from 2.0.1 to 2.1.1 and experiencing this issue on both Chrome and Firefox on OS X. I don't have FireQuery on Firefox, but I also don't see any errors in the console in either browser. The jsFiddle posted by @nodesocket gives me the same result, not sure if I need to create another, but would love to add my +1 on this issue ;)

@jschr
Copy link

jschr commented Sep 7, 2012

Make sure tabindex is set on the modal: http://jsfiddle.net/DCUYy/

This seems to be a breaking change in 2.1 as previously the keyup event was bound to the document but now is bound to the actual modal element. Hence, the the tabindex attribute is now required.

@nodesocket
Copy link
Author

Weird, in the previous version 2.0.X didn't have to set the tabindex. Any reason for the change binding to the actual modal element? Would prefer the previous behavior.

Thanks.

@jschr
Copy link

jschr commented Sep 7, 2012

Not sure, @fat may be able to offer a better explanation. My guess would be for performance or interference with other keyup events on the page.

A nice feature might be to automatically add tabindex="-1" if keyboard=true in the Modal class because right now this would be broken for anyone upgrading to 2.1.

@nodesocket
Copy link
Author

Word, I like the idea of adding tabindex="-1" automagically with JS.

@billsaysthis
Copy link

I did it in JS at the app's $.ready(). No way I was going to go back and
add that in all the views.

On Fri, Sep 7, 2012 at 2:19 PM, NodeSocket notifications@github.com wrote:

Word, I like the idea of adding tabindex="-1" automagically with JS.


Reply to this email directly or view it on GitHubhttps://github.com//issues/4663#issuecomment-8379297.

@andrewdeandrade
Copy link

Hey @nodesocket,

Thanks for opening this issue! Unfortunately, it looks like it fails to pass the criteria neccessary for submitting to bootstrap. The following things are currently failing:

  • should include a jsfiddle/jsbin illustrating the problem if tagged with js but not a feature

For a full list of issue filing guidelines, please refer to the bootstrap issue filing guidelines.

thanks!

@kaaposc
Copy link

kaaposc commented Nov 7, 2012

Got it working by adding a line in version 2.2.1 bootrstrap.js:917

$this.attr('tabindex', -1)

@Yohn
Copy link
Contributor

Yohn commented Nov 7, 2012

you shouldnt need to do that if you have tabindex="-1" on the modal div itself.. not tested though

@kaaposc
Copy link

kaaposc commented Nov 7, 2012

@Yohn, that also works as mentioned by @jschr earlier.
Adding tabindex="-1" to every modal div is more work than adding just one line to bootstrap.js
I just implemented @jschr's idea about automagically adding tabindex attribute to modal div...

@DigTheDoug
Copy link

Adding this.$element.attr('tabindex', -1) in the Modal class definition works to fix the escape issue, but it gives immediate focus to the modal on show which gives the whole modal window that ugly blue focus halo in Chrome, or the dotted lines in FF. Anyone else experienced this or have a good fix/workaround?

@mdo
Copy link
Member

mdo commented Dec 2, 2012

As this works in the docs now, I'm going to close it out. Please open a new issue against the latest code if the problem persists. Also, please include a jsfiddle with the bug and if possible a recommended solution. Thanks!

@mdo mdo closed this as completed Dec 2, 2012
@SergeyBarskiy
Copy link

I think it is broken again in 3.0.0 - jsfiddle to illustrate - http://jsfiddle.net/PRfHB/. If I remove "fade" it works.

@juthilo
Copy link
Collaborator

juthilo commented Oct 15, 2013

@SergeyBarskiy You used Bootstrap 2.1.0 CSS in your JSFiddle. Quick test showed that it worked with 3.0.0 CSS (you'll need to update the modal's markup according to our docs).

@SergeyBarskiy
Copy link

@juthilo. I updated the fiddle with 3.0 css, js was already 3.0. It does not quite work for me, only dismisses on escape when "fade" is not used. Am I missing something?

@SergeyBarskiy
Copy link

Sorry, missed link to updated fiddle http://jsfiddle.net/PRfHB/1/

@juthilo
Copy link
Collaborator

juthilo commented Oct 15, 2013

@SergeyBarskiy Works just fine: http://jsfiddle.net/PRfHB/2/

@SergeyBarskiy
Copy link

Thank you very much!, Julian I did not have class modal-dialog in my fiddle, that is why it did not work for me.

@cvrebert
Copy link
Collaborator

@SergeyBarskiy Yeah, as noted in the migration docs, the modal markup has changed a bunch in v3.

asfgit pushed a commit to apache/climate that referenced this issue Mar 18, 2014
- Adding "tabindex='-1'" to the modal's primary div tag fixes the issue.
  Additional information can be found at [1]

[1] twbs/bootstrap#4663

git-svn-id: https://svn.apache.org/repos/asf/incubator/climate/trunk/rcmet/src/main/ui@1483169 13f79535-47bb-0310-9956-ffa450edef68

Former-commit-id: 0a9e696
hustKiwi added a commit to hustKiwi/muui that referenced this issue Dec 6, 2014
@GearTheWorld
Copy link

GearTheWorld commented Aug 14, 2017

Not working in Microsoft Edge but working in all other browsers
Bootstrap 3.3.7

@patrickhlauke
Copy link
Member

@GearTheWorld don't necropost on an issue closed 4 years ago. also, 3.x is now in maintenance mode, and and focus is now on getting 4.x stable. if you find it's still an issue using the latest 4.x, please file a new fresh issue.

@GearTheWorld
Copy link

Sorry I didn't notice about the years. Ok I will test using 4.x Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests