Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Dialog: Remove the instance-storing for the overlay, just create one …
…whenever it is needed. Heavily simplifies the code, while the memorly leak should be hardly an issue anymore, since fixed positioning restricts the overlay size to the window dimensions. Fixes #6058 - Dialog overlays are not properly reused when multiple instances of a Dialog exist.
  • Loading branch information
jzaefferer committed Dec 4, 2012
1 parent b9068c1 commit 1e8baf5
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 31 deletions.
11 changes: 0 additions & 11 deletions tests/unit/dialog/dialog_tickets.js
Expand Up @@ -107,17 +107,6 @@ test("#6137: dialog('open') causes form elements to reset on IE7", function() {
d1.remove();
});

test("#6645: Missing element not found check in overlay", function(){
expect(2);
var 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(); }});

equal($.ui.dialog.overlay.instances.length, 2, 'two overlays created');
d2.dialog('close');
equal($.ui.dialog.overlay.instances.length, 1, 'one overlay remains after closing the 2nd overlay');
d1.add(d2).remove();
});

// TODO merge this with the main destroy test
test("#4980: Destroy should place element back in original DOM position", function(){
expect( 2 );
Expand Down
27 changes: 7 additions & 20 deletions ui/jquery.ui.dialog.js
Expand Up @@ -661,13 +661,13 @@ $.widget("ui.dialog", {
if ( !this.options.modal ) {
return;
}
if ( $.ui.dialog.overlay.instances.length === 0 ) {
if ( $.ui.dialog.overlayInstances === 0 ) {
// prevent use of anchors and inputs
// we use a setTimeout in case the overlay is created from an
// event that we're going to be cancelling (see #2804)
setTimeout(function() {
// handle $(el).dialog().dialog('close') (see #4065)
if ( $.ui.dialog.overlay.instances.length ) {
if ( $.ui.dialog.overlayInstances ) {
$( document ).bind( "focusin.dialog-overlay", function( event ) {
if ( !$( event.target ).closest( ".ui-dialog").length ) {
event.preventDefault();
Expand All @@ -678,40 +678,27 @@ $.widget("ui.dialog", {
}, 1 );
}

// reuse old instances due to IE memory leak with alpha transparency (see #5185)
var $el = this.overlay = ( $.ui.dialog.overlay.oldInstances.pop() || $( "<div>" ).addClass( "ui-widget-overlay ui-front" ) );

var $el = this.overlay = $( "<div>" ).addClass( "ui-widget-overlay ui-front" );
$el.appendTo( this.document[ 0 ].body );

this._on( $el, {
mousedown: "_keepFocus"
});

$.ui.dialog.overlay.instances.push( $el );
$.ui.dialog.overlayInstances += 1;
},

_destroyOverlay: function() {
if ( !this.options.modal ) {
return;
}
var indexOf = $.inArray( this.overlay, $.ui.dialog.overlay.instances );

if ( indexOf !== -1 ) {
$.ui.dialog.overlay.oldInstances.push( $.ui.dialog.overlay.instances.splice( indexOf, 1 )[ 0 ] );
}

if ( $.ui.dialog.overlay.instances.length === 0 ) {
$.ui.dialog.overlayInstances -= 1;
if ( $.ui.dialog.overlayInstances === 0 ) {
$( [ document, window ] ).unbind( ".dialog-overlay" );
}

this.overlay.remove();
}
});

$.ui.dialog.overlay = {
instances: [],
oldInstances: []
};
$.ui.dialog.overlayInstances = 0;

// DEPRECATED
if ( $.uiBackCompat !== false ) {
Expand Down

4 comments on commit 1e8baf5

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't a new unit test supporting this behavior be created?

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

In theory, there shouldn't be tests needed for this change since it's just removing a workaround for a memory leak that we can't detect in JS. However, there are no tests for the modal option, so we should add tests covering the normal functionality, but I don't think there's anything specific to this ticket that needs to be added.

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

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

OK, I just randomly threw that out there. Whenever I see a commit with no test, I'm alarmed, but a commit that only removes tests, well that's downright unacceptable :-)

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

Sure, the test that was removed was for a bug in the workaround for the memory leak. That bug can't possibly occur if the workaround is removed :-)

Please sign in to comment.