#8119 closed bug (fixed)

Opened February 16, 2012 06:32PM UTC

Closed December 04, 2012 03:03PM UTC

Last modified December 05, 2012 04:53PM UTC

Dialog: Destroying a dialog leaves some styles changed

Reported by: Domenic Owned by:
Priority: minor Milestone: 1.10.0
Component: ui.dialog Version: 1.8.17
Keywords: Cc:
Blocked by: Blocking:
Description

When you do $(el).dialog("destroy"), el has leftover dialog-related style, scrolltop, and scrollleft attributes.

The style attributes in particular are troubling, since they override any non-inline styles the original element might have. But all three attributes overwrite previously-existing values.

Here is a test case to show what I mean. The bug is present in all browsers, but the test case only works in browsers with outerHTML support, so in particular not Firefox before 11.

http://jsfiddle.net/aZ3CB/3/

If someone could quickly agree with me that this is a bug, then I will work on a pull request to fix it.

Attachments (0)
Change History (10)

Changed October 11, 2012 02:47PM UTC by scottgonzalez comment:1

milestone: 1.9.0 → 1.10.0

Changed October 15, 2012 06:51PM UTC by petersendidit comment:2

status: new → open
summary: Destroying a dialog leaves style, scrollleft, and scrolltop leftovers → Dialog: Destroying a dialog leaves style, scrollleft, and scrolltop leftovers

Changed November 30, 2012 11:27AM UTC by jzaefferer comment:3

_comment0: Since we aren't (and probably can't) including the style propert in domEqual, how can or should we test this? \ \ Adding this to _destroy removes a few of the style properties we add: \ \ {{{.css({ \ "width": "", \ "min-height": "", \ "height": "" \ })}}} \ \ Currently dialog makes the assumption that the dialog element is hidden when the dialog is created and calls show(), then hide() on destroy, leaving display:none in the DOM. Is this assumption still correct? → 1354274904423026

Since we aren't (and probably can't) including the style propert in domEqual, how can or should we test this?

Adding this to _destroy removes a few of the style properties we add:

.css({
	"width": "",
	"min-height": "",
	"height": ""
})

Currently dialog makes the assumption that the dialog element is hidden when the dialog is created and calls show(), then hide() on destroy, leaving display:none in the DOM. Is this assumption still correct?

Changed November 30, 2012 11:38AM UTC by scottgonzalez comment:4

Replying to [comment:3 joern.zaefferer]:

Since we aren't (and probably can't) including the style propert in domEqual, how can or should we test this?

We should try again. I don't remember what failures we had when I added style checks, but I think there were very few issues.

Currently dialog makes the assumption that the dialog element is hidden when the dialog is created and calls show(), then hide() on destroy, leaving display:none in the DOM. Is this assumption still correct?

Well, the element will generally be hidden, but it may not be explicitly hidden with display: none; on the element.

Changed December 03, 2012 11:36PM UTC by Jörn Zaefferer comment:5

resolution: → fixed
status: open → closed

Dialog: Cleanup style properties on _destroy. Reenables style check in domEqual, while removing commented and unnecessary old code. Fixes #8119 - Dialog: Destroying a dialog leaves style, scrollleft, and scrolltop leftovers.

Changeset: d687a1b10d1e176a6a8dd4420f1ea3a890640da0

Changed December 04, 2012 02:22PM UTC by Scott González comment:6

Revert "Dialog: Cleanup style properties on _destroy. Reenables style check in domEqual, while removing commented and unnecessary old code. Fixes #8119 - Dialog: Destroying a dialog leaves style, scrollleft, and scrolltop leftovers."

This reverts commit d687a1b10d1e176a6a8dd4420f1ea3a890640da0.

Changeset: 050e71bdd88708ce6e8462a89af4399cffa72cf3

Changed December 04, 2012 02:24PM UTC by scottgonzalez comment:7

resolution: fixed
status: closed → reopened

Changed December 04, 2012 02:31PM UTC by scottgonzalez comment:8

status: reopened → open
summary: Dialog: Destroying a dialog leaves style, scrollleft, and scrolltop leftovers → Dialog: Destroying a dialog leaves some styles changed

I think it's unreasonable to expect the scroll position to be set back.

Changed December 04, 2012 03:03PM UTC by Scott González comment:9

resolution: → fixed
status: open → closed

Dialog: Remove width, min-height, height styles on destroy. Fixes #8119 - Dialog: Destroying a dialog leaves some styles changed.

Changeset: 3c2acc322782cc08e575405f8123029342e33542

Changed December 05, 2012 04:53PM UTC by Scott González comment:10

Dialog: Restore inline styles for dimensions/display. Fixes #8119 - Dialog: Destroying a dialog leaves some styles changed.

Changeset: f59f5a8b12d50c87ba6e2fe47a1804a23535b3cf