Navigation Menu

Skip to content

Commit

Permalink
Dialog: Cleanup style properties on _destroy. Reenables style check i…
Browse files Browse the repository at this point in the history
…n domEqual, while removing commented and unnecessary old code. Fixes #8119 - Dialog: Destroying a dialog leaves style, scrollleft, and scrolltop leftovers.
  • Loading branch information
jzaefferer committed Dec 3, 2012
1 parent 8b15aaf commit d687a1b
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 30 deletions.
2 changes: 2 additions & 0 deletions tests/unit/dialog/dialog_methods.js
Expand Up @@ -35,6 +35,8 @@ test("init", function() {

test("destroy", function() {
expect( 6 );
// expect dialogs to be hidden before and after
$( "#dialog1, #form-dialog" ).hide();
domEqual( "#dialog1", function() {
var dialog = $( "#dialog1" ).dialog().dialog( "destroy" );
equal( dialog.parent()[ 0 ], $( "#qunit-fixture" )[ 0 ] );
Expand Down
32 changes: 2 additions & 30 deletions tests/unit/testsuite.js
Expand Up @@ -210,36 +210,10 @@ window.domEqual = function( selector, modifier, message ) {
"nodeName",
"role",
"tabIndex",
"title"
"title",
"style"
];
/*
function getElementStyles( elem ) {
var key, len,
style = elem.ownerDocument.defaultView ?
elem.ownerDocument.defaultView.getComputedStyle( elem, null ) :
elem.currentStyle,
styles = {};
if ( style && style.length && style[ 0 ] && style[ style[ 0 ] ] ) {
len = style.length;
while ( len-- ) {
key = style[ len ];
if ( typeof style[ key ] === "string" ) {
styles[ $.camelCase( key ) ] = style[ key ];
}
}
// support: Opera, IE <9
} else {
for ( key in style ) {
if ( typeof style[ key ] === "string" ) {
styles[ key ] = style[ key ];
}
}
}

return styles;
}
*/
function extract( elem ) {
if ( !elem || !elem.length ) {
QUnit.push( false, actual, expected,
Expand All @@ -257,8 +231,6 @@ window.domEqual = function( selector, modifier, message ) {
var value = elem.attr( attr );
result[ attr ] = value !== undefined ? value : "";
});
// TODO: Enable when we can figure out what's happening with accordion
//result.style = getElementStyles( elem[ 0 ] );
result.events = $._data( elem[ 0 ], "events" );
result.data = $.extend( {}, elem.data() );
delete result.data[ $.expando ];
Expand Down
5 changes: 5 additions & 0 deletions ui/jquery.ui.dialog.js
Expand Up @@ -127,6 +127,11 @@ $.widget("ui.dialog", {
this.element
.removeUniqueId()
.removeClass( "ui-dialog-content ui-widget-content" )
.css({
"width": "",
"min-height": "",
"height": ""
})
.hide()
// without detaching first, the following becomes really slow
.detach();
Expand Down

8 comments on commit d687a1b

@jzaefferer
Copy link
Member Author

Choose a reason for hiding this comment

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

This highlighted two issues that were fixed in 53b940f and b9068c1

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this is a legitimate check. We want to verify that the all styles are the same, not that the style attribute hasn't changed. @mikesherov @gnarf37 Isn't all the code that got deleted necessary to actually compare styles?

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

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

The old code checked computed styles, the new code checks the style attribute, which is a completely different thing. This is not a legitimate check.

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

Reverted in 050e71b. I'll re-implement with the proper style checks.

@jzaefferer
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand why we care for computed styles. What's wrong with just checking the style attribute?

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

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

This is what's wrong: http://jsfiddle.net/vry3v/

@jzaefferer
Copy link
Member Author

Choose a reason for hiding this comment

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

Still not getting it. jQuery UI adds stuff to the style attribute, so it should remove that stuff. Why does it matter that the computed style doesn't necessarily match that?

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

Because computed styles are what users care about.

Please sign in to comment.