Changeset 13521

Timestamp:

04/30/08 19:59:27 (7 months ago)

Author:

bill

Message:

Fix leak where destroyDescendants() wasn't working on layout widgets.

Problem was that getDescendants() wasn't finding the children (and grandchildren, etc) of widgets which didn't set containerNode, particularly layout widgets with no templates.

Fixes #6604.
!strict

Location:

dijit/trunk

Files:

• layout/ContentPane.js (modified) (5 diffs)
• layout/StackContainer.js (modified) (2 diffs)
• _Container.js (modified) (3 diffs)
• _Templated.js (modified) (1 diff)
• _Widget.js (modified) (2 diffs)

dijit/trunk/layout/ContentPane.js

@@ -133 +133 @@
                 // TODO: if there are two child widgets (a data store and a TabContainer, for example),
                 //      should still find the TabContainer
-                var childNodes = dojo.query(">", this.containerNode || this.domNode),
+                var childNodes = dojo.query(">", this.containerNode),
                         childWidgets = childNodes.filter("[widgetId]");
 
@@ -193 +193 @@
                         if(this._singleChild && this._singleChild.resize){
                                 this._singleChild.startup();
-                                this._singleChild.resize(this._contentBox || dojo.contentBox(this.containerNode || this.domNode));
+                                this._singleChild.resize(this._contentBox || dojo.contentBox(this.containerNode));
                         }
                 }
@@ -227 +227 @@
                 // But note that setting the margin box and then immediately querying dimensions may return
                 // inaccurate results, so try not to depend on it.
-                var node = this.containerNode || this.domNode,
+                var node = this.containerNode,
                         mb = dojo.mixin(dojo.marginBox(node), size||{});
 
