Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#12436 closed defect (fixed)

[patch][cla][dijit.Dialog] in 1.6: destroying dialog in `onHide` causes error

Reported by: Kenneth G. Franqueiro Owned by: Kenneth G. Franqueiro
Priority: high Milestone: 1.7
Component: Dijit Version: 1.6.0
Keywords: dialog Cc:
Blocked By: Blocking:

Description

In Dojo 1.6, dijit.Dialog's onHide event is triggered when the hide animation ends. This is generally awesome.

However, the current implementation causes an error in one of the more frequent use-cases I've seen: hooking destruction of the dialog to this event. It throws because the code path that executes onHide expects the _fadeOutDeferred instance member to still be set after onHide completes - however, when the widget is destroyed, the deferred is cancelled, and its canceller function deletes this instance member.

I've attached a patch that simply checks for existence of the member first.

For those who need to work around this in 1.6, it can be done simply by using setTimeout to defer destruction until after the offending code path has completed:

dlg.connect(dlg, 'onHide', function() {
    setTimeout(function() { dlg.destroyRecursive(); }, 0);
});

Attachments (3)

Dialog.diff (503 bytes) - added by Kenneth G. Franqueiro 8 years ago.
test12436.html (601 bytes) - added by Kenneth G. Franqueiro 8 years ago.
Simple isolated test case
Dialog.2.diff (649 bytes) - added by Kenneth G. Franqueiro 8 years ago.
Revised change, no need for condition; hook on Deferred resolution

Download all attachments as: .zip

Change History (7)

Changed 8 years ago by Kenneth G. Franqueiro

Attachment: Dialog.diff added

Changed 8 years ago by Kenneth G. Franqueiro

Attachment: test12436.html added

Simple isolated test case

comment:1 Changed 8 years ago by bill

Owner: changed from bill to Kenneth G. Franqueiro

Sure, I'm fine w/it if you add an automated test (not using robot, just close the dialog using hide() or something).

Note that I have it on my list to get rid of the JS animations in widgets (replacing them with CSS animations), not sure when that will be done (hopefully in 1.7) but that will change the landscape some. Still good to have the test checked in though.

Changed 8 years ago by Kenneth G. Franqueiro

Attachment: Dialog.2.diff added

Revised change, no need for condition; hook on Deferred resolution

comment:2 Changed 8 years ago by Kenneth G. Franqueiro

Resolution: fixed
Status: newclosed

(In [24031]) Dialog: don't fire onHide until after promise is resolved to avoid explosions if Dialog is destroyed within onHide. Added manual test to test_Dialog.html reflecting the issue. Fixes #12436 !strict

comment:3 Changed 8 years ago by Kenneth G. Franqueiro

Milestone: tbd1.7

comment:4 Changed 6 years ago by bill

#17088 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.