Skip to content

Commit

Permalink
Dialog: Remove width, min-height, height styles on destroy. Fixes #81…
Browse files Browse the repository at this point in the history 
…19 - Dialog: Destroying a dialog leaves some styles changed.
  • Loading branch information
@scottgonzalez
scottgonzalez committed Dec 4, 2012
1 parent 975bde5 commit 3c2acc3
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 4 deletions.
5 changes: 5 additions & 0 deletions tests/unit/dialog/dialog_methods.js
View file
Expand Up @@ -35,6 +35,11 @@ test("init", function() {

test("destroy", function() {
expect( 6 );

// Dialogs are expected to be hidden on destroy, so make sure they're hidden
// before the test
$( "#dialog1, #form-dialog" ).hide();

domEqual( "#dialog1", function() {
var dialog = $( "#dialog1" ).dialog().dialog( "destroy" );
equal( dialog.parent()[ 0 ], $( "#qunit-fixture" )[ 0 ] );
Expand Down
7 changes: 3 additions & 4 deletions tests/unit/testsuite.js
View file
Expand Up @@ -212,7 +212,7 @@ window.domEqual = function( selector, modifier, message ) {
"tabIndex",
"title"
];
/*

function getElementStyles( elem ) {
var key, len,
style = elem.ownerDocument.defaultView ?
Expand All @@ -239,7 +239,7 @@ window.domEqual = function( selector, modifier, message ) {

return styles;
}
*/

function extract( elem ) {
if ( !elem || !elem.length ) {
QUnit.push( false, actual, expected,
Expand All @@ -257,8 +257,7 @@ 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.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
View file
Expand Up @@ -127,6 +127,11 @@ $.widget("ui.dialog", {
this.element
.removeUniqueId()
.removeClass( "ui-dialog-content ui-widget-content" )
.css({
width: "",
minHeight: "",
height: ""
})
.hide()
// without detaching first, the following becomes really slow
.detach();
Expand Down

12 comments on commit 3c2acc3

@domenic
Copy link

@domenic domenic commented on 3c2acc3 Dec 4, 2012

Choose a reason for hiding this comment

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

Doesn't this destroy any (inline) styles they had prior to be dialoged? The ticket contains a fiddle showing the problem: http://jsfiddle.net/aZ3CB/3/

@scottgonzalez
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but what is the use case that you're working with?

@domenic
Copy link

@domenic domenic commented on 3c2acc3 Dec 4, 2012

Choose a reason for hiding this comment

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

An element that is a sidebar but when the window gets too small pops out into a dialog.

@scottgonzalez
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 guess our two options if we want to actually restore those are either saving the three properties individually or just saving style.cssText. @mikesherov thoughts?

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

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

style.cssText gets the inline styles. You'd want getComputedStyle().cssText, however, that'll add a whole bunch of inline styles. So, saving the 3 properties is the way to go.

@scottgonzalez
Copy link
Member Author

Choose a reason for hiding this comment

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

My thought was that we only care about inline styles in this case, because that's what we're changing. But I agree that handling the 3 specific properties is better.

@scottgonzalez
Copy link
Member Author

Choose a reason for hiding this comment

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

@domenic Unfortunately there's no good way for us to restore the values, since we can't tell the difference between an explicit height and auto height. Clearing the inline styles seems like the best solution, unless you have another proposal.

@domenic
Copy link

@domenic domenic commented on 3c2acc3 Dec 5, 2012

Choose a reason for hiding this comment

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

@scottgonzalez can't take the time to prototype now, but I'm pretty sure there's a way to get the currently-applied inline styles for an element. Then you could save the ones that are there before you guys overwrite them, and restore them after the dialog is destroyed. We are doing something similar in our app as a workaround.

@scottgonzalez
Copy link
Member Author

Choose a reason for hiding this comment

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

@domenic I have an idea for this. I'll test it out now.

@scottgonzalez
Copy link
Member Author

Choose a reason for hiding this comment

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

@domenic Check out f59f5a8, this should address your issues.

@jzaefferer
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this breaks accordion tests in IE7: http://swarm.jquery.org/result/636370
Expects overflow: "visible", but actual is "scroll".

@scottgonzalez
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I know. That's why it was commented out the first time. We haven't tracked down the cause of the failure yet, but it seems to be a bug in IE7.

Please sign in to comment.