@@ -344 +344 @@
 
         _setContent: function(cont){
+                // first get rid of child widgets
                 this.destroyDescendants();
 
                 try{
-                        var node = this.containerNode || this.domNode;
+                        // ... and then get rid of child dom nodes
+                        var node = this.containerNode;
                         while(node.firstChild){
                                 dojo._destroyElement(node.firstChild);
@@ -395 +397 @@
         _createSubWidgets: function(){
                 // summary: scan my contents and create subwidgets
-                var rootNode = this.containerNode || this.domNode;
                 try{
-                        dojo.parser.parse(rootNode, true);
+                        dojo.parser.parse(this.containerNode, true);
                 }catch(e){
                         this._onError('Content', e, "Couldn't create widgets in "+this.id

dijit/trunk/layout/StackContainer.js

@@ -36 +36 @@
 =====*/
         postCreate: function(){
-                dijit.setWaiRole((this.containerNode || this.domNode), "tabpanel");
+                dijit.setWaiRole(this.containerNode, "tabpanel");
                 this.connect(this.domNode, "onkeypress", this._onKeyPress);
         },
@@ -348 +348 @@
                         newButton.focusNode.setAttribute("tabIndex", "0");
                         var container = dijit.byId(this.containerId);
-                        dijit.setWaiState(container.containerNode || container.domNode, "labelledby", newButton.id);
+                        dijit.setWaiState(container.containerNode, "labelledby", newButton.id);
                 },
 

dijit/trunk/_Container.js

@@ -57 +57 @@
         {
                 // summary:
-                //              Mixin for widgets that contain a list of children.
+                //              Mixin for widgets that contain a set of widget children.
                 // description:
-                //              Use this mixin when the widget needs to know about and
-                //              keep track of it's widget children. Widgets like SplitContainer
-                //              and TabContainer.  
-
+                //              Use this mixin for widgets that needs to know about and
+                //              keep track of their widget children. Suitable for widgets like BorderContainer
+                //              and TabContainer which contain (only) a set of child widgets.
+                //
+                //              It's not suitable for widgets like ContentPane
+                //              which contains mixed HTML (plain DOM nodes in addition to widgets),
+                //              and where contained widgets are not necessarily directly below
+                //              this.containerNode.   In that case calls like addChild(node, position)
+                //              wouldn't make sense.
+
+                // isContainer: Boolean
+                //              Just a flag indicating that this widget descends from dijit._Container
                 isContainer: true,
 
+                buildRendering: function(){
+                        this.inherited(arguments);
+                        if(!this.containerNode){
+                                // all widgets with descendants must set containerNode
+                                this.containerNode = this.domNode;
+                        }
+                },
+
                 addChild: function(/*Widget*/ widget, /*int?*/ insertIndex){
                         // summary:
-                        //              Process the given child widget, inserting it's dom node as
-                        //              a child of our dom node
+                        //              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 otherprocessing (such as layout).
 
                         if(insertIndex === undefined){
                                 insertIndex = "last";
                         }
-                        var refNode = this.containerNode || this.domNode;
+                        var refNode = this.containerNode;
                         if(insertIndex && typeof insertIndex == "number"){
                                 var children = dojo.query("> [widgetid]", refNode);
@@ -94 +112 @@
                         // summary:
                         //              Removes the passed widget instance from this widget but does
-                        //              not destroy it
+                        //              not destroy it.
                         var node = widget.domNode;
                         node.parentNode.removeChild(node);      // detach but don't destroy
@@ -116 +134 @@
                 getChildren: function(){
                         // summary:
-                        //              Returns array of children widgets
-                        return dojo.query("> [widgetId]", this.containerNode || this.domNode).map(dijit.byNode); // Array
+                        //              Returns array of children widgets.
+                        // description:
+                        //              Returns the widgets that are directly under this.containerNode.
+                        return dojo.query("> [widgetId]", this.containerNode).map(dijit.byNode); // Widget[]
                 },
 
                 hasChildren: function(){
                         // summary:
-                        //              Returns true if widget has children
-                        var cn = this.containerNode || this.domNode;
-                        return !!this._firstElement(cn); // Boolean
+                        //              Returns true if widget has children, i.e. if this.containerNode contains something.
+                        return !!this._firstElement(this.containerNode); // Boolean
                 },
 

dijit/trunk/_Templated.js

@@ -30 +30 @@
                 //              declared in markup inside it? false by default.
                 widgetsInTemplate: false,
-
-                // containerNode: DomNode
-                //              holds child elements. "containerNode" is generally set via a
-                //              dojoAttachPoint assignment and it designates where children of
-                //              the src dom node will be placed
-                containerNode: null,
 
                 // skipNodeCache: Boolean

dijit/trunk/_Widget.js

@@ -47 +47 @@
 
         // domNode: DomNode
-        //              this is our visible representation of the widget! Other DOM
+        //              This is our visible representation of the widget! Other DOM
         //              Nodes may by assigned to other properties, usually through the
-        //              template system's dojoAttachPonit syntax, but the domNode
+        //              template system's dojoAttachPoint syntax, but the domNode
         //              property is the canonical "top level" node in widget UI.
         domNode: null,
+
+        // containerNode: DomNode
+        //              Designates where children of the source dom node will be placed.
+        //              "Children" in this case refers to both dom nodes and widgets.
+        //              For example, for myWidget:
+        //
+        //              |       <div dojoType=myWidget>
+        //              |               <b> here's a plain dom node
+        //              |               <span dojoType=subWidget>and a widget</span>
+        //              |               <i> and another plain dom node </i>
+        //              |       </div>
+        //
+        //              containerNode would point to:
+        //
+        //              |               <b> here's a plain dom node
+        //              |               <span dojoType=subWidget>and a widget</span>
+        //              |               <i> and another plain dom node </i>
+        //
+        //              In templated widgets, "containerNode" is set via a
+        //              dojoAttachPoint assignment.
+        //
+        //              containerNode must be defined for any widget that accepts innerHTML
+        //              (like ContentPane or BorderContainer or even Button), and conversely
+        //              is null for widgets that don't, like TextBox.
+        containerNode: null,
 
         // attributeMap: Object
@@ -316 +341 @@
         getDescendants: function(){
                 // summary:
-                //      Returns all the widgets that contained by this, i.e., all widgets underneath this.containerNode.
+                //              Returns all the widgets that contained by this, i.e., all widgets underneath this.containerNode.
+                // description:
+                //              This method is designed to *not* return widgets that are, for example,
+                //              used as part of a template, but rather to just return widgets that are defined in the
+                //              original markup as descendants of this widget, for example w/this markup:
+                //
+                //              |       <div dojoType=myWidget>
+                //              |               <b> hello world </b>
+                //              |               <div>
+                //              |                       <span dojoType=subwidget>
+                //              |                               <span dojoType=subwidget2>how's it going?</span>
+                //              |                       </span>
+                //              |               </div>
+                //              |       </div>
+                //
+                //              getDescendants() will return subwidget, but not anything that's part of the template
+                //              of myWidget.
+
                 if(this.containerNode){
                         var list= dojo.query('[widgetId]', this.containerNode);