Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#12355 closed defect (worksforme)

_Container: addChild(widget, 0) not working as intended

Reported by: aedart@… Owned by: bill
Priority: low Milestone: 1.7
Component: Dijit Version: 1.6.0rc1
Keywords: dijit._Container, addChild Cc:
Blocked By: Blocking:

Description

when attempting to add a child widget at specific index, the dijit._Container.addChild method does not work as intended.

For instance, if I want to add a child widget at index 0 (zero), and given that there already have been added some children, then the specified widget will just be added as the last element of the container.

Another example, if you need to add a child somewhere between the first and last position, e.g. at index 12 out of 30, then the results will be the same as previously mentioned example.

In my custom widget, that takes advantage of dijit._Container, I attempted to create a correct version of the method, however, regardless of what I try, the child widgets always end up as the last element! Below is the source for my failed attempt to correct the defect:

		// summary:
		//		Makes the given widget a child of this widget.
		// description:
		//		Inserts specified child widget's dom node as a child of this widget's
		//		container node, and possibly does other processing (such as layout).
		
		var refNode = this.containerNode;
			
		if(insertIndex != undefined && typeof insertIndex == "number"){
			var children = this.getChildren();
						
			if(children.length > 0){
				// many children & insert at zero
				if(insertIndex == 0){
					insertIndex = "first";
				} else if(insertIndex >= children.length){
					// many children & insert last
					insertIndex = "last";
				} else {
					// many children & insert somewhere in between or 
					refNode = children[insertIndex-1].domNode;
					insertIndex = "after";
				}
			}
		}
		dojo.place(widget.domNode, refNode, insertIndex);
		
		// If I've been started but the child widget hasn't been started,
		// start it now.  Make sure to do this after widget has been
		// inserted into the DOM tree, so it can see that it's being controlled by me,
		// so it doesn't try to size itself.
		if(this._started && !widget._started){
			widget.startup();
		}

Note, the statement has to check if 'insertIndex is not undefined' or 0 (zero) will never be read (as in current version)!

Attachments (4)

DijitContainerTest.html (9.2 KB) - added by aedart@… 8 years ago.
Custom test cases for Dijit._Container addChild and removeChild
script.rar (46.2 KB) - added by aedart@… 8 years ago.
Custom widgets
_ContainerAddChild.patch (866 bytes) - added by bill 8 years ago.
fix to _Container::addChild() for when insertIndex == 0
_ContainerAddChildTextNodes.patch (734 bytes) - added by bill 8 years ago.
above patch will probably fail in container has spurious text nodes or comment nodes in it, in that case the DOMNode position # and widget position # are different

Download all attachments as: .zip

Change History (12)

comment:1 Changed 8 years ago by bill

Milestone: tbd1.7
Owner: set to bill
Status: newassigned
Summary: dijit._Container addChild(widget, index) not working as intended_Container: addChild(widget, index) not working as intended

Ah, well OK I see the same problem in addChild() that you mentioned in #12291 about removeChild(). I don't know any reason why inserting at index 12 wouldn't work though. Can you attach a test case? Looks like dijit/tests/_Container.html needs tests for addChild(), it doesn't have any.

Changed 8 years ago by aedart@…

Attachment: DijitContainerTest.html added

Custom test cases for Dijit._Container addChild and removeChild

comment:2 Changed 8 years ago by aedart@…

Hmmm... I produced a few test-cases for the _Container, however, I am unable to reproduce the errors! So now I am really confused as to what and where the defect is created!

That the defect are happening on my custom widgets, basically a container and a node (contained). Both of them take advantage of templates, ...etc, and are part of a rather deep package. Thus, "real" implementation of _Container and _Contained.

I will attempt to export them and include the test-cases for them, which do fail on the addChild. Nevertheless, I do not expect you to debug my script - that's my problem - but maybe you still want to take a look at them and see if there really is something inside the container that is not working as intended.

Changed 8 years ago by aedart@…

Attachment: script.rar added

Custom widgets

comment:3 Changed 8 years ago by aedart@…

The attached "script.rar" contains my custom widgets + some util classes. The test-cases for the widgets are placed at "dk.symbi.widget.test.module". Furthermore, if you wish to look at them, you might have to change some paths inside the *.html files, inside the test folder. As I mentioned earlier, they are part of a rather complex package.

I hope that this helps... an apologies if this proves to be a waste of your time (I still consider myself a noob messing with Dojo, Dijit Widgets and tests)

comment:4 Changed 8 years ago by bill

I see 4 test files in customModules/dk/symbi/widget/test/layout, I tried the first two:

  • DijitContainerTest - fails against 1.5 but works for me against trunk
  • ExpandoContainerTest - in the "Add node(s) at specific index" test, I patched dijit and will put the fix in 1.7, see attached patch file, but there's also a bug in your code when it calls
this._dndHandler.insertNodes(false, [widget]); 

... that moves the first element to be the last element.

BTW, your test files should have comments after each assert so you can tell which one is failing in the log. Ex:

doh.assertEqual(this.node1, this.container.getNodeAt(0), "node1");

Changed 8 years ago by bill

Attachment: _ContainerAddChild.patch added

fix to _Container::addChild() for when insertIndex == 0

Changed 8 years ago by bill

above patch will probably fail in container has spurious text nodes or comment nodes in it, in that case the DOMNode position # and widget position # are different

comment:5 Changed 8 years ago by aedart@…

Thx for the help. I'll update my version of Dojo to the trunk, and look at that bug you mentioned. Furthermore, ... I did not know about that D.O.H. accepted hints. Its not mentioned in on http://dojotoolkit.org/reference-guide/util/doh.html. But will start taking advantage of it from now on...

comment:6 Changed 8 years ago by bill

Summary: _Container: addChild(widget, index) not working as intended_Container: addChild(widget, 0) not working as intended

OK, and I'll check in a fix. Changing summary as it seems like the only issue is with addChild(child, 0).

comment:7 Changed 8 years ago by bill

Resolution: worksforme
Status: assignedclosed

Actually, it seems like the bug is just in your code, in the line I mentioned above. I'll check in some tests for posterity, but on tracing through this (again) in the debugger I don't see anything wrong with _Container.

comment:8 Changed 8 years ago by bill

(In [23985]) addChild() tests, refs #12355

Note: See TracTickets for help on using tickets.