Opened 10 years ago

Closed 9 years ago

#9944 closed defect (fixed)

Dialog: hide() of dialogs out of order

Reported by: Dustin Machi Owned by: bill
Priority: high Milestone: 1.6
Component: Dijit Version: 1.3.2
Keywords: Cc: Karl Tiedt
Blocked By: Blocking:

Description

Changes to the dialog to allow multiple dialog to show simultaneously contain a parameter to be passed to the hide() function, "ingoreStackPos". Currently this function does nothing, but presumably it was there to get around the issue of allowing some code to close a dialog that isn't necessarily the active dialog.

Changing:

if(!this._alreadyInitialized
this != ds[ds.length-1]){

to

if(!ignoreStackPos && (!this._alreadyInitialized
this != ds[ds.length-1])){

and modifying the call to pop() the top item of the stack:

Change: dijit._dialogStack.pop();

to: if(ignoreStackPos){

var ds = dijit._dialogStack; var positionInStack = dojo.indexOf(ds, this); if(positionInStack >= 0){

ds.splice(positionInStack, 1);

}

}else{

dijit._dialogStack.pop();

}

should address this. Not sure if this was actually the intent of ignoreStackPos, so I wanted to verify before applying these changes.

Dustin

Change History (10)

comment:1 Changed 10 years ago by bill

Cc: Karl Tiedt added
Summary: Dialog hide ignoreStackPosDialog: hide()'s ignoreStackPos ignored

Karl, that flag is your creation, right? Even assuming that we want the feature to close dialogs out of order, I'm not sure why we need a flag for it? Couldn't we just assume that flag is true all the time?

BTW Dustin, could use the findLast parameter of indexOf() to start searching from the end of the list/top of the stack (under the assumption that most of the time the dialog being closed will be the most recent), although I guess the app design would be pretty weird to have so many dialogs simultaneously shown that that would make a difference.

comment:2 Changed 10 years ago by Karl Tiedt

Actually I've never seen that flag (I havent touched trunk much since Dialog was updated -- waiting on other fixes to land before updating our repo at work).

I guess the idea makes a little sense... a previous dialog could be invalidated by something in the most recent... but... definitely not my idea :P I kept it extremely basic and simple! :)

comment:3 Changed 10 years ago by bill

Karl - the flag is from your code in [18870].

Anyway, let's get rid of that flag and make hide() work regardless of the dialog position.

comment:4 Changed 10 years ago by Karl Tiedt

Owner: set to Karl Tiedt
Status: newassigned

wow it is, dont remember something like that at all, I'll write up a patch today or this weekend hopefully, not that its really a big patch, just busy :)

comment:5 Changed 10 years ago by Dustin Machi

I assumed it was necessary for some reason (maybe keyboard handling?), but if it isn't necessary at all, then removing it is fine with me.

comment:6 Changed 10 years ago by bill

Summary: Dialog: hide()'s ignoreStackPos ignoredDialog: hide() of dialogs out of order

Note that code like:

dojo.style(dijit._underlay.domNode, 'zIndex', 948 + dijit._dialogStack.length*2);

would also need to be changed or otherwise new dialogs might appear underneath old dialogs, rather than on top of them, in a situation like:

  1. open dialog 1 (z-index=948)
  2. open dialog 2 (z-index = 950)
  3. open dialog 3 ( z-index = 952)
  4. close dialog1, dialog2
  5. open dialog 4 (z-index = 950)

If we support this I want to have a test case for it.

comment:7 Changed 10 years ago by bill

(In [20373]) Remove hide()'s ignoreStackPos parameter. Refs #9944 !strict.

comment:8 Changed 10 years ago by bill

Milestone: 1.41.5
Owner: changed from Karl Tiedt to bill
Status: assignednew

I'll do this for 1.5.

comment:9 Changed 10 years ago by bill

Milestone: 1.51.6

See also #10705.

comment:10 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.