Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#8119 closed bug (fixed)

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.

Change History (10)

comment:1 Changed 7 years ago by Scott González

Milestone: 1.9.01.10.0

comment:2 Changed 7 years ago by petersendidit

Status: newopen
Summary: Destroying a dialog leaves style, scrollleft, and scrolltop leftoversDialog: Destroying a dialog leaves style, scrollleft, and scrolltop leftovers

comment:3 Changed 7 years ago by Jörn Zaefferer

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?

Last edited 7 years ago by Jörn Zaefferer (previous) (diff)

comment:4 in reply to:  3 Changed 7 years ago by Scott González

Replying to 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.

comment:5 Changed 7 years ago by Jörn Zaefferer

Resolution: fixed
Status: openclosed

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

comment:6 Changed 7 years ago by Scott González

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

comment:7 Changed 7 years ago by Scott González

Resolution: fixed
Status: closedreopened

comment:8 Changed 7 years ago by Scott González

Status: reopenedopen
Summary: Dialog: Destroying a dialog leaves style, scrollleft, and scrolltop leftoversDialog: Destroying a dialog leaves some styles changed

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

comment:9 Changed 7 years ago by Scott González

Resolution: fixed
Status: openclosed

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

Changeset: 3c2acc322782cc08e575405f8123029342e33542

comment:10 Changed 7 years ago by Scott González

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

Changeset: f59f5a8b12d50c87ba6e2fe47a1804a23535b3cf

Note: See TracTickets for help on using tickets.