Opened 12 years ago

Closed 10 years ago

Last modified 10 years ago

#6759 closed enhancement (fixed)

Dialog: support multiple dialogs (one dialog launching another)

Reported by: lloydmurray@… Owned by: Karl Tiedt
Priority: high Milestone: 1.4
Component: Dijit Version: 1.1.0
Keywords: Dialog TextBox model keypress onkeypress 13 Cc: Sam Foster
Blocked By: Blocking:

Description

I'm trying to launch a dialog from a dialog. On the second dialog with a textbox, I'm having problems with keypresses. Under IE7, I can't type in that textbox at all, while in FF2, I can type, but can not hit back space. This seems to be new in dojo 1.1.x. (Tried in 1.1.0 and 1.1.1) This was working in previous versions of dojo.

<html>
    <head>
        <style type="text/css">
        /* <![CDATA[ */
            @import url("http://o.aolcdn.com/dojo/1.1.1/dojo/resources/dojo.css");
            @import url("http://o.aolcdn.com/dojo/1.1.1/dijit/themes/tundra/tundra.css");
        /* ]]> */
        </style>

        <script type="text/javascript">
        // <![CDATA[
            djConfig = { isDebug: false, parseOnLoad: true };
        // ]]>
        </script>
        <script type="text/javascript" src="http://o.aolcdn.com/dojo/1.1.1/dojo/dojo.xd.js"></script>
        <script type="text/javascript">
        // <![CDATA[
            dojo.require("dojo.parser");
            dojo.require("dijit.form.Button");
            dojo.require("dijit.form.Form");
            dojo.require("dijit.form.ValidationTextBox");
            dojo.require("dijit.Dialog");

            function openDialog1() {
                dijit.byId("test1").show();
            }
            function openDialog2() {
                dijit.byId("test2").show();
            }

            dojo.addOnLoad(function() {
                var foo1 = new dijit.Dialog({ title: "test1 dialog" },dojo.byId("test1"));
                foo1.startup();

                var foo2 = new dijit.Dialog({ title: "test2 dialog" },dojo.byId("test2"));
                foo2.startup();
            });
        // ]]>
        </script>

        <title>dialog test</title>
    </head>

    <body class="tundra">
        <td id="dialogButtonCell"><button id="dialogButton" dojoType="dijit.form.Button" type="button" onClick="openDialog1();">Open Dialog</button></td>
        <div id="test1">test content 1 - press button to open another dialog
           <input type="text" dojotype="dijit.form.ValidationTextBox" size="25" value="test1">
           <button id="dialogButton1" dojoType="dijit.form.Button" type="button" onClick="openDialog2();">Dialog2</button>
        </div>
        <div id="test2">test content 2 - try typing, and backspacing in FF and IE.
            <input type="text" dojotype="dijit.form.ValidationTextBox" size="25" value="test2">
        </div>
    </body>
</html>

Attachments (6)

dialog_test.html (2.1 KB) - added by guest 12 years ago.
Test of error condition
multipledialog.patch (1.7 KB) - added by Becky Gibson 12 years ago.
Dialog.js (17.1 KB) - added by Phil DeJarnett 11 years ago.
Updated Dialog.js that includes a manager for multiple dialogs.
Dialog.2.js (7.2 KB) - added by Avachan 10 years ago.
Dojo Dijit 1.3.1 File, keypress possible, multidialog possible
MultiDialog.patch (6.1 KB) - added by Karl Tiedt 10 years ago.
multiple Dialogs via a "dialog stack" in trunk
dialogs_14949.zip (9.7 KB) - added by Sam Foster 10 years ago.
[CLA] Runtime patch for 1.3's Dialog to add in changes associated with this ticket

Download all attachments as: .zip

Change History (41)

Changed 12 years ago by guest

Attachment: dialog_test.html added

Test of error condition

comment:1 Changed 12 years ago by guest

added by lloydmurray@…

comment:2 Changed 12 years ago by bill

Reporter: changed from guest to lloydmurray@…

We really don't support launching a Dialog from another Dialog. (I've seen bug reports about this before and written the same thing, but can't seem to find them now.) Not sure if that's related to your problem at all. Can you make a simpler test case that doesn't have the nested dialogs?

comment:3 Changed 12 years ago by guest

This log is about using dialogs from dialogs. I can't scale back the test any more then what's attached. We have been using dialogs on dialogs within our application (from dojo 0.3.x) since Dec 2007. We did some hacks with setting the Zindex appropriately. Can you give me some pointers on why this might not work? Are there any known work-arounds? thanks, Lloyd

comment:4 Changed 12 years ago by bill

Hmm, if the problem was IE6 my first suspect would be the iframe created as part of the DialogUnderlay (search for !BackgroundIFrame in Dialog.js). However, that's only there for IE6, not IE7 or FF.

