Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#6954 closed defect (fixed)

destroyRecursive() and getDescendants() architectural issues

Reported by: bill Owned by: bill
Priority: high Milestone: 1.3
Component: Dijit Version: 1.1.1
Keywords: Cc:
Blocked By: Blocking:

Description

There are a number of architectural issues with getDescendants(), and consequently destroyDescendants() and destroyRecursive().

  1. getDescendants() doesn't follow links to drop downs since their DOM nodes are attached to <body> (see #6948). Thus those widgets are not destroyed.
  2. getDescendants() returns widgets that are declared as part of a template, etc. like the buttons in InlineEditBox even though those are supposed to be hidden. Thus destroyRecursive() will destroy said widgets twice.
  3. API comment for getDescendants() is incorrect; it suggest that it doesn't find widgets in the container of other widgets (specifically subwidget2 in the example)

All of these issues stem from using dojo.query() to find descendant widgets, rather than searching the DOM tree manually... might make sense to implement getChildren() for non _Container widgets (like ContentPane?) to function as it did in 0.4, finding all widgets who's closest ancestor is "this" (even if child.domNode is not a direct child of parent.containerNode).

Change History (10)

comment:1 Changed 11 years ago by bill

(In [13988]) For _Container based widgets, make destroyRecursive() work hierarchically, thus avoiding use of getDescendants() which has various issues (see tickets for details). Fixes #6948, refs #6954. !strict

comment:2 Changed 11 years ago by bill

Milestone: 1.4future

testing bulk modify

comment:3 Changed 11 years ago by bill

See related tickets #2056, #4980, #7706.

comment:4 Changed 11 years ago by ben hockey

i'm using this post as a bit of a placeholder for some work that i haven't yet completed. below is a work in progress of a possible change to destroyDescendants

        if (this.containerNode) {
            var list = dojo.query('[widgetId]', this.containerNode);
            list.filter(dojo.hitch(this,function(node){
                return (dijit.getEnclosingWidget(node.parentNode).containerNode == this.containerNode);
            })).map(function(node){
                    return dijit.byNode(node).destroyRecursive(preserveDom);
            });
        }

summary: if we have a containerNode then it will create a list of all nodes with a widgetId tag that are children of our containerNode. then we will filter out all the nodes which do not meet this criteria "dijit.getEnclosingWidget(node.parentNode).containerNode == this.containerNode". this means that the list is then left with the domNodes of widgets who's nearest parent widget is the widget with our containerNode. then for each of those widgets represented by the nodes left in the list, we are going to call destroyRecursive(). this should then take care of destroying their children and them through destroyDescendants() and destroy() respectively.

caveat: this works iff each widget destroys it's own children (sub-widgets, widgets in templates or whatever else) through it's destroy() method (if it does not have a containerNode) or through the destroyDescendants() method (if it has a containerNode). a current example of a widget that does not do this is the toolbar - it does not destroy the buttons it knows about through it's own destroy() method. if it is wrong to expect each widget to destroy it's own children then the code above will not work because it is based on the assumption that each widget would take care of it's own children through either destroyDescendants() or destroy().

note: the code above offers an improvement on the current code given that the current destroyDescendants() does not respect any subclassed destroyDescendants() of any child widgets but the code above will take that into account. this may not be necessary but it is possibly worth mentioning for consideration.

#7784 is possibly somewhat dependent on this getting fixed so i've mentioned it for reference.

comment:5 in reply to:  4 Changed 11 years ago by ben hockey

Replying to neonstalwart:

caveat: this works iff each widget destroys it's own children (sub-widgets, widgets in templates or whatever else) through it's destroy() method (if it does not have a containerNode) or through the destroyDescendants() method (if it has a containerNode). a current example of a widget that does not do this is the toolbar - it does not destroy the buttons it knows about through it's own destroy() method. if it is wrong to expect each widget to destroy it's own children then the code above will not work because it is based on the assumption that each widget would take care of it's own children through either destroyDescendants() or destroy().

