Skip to content

Commit

Permalink
Dialog: Before handling escape key presses, check if the default acti…
Browse files Browse the repository at this point in the history 
…on has been prevented. Fixes #6966 - Pressing ESC on dialog when 2 dialogs are open closes both dialogs.
  • Loading branch information
@fracmak @scottgonzalez
fracmak authored and scottgonzalez committed Mar 8, 2011
1 parent 3a0ec39 commit f999668
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 2 deletions.
25 changes: 25 additions & 0 deletions tests/unit/dialog/dialog_tickets.js
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -88,4 +88,29 @@ test("#6645: Missing element not found check in overlay", function(){
d1.add(d2).remove(); d1.add(d2).remove();
}); });


test("#6966: Escape key closes all dialogs, not the top one", function(){
expect(8);
// test with close function removing dialog
d1 = $('<div title="dialog 1">Dialog 1</div>').dialog({modal: true});
d2 = $('<div title="dialog 2">Dialog 2</div>').dialog({modal: true, close: function(){ d2.remove()}});
ok(d1.dialog("isOpen"), 'first dialog is open');
ok(d2.dialog("isOpen"), 'second dialog is open');
d2.simulate("keydown", {keyCode: $.ui.keyCode.ESCAPE});
ok(d1.dialog("isOpen"), 'first dialog still open');
ok(!d2.data('dialog'), 'second dialog is closed');
d2.remove();
d1.remove();

// test without close function removing dialog
d1 = $('<div title="dialog 1">Dialog 1</div>').dialog({modal: true});
d2 = $('<div title="dialog 2">Dialog 2</div>').dialog({modal: true});
ok(d1.dialog("isOpen"), 'first dialog is open');
ok(d2.dialog("isOpen"), 'second dialog is open');
d2.simulate("keydown", {keyCode: $.ui.keyCode.ESCAPE});
ok(d1.dialog("isOpen"), 'first dialog still open');
ok(!d2.dialog("isOpen"), 'second dialog is closed');
d2.remove();
d1.remove();
});

})(jQuery); })(jQuery);
4 changes: 2 additions & 2 deletions ui/jquery.ui.dialog.js
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ $.widget("ui.dialog", {
// TODO: move to stylesheet // TODO: move to stylesheet
.css( "outline", 0 ) .css( "outline", 0 )
.keydown(function( event ) { .keydown(function( event ) {
if ( options.closeOnEscape && event.keyCode && if ( options.closeOnEscape && !event.isDefaultPrevented() && event.keyCode &&
event.keyCode === $.ui.keyCode.ESCAPE ) { event.keyCode === $.ui.keyCode.ESCAPE ) {
self.close( event ); self.close( event );
event.preventDefault(); event.preventDefault();
Expand Down Expand Up @@ -699,7 +699,7 @@ $.extend( $.ui.dialog.overlay, {


// allow closing by pressing the escape key // allow closing by pressing the escape key
$( document ).bind( "keydown.dialog-overlay", function( event ) { $( document ).bind( "keydown.dialog-overlay", function( event ) {
if ( dialog.options.closeOnEscape && event.keyCode && if ( dialog.options.closeOnEscape && !event.isDefaultPrevented() && event.keyCode &&
event.keyCode === $.ui.keyCode.ESCAPE ) { event.keyCode === $.ui.keyCode.ESCAPE ) {


dialog.close( event ); dialog.close( event );
Expand Down

5 comments on commit f999668

@dustingraham
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this tested on 3 dialogs? I think when you hit ESC on the top dialog, then ESC again it may close the lowest dialog (should close the middle dialog the second time.)

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You think or you know? If there's a bug, please provide a reduced test case.

@dustingraham
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm on the clock with something else and ran into the problem. I'll try to build a test case in a bit.

(Edit*: I guess I figured it was trivial simple for devs that have used these test cases to throw in a third dialog and test, but I've never used jQuery unit tests, so I wasn't sure how to set them up is there a test runner or docs for testing? I guess I can search around a bit afterwards.)

@dustingraham
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so I just grabbed the test case, and added a third dialog.

http://jsfiddle.net/e4bxu/

You'll notice if you press "Escape" it will close dialog 3, but then if you immediately press "Escape" again, it will close the first dialog in the background, and leave the middle dialog (2) open.

@fracmak
Copy link
Contributor Author

@fracmak fracmak commented on f999668 May 3, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found the problem, this is only triggered if the escape key is triggered through the overlay, we only bind 1 keydown handler for all overlays, and we only do it on the first overlay, and it only holds a reference to the first dialog that gets registered because it's inside a if ( this.instances.length === 0 ) {. There's multiple ways to fix this. But the least invasive would be to move the key binder outside of the if statement, and then put in some code to make certain that the highest overlay instance is always the one that gets triggered. It's a little icky because we leave behind all the key binders until all the overlays are gone. I'll be updating my pull request with new tests and the fix shortly.

Please sign in to comment.