Destroying dialogs does not run proper teardown procedures

[kcjonson]

Expected: Calling .destroyRecursive on a dijit.dialog removes the dialog from the dijit_dialogStack array and properly re-calculates the z-index of the dialog underlay.

Found: Calling .destroyRecursive on a dijit.dialog instance does not remove it from the dijit._dialogStack array and reposition the underlay properly. This generally only becomes visually apparent when multiple dialogs are present. Calling .destroyRecursive on the top dialog leaves the dialogUnderlay hidden and z-indexed above the bottom dialog. Triggering the layout logic on the page by scrolling or resizing the browser window causes the underlay to become visible but not lower its position (its z-index) and renders the page un-usable.

The proper teardown procedures live in the dialog._fadeOut.onEnd function which is not accessible in publicly to override or extend. This is complicated by dialog.onHide runs before dialog._fadeOut.onEnd

Proposed Solution: Run dialog.onHide only after dialog._fadeOut.onEnd has completed its interval and move the teardown functionality into the publicly available onHide function

[kcjonson]

Dialog DestroyRecursive? Bug Example

[Karl Tiedt]

Here is a possible patch that may fix this issue... just checked if trac was up before I went to bed... so I havent tested it against the attached test case.

[bill]

See also #9944.

[bill]

Hi Karl, the patch definitely looks like it's going in the right direction. Thanks for working on it. This is overlapping with #9944 in that you are trying to handle out-of-order dialog removal too, and that's a tricky thing.

In your patch I think there's a problem with refocusing, in that the "previous focus" is stored in the Dialog object itself, so if you delete a dialog out of the middle of the stack you are losing vital information. The worst case is if you delete the first dialog in the stack, in which case you lose where focus was before the first dialog popped up.

So I think probably the stack needs to be something like:

0. base page, focus = ...
1. dialog #1, focus = ...
2. dialog #2, focus = ...

So that any element from the stack (except position 0) can be deleted without incident.

The other thing is that this needs some unit tests. Not robot tests but just straight DOH tests that popup three dialogs, destroy() or hide() them out of order and make sure focus, underlay z-index, underlay visibility, etc. all work correctly. Too complicated to add in otherwise.

[Karl Tiedt]

you are right Bill, that would become a problem... maybe we should turn the dialogStack into an object array that tracks all the needed values?

[bill]

I think so. I think each stack element is just those two things, the Dialog and the focused element in that dialog, right? (Plus need to remember the focused element on the base page itself). Is there anything else?

[Karl Tiedt]

updated to fix focus issue

[Karl Tiedt]

Bill,

I was overcomplicating my idea... check line 239 of the patch here: ​http://bugs.dojotoolkit.org/attachment/ticket/10705/10705.diff

I haven't tested it since I brought it up to trunk changes, but if you think the idea is sound I'll finish it up by testing it and commit

[bill]

I guess that will work, but please don't commit it w/out automated tests.

[ben hockey]

i added a test case which fails intermittently. it seems to fail more often when the lateness is low so maybe the last setTimeout of the animation is happening after the setTimeout for the destroyRecursive?

[ben hockey]

grr... ignore 10705.2.html - i meant to replace 10705.html

i've found that if i increase the setTimeout to be dialog2.duration + 100 then it seems to work. i think it's just a timing issue which would be resolved if we get a callback from the end of the animation rather than trying to get a setTimeout to line up with the end of the animation.

[bill]

Agreed, the race condition problem needs to be fixed.

[bill]

(In [22662]) Refactoring Dialog to handle when multiple dialogs are displayed and a non-top-level dialog is hidden or destroyed. The salient change is that rather than a Dialog saving the previous focus, there's a "level manager" that remembers each active layer (starting with the base page itself) and the focus in that layer.

Also fixes race conditions on concurrent dialog show/hide.

Previously Dialog._onKey was connected to the document body. I couldn't find a case when that was necessary (as opposed to connecting to Dialog.domNode), so I changed it to attach to Dialog.domNode, so Dialog wouldn't have to check "am I the top dialog on the stack?" in that method.

Fixes #2238, #9944, #10705 !strict.