#7968 relates to a memory leak for toolbar. i have suggested a fix for that.

comment:6 in reply to:  4 Changed 11 years ago by ben hockey

Replying to neonstalwart:

>         if (this.containerNode) {
>             var list = dojo.query('[widgetId]', this.containerNode);
>             list.filter(dojo.hitch(this,function(node){
>                 return (dijit.getEnclosingWidget(node.parentNode).containerNode == this.containerNode);
>             })).map(function(node){
>                     return dijit.byNode(node).destroyRecursive(preserveDom);
>             });
>         }

after some thought, it probably makes sense that most of this is inside getDescendants. this should fix points 2 and 3 in the original post above. so getDescendants would become:

    if(this.containerNode){
        var list = dojo.query('[widgetId]', this.containerNode);
        return list.filter(dojo.hitch(this, function(node){
                return (dijit.getEnclosingWidget(node.parentNode).containerNode == this.containerNode);
            })).map(dijit.byNode);		// Array
    }else{
    return [];
}

and then destroyDescendants would need a small change to call destroyRecursive on each widget in the list since now we only have the nearest-level widget children in the list:

dojo.forEach(this.getDescendants(), function(widget){ 
    if(widget.destroyRecursive){
        widget.destroyRecursive(preserveDom);
    }
});

comment:7 Changed 11 years ago by bill

Milestone: future1.4

Hi Neonstalwart,

Thanks for the suggestion. I see what you are getting at. Putting the above code in _Widget.destroyDescendants() would solve some double-delete issues; it's the same strategy that _Container.destroyDescendants() uses.

Putting that code inside getDescendants() might be a good idea although it changes the behavior (some would say "breaks the API"). getDescendants() would no longer return grandchildren. I'm talking about the simple case, nothing with widgetsInTemplate, etc. For example:

<div dojoType="dijit.layout.ContentPane">
     <div dojoType="dijit.layout.TitlePane">
           <button dojoType=...>
     </div>
</div>

However, considering how broken things currently are it might make sense anyway.

comment:8 Changed 11 years ago by bill

Milestone: 1.41.3
Status: newassigned

comment:9 Changed 11 years ago by bill

Resolution: fixed
Status: assignedclosed

Fixed in [15607]: Fix destroyDescendants() to avoid problems with double-deletes of widgets in templates.

Added flag to getDescendants() to only get direct descendants, not nested descendants. getDescendants(true) is similar to getChildren() except that the DOM nodes of the descendant widgets don't need to be direct children of this.containerNode, so it's suitable for free-form contents like ContentPane?.

Fixes #6954, #7550. Refs #7706, #7784, #7823. !strict

comment:10 Changed 11 years ago by bill

(In [16904]) 1. Refactor [15607] (refs #6954, #7550, #7706, #7784, #7823) so that getDescendants() works as in 1.2 again, returning all descendant widgets, even nested widgets that are defined in templates. Expose the new functionality from [15607], to find direct descendants only, into a new _Widget.getChildren() method, rather than as flag to getDescendants().

This is because:

  • [15607] gave getDescendants() a subtle API change: it no longer found widgets inside of a dijit.Declaration (since no containerNode was defined)
  • because of that, Form was broken in that it didn't find form widgets inside of a custom widgets, declared with dijit.Declaration or dojo.declare, which didn't define this.containerNode
  1. Also, rolling back #7819: ContentPane?: missing an addChild() method (refs #7819), because of the backwards compatibility issue for custom widgets that extend ContentPane? and also extend _Container/_Contained.

Summary:

  • getChildren() is now supported by all widgets, and _Container widgets have (as before) an optimized implementation of getChildren() that overrides the one in _Widget.
  • ContentPane? not supporting any _Container methods except getChildren(). Users can set a ContentPane? child by calling attr('value', ...) etc.

!strict

Note: See TracTickets for help on using tickets.