Maybe it's got something to do with the fade-in of Dialog? You could try modifying Dialog.js to just display the dialog and it's underlay to see if the problem goes away...

comment:5 Changed 12 years ago by guest

I tried setting the duration to 0 in Dialog.js, and that didn't help. I then removed the fade-in/out code from Dialog, and that didn't help this problem. (It did make the Dialogs appear "faster"). Any other ideas? Maybe I'll diff Dialog.js from 1.1.0 to 1.0 and see what other changes took place. Think anything in the _onKey function could be doing this?

thanks again, Lloyd

comment:6 Changed 12 years ago by guest

If I comment out the dojo.stopEvent(evt) line in _onKey() in Dialog.js (line 309 v 1.1.0), that will fix this problem.

<code> this key is for the disabled document window if(evt.keyCode != dojo.keys.TAB){ allow tabbing into the dialog for a11y

dojo.stopEvent(evt);

opera won't tab to a div }else if(!dojo.isOpera){ <code>

This has the unfortunate side effect of allowing me to Tab to other dialogs. Any thoughts?

Lloyd

comment:7 Changed 12 years ago by guest

This has the unfortunate side effect of allowing me to Tab to other dialogs. Any thoughts?

...and it allows me to tab back to the base screen, so I wouldn't consider this a "fix".

comment:8 Changed 12 years ago by bill

That's a good discovery. (BTW you can quote code in this system with triple left curly brace to start, and right curly brace to end it. <code> is for drupal I think). As per:

Any thoughts?

My thought is... why is that stopEvent(evt) getting called when you type normal characters or backspace? It's only supposed to be called on the tab key.

comment:9 Changed 12 years ago by guest

That stop event is being called when the matching parent dialog is never found. (It's a != TAB, not an == TAB).

To fix this correctly on a dialog on a dialog, there should be a tracking means for which dialog is "active". Then, the connect/disconnects methods for the onKey could be called for the inactive dialogs. (This would ensure the _onKey is only ever called on the active dialog). For a quick fix, I added the following, right above that code I commented it, but after the while loop.

                 if (!node) {
                        return;
                    }
                    // this key is for the disabled document window
					if(evt.keyCode != dojo.keys.TAB){ // allow tabbing into the dialog for a11y
						dojo.stopEvent(evt);
                    // opera won't tab to a div

}

I'd test this through the browsers to make sure it works properly.  (I checked it on FF2 and IE7)  One more note.  This will only work if the firstFocusItem and lastFocusItem are correctly set.  Sometimes, it seems, these are set to odd dialog span, or other "un-tabbable" items, which will allow the you to Tab back to another dialog, or the main screen.

comment:10 Changed 12 years ago by bill

Component: DijitAccessibility
Owner: set to Becky Gibson

Ah I see. OK well sounds like you found a solution.

I'm not sure that the if(evt.charOrCode != dk.TAB){ block of code is even used anymore though, now that we have code to find the first focusable item in a Dialog (assuming there is one) and move to it. Passing this over to Becky because if that code isn't needed anymore then we should delete it.

If not, then I don't really want to put in code to track which Dialog is active, so might mark this as wontfix.

comment:11 Changed 12 years ago by guest

We'll, I found a solution that requires us to patch your code, then check when updates come. We'd like to get away from doing this in every release, that's why I posted this up there. With this solution, and a mechanism to automatically set the zindex on a dialog show, you'll be able to support multiple dialogs in dojo/dijit. This seems to be well wanted on the forums.

keep me updated. thanks! Lloyd

Changed 12 years ago by Becky Gibson

Attachment: multipledialog.patch added

comment:12 Changed 12 years ago by Becky Gibson

multipledialog.patch fixes the multiple dialog problem as well as an existing bug where the use can click off of the dialog and thus lose focus in the dialog. I added a trap of mousedown in the dialogunderlay and modified the dialog to only listen for keyevents within the dialog itself rather than the documentElement. It needs more testing to make sure I didn't cause any regressions.

comment:13 Changed 11 years ago by Becky Gibson

Milestone: 1.3

the simple fix will not work. multipledialog.patch still has issues on IE if the user clicks outside of the dialog. I am moving this to 1.3 since it won't make 1.2 due to the IE issues and further discussion about whether or not we should support launching a dialog from a dialog.

comment:14 Changed 11 years ago by Josh Trutwin

please add josh@… to CC

I'm not sure I understand the last response - how can this be both "wontfix" and milestone of 1.3 - does that mean this will be re-evaluated for inclusion in 1.3? I would like to voice my interest in having some solution to this problem as it has come up numerous times in my application. It's something I certainly try to avoid due to complicated UI (having multiple dialogs with form controls) but it does happen occasionally. Most recently a dialog with an edit company page, where an icon next to the email text field allows users to send them an email via a dialog with simple subject / message form fields. Gotta redo that though as I ran into this issue again...

Thanks, Josh

comment:15 Changed 11 years ago by Becky Gibson

Bill, isn't sure it should be fixed since dijit doesn't support launching a dialog from a dialog which is why he is inclined to mark it as wontfix. Thus, if it is to get fixed I have to convince bill and make the fix :-) so I marked it as 1.3 in order to take another look. Unfortunately your solution won't work in all cases and the solution I posted doesn't work in IE. Keeping focus within the dialog is tricky so this will take time to fix correctly.

comment:16 Changed 11 years ago by Becky Gibson

Resolution: wontfix
Status: newclosed

since Bill doesn't want to support launching a dialog from a dialog and I won't have the time to spend on this anytime soon I am closing this as wontfix. If someone wants to create a dialogManager and integrate it into dijit or to investigate my patch and make it work in IE, we can reconsider. If so, please open a new ticket and reference this one.

comment:17 Changed 11 years ago by Karl Tiedt

Cc: Sam Foster added
Component: AccessibilityDijit
Resolution: wontfix
Status: closedreopened

sfoster -- I added you to the cc list because you modified ContentPanes? to use the new html.setter classes

I think this is fixable now... with recent changes to ContentPane? (the use of setter.parseResults for example) I believe we can pull this off... I was able to fix an old Dialog (just shy of 1.0) to work perfectly in IE6+ and FF based on a custom widget... Basically in DIalogs _onKey just before going into the while(node) loop, I added a check based on dijit.getEnclosingWidget(node) and working back to the TooltipDialogs? dropDownButton that opened it and setting that to the value of node

Its a more specialized fix because my TooltipDialogs? have a .parent property that points back to their Dropdown Button but this could easily harness the new power of ContentPanes? to make Dialogs work in this manner again.

Here is the onKey I used with comments for where I updated it

		_onKey: function(/*Event*/ evt){
			// summary: handles the keyboard events for accessibility reasons
			if(evt.keyCode){
				var node = evt.target;
				// see if we are shift-tabbing from titleBar
				if(node == this.titleBar && evt.shiftKey && evt.keyCode == dojo.keys.TAB){
					if(this._lastFocusItem){
						this._lastFocusItem.focus(); // send focus to last item in dialog if known
					}
					dojo.stopEvent(evt);
				}else{
					//see if we belong to a TooltipDialog whose button is inside dojox.Dialog
					var widget = dijit.getEnclosingWidget(node);
					if(widget && widget.declaredClass == "dijit.TooltipDialog"){
						//for this usecase all TooltipDialogs are assigned a parent property that points them to the dropDownButton
						//reassign node as the domNode of the dropDownButton that opens the TooltipDialog
						node = widget.parent.domNode;
					}
					// see if the key is for the dialog
					while(node){
						if(node == this.domNode){
							if(evt.keyCode == dojo.keys.ESCAPE){
								this.hide(); 
							}else{
								return; // just let it go
							}
						}
						node = node.parentNode;
					}
					// this key is for the disabled document window
					if(evt.keyCode != dojo.keys.TAB){ // allow tabbing into the dialog for a11y
						dojo.stopEvent(evt);
					// opera won't tab to a div
					}else if (!dojo.isOpera){
						try{
							this.titleBar.focus();
						}catch(e){/*squelch*/}
					}
				}
			}
		},

comments?

note: this change still allowed TAB to behave normally, no tabbing out of the Dialog occurred backspace works for input fields and all around despite the getEnclosingWidget call onKey press, input was not laggy at all.

comment:18 Changed 11 years ago by bill

Owner: changed from Becky Gibson to Dustin Machi
Status: reopenednew
Summary: keypress problems in a TextBox on a dialog, launched from a dialogDialog: keypress problems in a TextBox on a dialog, launched from a dialog

Dustin is working on a Dialog manager that would allow nested dialogs, so assigning to him for now.

comment:19 Changed 11 years ago by bill

Milestone: 1.31.4

bumping 1.4 tickets to 1.5, and most 1.3 tickets to 1.4

comment:20 Changed 11 years ago by Dustin Machi

Milestone: 1.4tbd

comment:21 Changed 11 years ago by bill

Dustin, please set a milestone on this, or future if you don't want to commit to a date. I don't want to leave dijit bugs as TBD.

comment:22 Changed 11 years ago by bill

Milestone: tbd1.4

Changed 11 years ago by Phil DeJarnett

Attachment: Dialog.js added

Updated Dialog.js that includes a manager for multiple dialogs.

comment:23 Changed 11 years ago by Phil DeJarnett

I just attached a modified version of Dialog.js (I'm sorry, I don't know how to attach just a patch). I've taken changes I'm currently using in my own code and applied them to the source of Dialog.js.

The changes, together, should allow for multiple stacked dialogs. I combined the information from above to achieve the following:

There is a base zIndex of 950 (to be below menus used in ComboBoxes?) for every DialogUnderlay?, and 951 for Dialogs.

Whenever a Dialog is shown, it is tracked in dijit.Dialog._manager. The first dialog simply updates the base z-index, and stores inside the _manager as last. The following dialogs push "last" onto a stack, and update their own z-index to be above (including the underlay).

Whenever a Dialog is set to "last", the _onKey method runs normally. Otherwise it returns before any code is run.

When a Dialog is hidden, the stack is popped, and the previous dialog (if any) gets set to "last". This allows the previous dialog to resume tracking _onKey.

There are no additional listeners, and everything is handled through two variables, last and stack. (I just realized, however, that some code probably should be added to destroy() to ensure that _manager isn't keeping a copy of a dialog in memory that was destroyed before being hidden.)

Hopefully this helps.

comment:24 Changed 10 years ago by Karl Tiedt

Has there been any progress on a current patch that uses the single master overlay? I'm tinkering with it, but current attempts arent working out as well as planned.... the attached dialog.js file from above is prior to the master underlay method that is currently in use.

comment:25 Changed 10 years ago by Karl Tiedt

Here is a new patch from current trunk that in my tests works (test patch included)

Comments/review? and viewable http://ktiedt.dojotoolkit.org/dojo/dijit/tests/test_Dialog.html

Changed 10 years ago by Avachan

Attachment: Dialog.2.js added

Dojo Dijit 1.3.1 File, keypress possible, multidialog possible

Changed 10 years ago by Karl Tiedt

Attachment: MultiDialog.patch added

multiple Dialogs via a "dialog stack" in trunk

comment:26 Changed 10 years ago by Karl Tiedt

Owner: changed from Dustin Machi to Karl Tiedt
Status: newassigned

Updated with latest suggestions from Bill.... Now hide() checks if _savedFocus is inside the next Dialog and if so, uses it, otherwise will focus the Dialogs domNode, will also restore dijit.underlay's attr's for each dialog so bg's carry across stacking.

Avachan: your attachment is garbled, please make a unified diff/patch file of an uncompressed Dialog.js....

or if you'd like, test out my patch as its on the verge of being 'ok' for Dijit...

comment:27 Changed 10 years ago by Avachan

Ive only changed it in the compressed File. Sry, next time I change the uncompressed and make a patch.

Sincerly Ava-chan

comment:28 Changed 10 years ago by bill

Summary: Dialog: keypress problems in a TextBox on a dialog, launched from a dialogDialog: support multiple dialogs (one dialog launching another)
Type: defectenhancement

comment:29 Changed 10 years ago by liucougar

Resolution: fixed
Status: assignedclosed

in [18870]: Multiple dialog support (one dialog displaying on top of another). Patch from Karl (thanks!) w/some modifications from me, plus regression test.

Fixes #6759 !strict.

comment:30 Changed 10 years ago by liucougar

(In [18891]) refs #6759: when hiding a dialog, don't pop the self underlay info from dijit._dialogStack until fadeOut animation ends (this is to ensure that if the fadeout animation is synchronous, the underlay still hides properly) this commit also fixes a strict warning

comment:31 Changed 10 years ago by liucougar

(In [18903]) refs #6759: fixed regression introduced in [18891]

race condition: before a dialog fadeout animation finishes, if show() is called on the dialog, the underlay won't be hidden when hide() is called afterwards

Changed 10 years ago by Sam Foster

Attachment: dialogs_14949.zip added

[CLA] Runtime patch for 1.3's Dialog to add in changes associated with this ticket

comment:32 Changed 10 years ago by Aleksey Rechinskiy

Have just fallen into nearly the same issue with D1.3.2 trying to create second dialog right after closing the first.

It looks like the fix has been made. May I ask, when it will be merged into next stable branch and when it will be released?

Thanks.

comment:33 Changed 10 years ago by bill

It's it trunk. Trunk is the basis of 1.4. 1.4 will be released.... well in theory in 4 months (see http://bugs.dojotoolkit.org/roadmap) but we haven't nailed down a date yet.

comment:34 Changed 10 years ago by Aleksey Rechinskiy

Bill, thanks for reply.

Ok... I'll try to find a workaround for this issue at first. I don't feel like it will be easy extract all necessary Dialog's changes from the trunk to merge it into 1.3.2

comment:35 Changed 10 years ago by dante

Keywords: 13 added

tagging 1.3.x candidate as requested by seanosea

Note: See TracTickets for help on using tickets.