Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#8015 closed enhancement (fixed)

_LayoutWidget/_Templated(widgetsInTemplate) mixin bugs

Reported by: Neil Roberts Owned by: bill
Priority: high Milestone: 1.4
Component: Dijit Version: 1.2.0
Keywords: Cc:
Blocked By: Blocking:

Description

Extending from _LayoutWidget, with a _Templated mixin (using widgetsInTemplate) causes several bugs, which need not happen.

Two things create these bugs:

  1. The buildRendering function of the dijit._Container mixin (which is part of _LayoutWidget) gets overridden by _Templated.
  2. _LayoutWidget relies on the proper functioning of dijit._Contained's getParent method in order to determine whether or not the startup function should be called (which calls the resize function). In order for the layout system to really do its thing, the container should be the one calling the resize function, not the widget itself.

Here's why:

  1. _LayoutWidget calls getChildren during startup. getChildren expects containerNode to be set, which usually happens during buildRendering. Since buildRendering was overridden, this wasn't set.
  2. When widgetsInTemplate is set to true, the parser creates instances of al the specified widgets. The parser checks to see if it should call the startup function by using the getParent function. The getParent function checks to see if widgetId is set on the parent's domNode. Since widgetId isn't set until after buildRendering, this is always false, and the startup function gets called.

To fix it:

  1. In the startup function, assign containerNode if it hasn't already been set.
  2. In _Templated, assign widgetId before we run the parser.

I've added a patch with these changes

Attachments (1)

layout-alternative.diff (1.3 KB) - added by Neil Roberts 11 years ago.
The _Templated change, but adding _TemplatedLayoutWidget

Download all attachments as: .zip

Change History (8)

comment:1 Changed 11 years ago by Neil Roberts

Bill indicated that getChildren is uniquely designed to only give the children as they were defined upon declaration. It's therefore assumed that the children came from declaration either through an explicit containerNode assignment in a template or directly below the DOM node, which is why the reassignment of domNode to containerNode in buildRendering is there.

With this knowledge, I think the better proposal is to create an alternative to the _LayoutWidget mixin, _TemplatedLayoutWidget, so handle the situation where the children may come from a template. Updated with layout-alternative.diff

Changed 11 years ago by Neil Roberts

Attachment: layout-alternative.diff added

The _Templated change, but adding _TemplatedLayoutWidget

comment:2 Changed 11 years ago by Neil Roberts

Bill also points out that destroyDescendents needs some fixing, so the _TemplatedLayoutWidget solution seems to be the way to go

comment:3 Changed 11 years ago by bill

This is a delicate area as shown by #5865 and #6604, so I want to be careful about changing anything or guaranteeing anything about widgetsInTemplates behavior.

The way I intended getChildren() to work is that if a user declares a widget like:

<div dojoType=SomeWidget jsId=myInstance></div>

and then calls

myInstance.getChildren()

It's supposed to return a zero length array, regardless of the template of SomeWidget? is, or how many widgets SomeWidget? has created internally (through widgetsInTemplates or other means). That's because from the widget *user's* perspective there are no child widgets.

You refer to the code in _Container that defaults containerNode to the root domNode:

if(!this.containerNode){
	// all widgets with descendants must set containerNode
	this.containerNode = this.domNode;
}

That was put there for backwards-compatibility reasons (see #5865 and #6604), to support custom widgets that don't set containerNode explicitly. But the idea going forward is that all widgets, iff they have children, should define containerNode explicitly, and that's always been the rule for templated widgets.

You can actually make getChildren() do what you want by adding dojoAttachPoint="containerNode" into your template on the outermost node. Usually the <div> with dojoAttachPoint=containerNode is empty, but it doesn't need to be. Of course, that wouldn't solve the double-destroy problem w/_supportingWidgets, and not sure if there would be other lurking issues.

As for the problem where widgetId gets assigned too late (it gets set in create() after buildRendering() is finished), I guess the change to also set it in _Templated.buildRendering() makes sense.

comment:4 Changed 11 years ago by bill

PS: Probably a better example than the one I gave above is TabContainer. That's a templated layout widget with 2n+1 children under the domNode, typically

  • n ContentPanes
  • 1 TabController
  • n TabButtons

getChildren() on TabContainer(), and also destroyDescendants(), are just meant to return/destroy the ContentPanes.

I realize the above patch respects the value of containerNode if it's set, so this wouldn't cause a problem, but anyway that example demonstrates the way getChildren() and related functions are intended to work.

comment:5 Changed 11 years ago by bill

Milestone: tbd1.5

comment:6 Changed 10 years ago by bill

Milestone: 1.51.4
Resolution: fixed
Status: newclosed

I think this is fixed by #9477 and #9657.

startup() on the embedded widgets is called when the main widget's startup() is called, so the embedded widgets will detect that the main widget is their layout parent, and they will wait for a resize() call.

getChildren() won't return the embedded widgets unless the template has dojoAttachPoint=containerNode on the root node, but given the API of what getChildren() is supposed to do I don't want to change that (even in a new class).

comment:7 in reply to:  6 Changed 10 years ago by Neil Roberts

Replying to bill: You think this is fixed?

Note: See TracTickets for help on using tickets.