Opened 9 years ago

Closed 9 years ago

#10705 closed defect (fixed)

Destroying dialogs does not run proper teardown procedures

Reported by: kcjonson Owned by: bill
Priority: high Milestone: 1.6
Component: Dijit Version: 1.4.0
Keywords: Cc: Karl Tiedt
Blocked By: Blocking:


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

Attachments (4)

dialog_teardown_bug.html (1.3 KB) - added by kcjonson 9 years ago.
Dialog DestroyRecursive? Bug Example
10705.diff (3.7 KB) - added by Karl Tiedt 9 years ago.
updated to fix focus issue
10705.2.html (1.4 KB) - added by ben hockey 9 years ago.
10705.html (1.4 KB) - added by ben hockey 9 years ago.

Download all attachments as: .zip

Change History (16)

Changed 9 years ago by kcjonson

Attachment: dialog_teardown_bug.html added

Dialog DestroyRecursive? Bug Example

comment:1 Changed 9 years ago by 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.

comment:2 Changed 9 years ago by bill

See also #9944.

comment:3 Changed 9 years ago by bill

Cc: Karl Tiedt added
Milestone: tbd1.6

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:

  1. base page, focus = ...
  2. dialog #1, focus = ...
  3. 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.

comment:4 Changed 9 years ago by 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?

comment:5 Changed 9 years ago by 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?

Changed 9 years ago by Karl Tiedt

Attachment: 10705.diff added

updated to fix focus issue

comment:6 Changed 9 years ago by Karl Tiedt

Owner: set to Karl Tiedt
Status: newassigned


I was overcomplicating my idea... check line 239 of the patch here:

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

comment:7 Changed 9 years ago by bill

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

comment:8 Changed 9 years ago by 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?

Changed 9 years ago by ben hockey

Attachment: 10705.2.html added

Changed 9 years ago by ben hockey

Attachment: 10705.html added

comment:9 Changed 9 years ago by 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.

comment:10 Changed 9 years ago by bill

Agreed, the race condition problem needs to be fixed.

comment:11 Changed 9 years ago by bill

Owner: changed from Karl Tiedt to bill
Status: assignednew

comment:12 Changed 9 years ago by bill

Resolution: fixed
Status: newclosed

(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.

Note: See TracTickets for help on using tickets.