Opened 8 years ago

Closed 8 years ago

#14946 closed defect (fixed)

placeAt() API confusing

Reported by: bill Owned by: bill
Priority: undecided Milestone: 1.8
Component: Dijit Version: 1.7.2
Keywords: Cc: dante
Blocked By: Blocking:

Description (last modified by bill)

See attached file, It doesn't layout correctly, although if startup() is called at the very end it does.

Attachments (3)

earlyStartup.html (2.4 KB) - added by bill 8 years ago.
teslan's test case
earlyStartupFixed.html (2.1 KB) - added by bill 8 years ago.
test case with placeAt() calls corrected to be used according to it's spec... it's working
placeAtFullyFunctional.js (3.3 KB) - added by bill 8 years ago.
Code for a placeAt() that supports all the positional parameters that domConstruct.place() does. But I don't want to check this in since it's a fair amount of code.

Download all attachments as: .zip

Change History (8)

Changed 8 years ago by bill

Attachment: earlyStartup.html added

teslan's test case

comment:1 Changed 8 years ago by bill

Cc: dante added
Description: modified (diff)
Summary: startup() called early doesn't layout correctlyplaceAt() API confusing

Turns out it's a problem with the test case, or arguably a design problem in the spec of the placeAt() function. In other words, placeAt() is working according to spec, but the spec turns out to be confusing. I'm CC'ing Pete to see if he has anything to say, since placeAt() is his function.

The current rules are:

(1) if the reference widget has an addChild() method then pass it in without quotes. So, this works:

myWidget.placeAt(myLayoutWidget)

But this fails:

myWidget.placeAt("myLayoutWidget")

If placeAt() is passed a string, it's treated as a reference to a DOMNode, not as a reference to a widget.

(2) If the reference widget does not have an addChild() method, the opposite is true. This is correct:

myWidget.placeAt("myContentPane")

or slightly clearer:

myWidget.placeAt(myContentPane.domNode)

but this will fail:

myWidget.placeAt(myContentPane)

Of course, one way to avoid this problem is to stop using placeAt(). The test case could be rewritten to use addChild() for the layout widgets and set("content", ...) for the ContentPane.

Changed 8 years ago by bill

Attachment: earlyStartupFixed.html added

test case with placeAt() calls corrected to be used according to it's spec... it's working

comment:2 Changed 8 years ago by bill

Turns out this is complicated to implement to work intuitively, since:

childWidget.placeAt(parentWidget, 5);

should call parentWidget.addChild(), whereas

childWidget.placeAt(parentWidget, "before");
childWidget.placeAt(parentWidget, "after");

should be calling domConstruct.place().

Further, we need special handling for first and last when an addChild() method is available:

childWidget.placeAt(parentWidget, "first");
       --> parentWidget.addChild(childWidget, 0);
childWidget.placeAt(parentWidget, "last");
       --> parentWidget.addChild(childWidget, parentWidget.getChildren().length);

(This applies equally whether parentWidget is a widget reference, or a widget id.)

comment:3 Changed 8 years ago by bill

Even worse are the "before", "after", "replace" and "only" options, which currently somewhat work when you pass a DOMNode or string to placeAt(). They don't generally do what's expected when you pass an id of a widget, but they do something.

A correct implementation of childWidget.placeAt(reference, "before"), childWidget.placeAt(reference, "after") and childWidget.placeAt(reference, "replace") need to call addChild() on the grandparent widget, plus for position="replace", removeChild() and destroyRecursive(), and would be something like:

var refWidget = registry.byId(reference);
     gpw = refWidget && refWidget.getParent();
if(gpw && gpw.addChild){
    var parentIndex = gpw.getIndexOfChild(refWidget);
    gpw.addChild(this, position == "before" ? parentIndex : parentIndex+1);
}else{
   domConstruct.place(this.domNode, reference, position);
}
if(position == "replace" && refWidget){
    gpw.removeChild && gpw.removeChild(refWidget);
    refWidget.destroyRecursive();
}

and for childWidget.placeAt(reference, "only"):

if(refWidget){
   array.forEach(refWidget.getChildren(), function(child){
        refWidget.removeChild && refWidget.removeChild(child);
        child.destroyRecursive();
   }
}
domConstruct.empty(refWidget ? refWidget.containerNode || refWidget.domNode : reference);  // in case any stray DOM left
this.placeAt(reference, 0);

Changed 8 years ago by bill

Attachment: placeAtFullyFunctional.js added

Code for a placeAt() that supports all the positional parameters that domConstruct.place() does. But I don't want to check this in since it's a fair amount of code.

comment:4 Changed 8 years ago by bill

Milestone: tbd1.8
Owner: set to bill
Status: newassigned

comment:5 Changed 8 years ago by bill

Resolution: fixed
Status: assignedclosed

In [28251]:

Allow placeAt() to take an id of a widget or DOMNode, rather than an id refers to a DOMNode. This will (hopefully) avoid confusion given that a widget's id == it's DOMNode's id.

If the parent widget is started then call startup() on the child widget automatically, even if the parent doesn't have an addChild() method.

Also upgrading test to AMD.

Fixes #14946, #15066 !strict.

Note: See TracTickets for help on using tickets.