PDA

View Full Version : [CLOSED-28][3.x] FormLayout is still not dynamic.



Animal
1 May 2009, 4:58 AM
Now remove cleans up the field, but because of the isValidParent implementation in FormLayout, already-rendered fields which have been removed are not placed back into the DOM correctly on layout.

The problem is that Ext.form.Field does not have a valid getDomPositionEl implementation to allow the layout manager to determine the outermost element with which the layout is dealing.

I propose the following changes:



Ext.override(Ext.form.Field, {
getItemCt : function(){
return this.el.up('.x-form-item', 8);
},

getDomPositionEl: function() {
return this.getItemCt() || this.el;
}
});


getItemCt may need to go up more for more complex fields (eg DateTime input fields and others!)

Then FormLayout should revert to using the inherited isValidParent method from ContainerLayout instead of a hacked up version which just returns true

This is related to this 2.0 bug report: http://extjs.com/forum/showthread.php?t=67220

But it's more difficult to fix on 2.0 because removing a form Field from a form did not correctly clean up anyway in 2.0, so I propose that this fix is 3.0 only.

Condor
1 May 2009, 5:02 AM
Don't you think this solution (http://extjs.com/forum/showthread.php?p=120152#post120152) would be better?

Animal
1 May 2009, 5:21 AM
Different things.

3.0 does form field removal without any overrides.

But try removing a field (Passing false as the autoDestroy parameter), and then adding it back.

It won't be rendered, and that's what this report is about.

Condor
1 May 2009, 5:27 AM
1. Your solution is flawed. It relies on all components inside the form layout being Fields. But it could just as well be a BoxComponent with a fieldLabel config option.

2. The linked fix would write the itemCt as formItem to the component, so you could use:

getItemCt : function(){
return this.formItem;
},

Animal
1 May 2009, 5:37 AM
1. Your solution is flawed. It relies on all components inside the form layout being Fields.

How does it do that?

My solution simply adds an implementation of getDomPositionEl at the Ext.form.Field level, and then makes FormLayout use ContainerLayout's isValidParent method which examines the positionEl of an item.

Animal
1 May 2009, 5:41 AM
2. The linked fix would write the itemCt as formItem to the component, so you could use:

getItemCt : function(){
return this.formItem;
},

I admit that I do prefer the linked fix to stamp the formItem into the field on render rather than having to seek it out.

But as I said, that is a separate issue to this report.

Animal
15 Jun 2009, 12:29 PM
This is still here.

mystix
15 Jun 2009, 10:23 PM
[ moved from 2.x Bugs to 3.x Bugs ]

Condor
15 Jun 2009, 11:15 PM
How does it do that?

My solution simply adds an implementation of getDomPositionEl at the Ext.form.Field level, and then makes FormLayout use ContainerLayout's isValidParent method which examines the positionEl of an item.

In Ext 3.0 all components (even non-Fields) can have a fieldLabel that will be rendered by the FormLayout. You code only applies to Fields, so all non-Fields will still have the problem.

Instead, the formItem fix (http://extjs.com/forum/showthread.php?p=120152#post120152) could be modified to change not only the actionMode but also the positionEl of the element.

IMHO that is a better fix, because the FormLayout added the label in the first place, so it should also be responsible for removing it also.

Animal
15 Jun 2009, 11:37 PM
That's undoubtedly the best way of enabling hide/remove. By having the layout which did the rendering set the container, and change the actionMode to "container".

But there is the isValidParent problem.

It's a strange method that. It exists to let dynamic layout managers know if already rendered Components must be moved within the DOM on layout.

If it returns false, then ContainerLayout.renderItem will move it into it's position.

If it returns true, then the Component will stay removed from the DOM or in an incorrect position.

See below. If a Component is rendered, the red condition is evaluated. The Component's domPositionEl will NOT be inserted correctly if the isValidParent call returns true.

And FormLayout's isValidParent is hardcoded to return true! This is an incorrect implementation.

THis is what this bug report is about, not the hiding/showing of form fields using their outermost wrapper.



renderItem : function(c, position, target){
if(c && !c.rendered){
c.render(target, position);
this.configureItem(c, position);
}else if(c && !this.isValidParent(c, target)){
if(typeof position == 'number'){
position = target.dom.childNodes[position];
}
target.dom.insertBefore(c.getDomPositionEl().dom, position || null);
c.container = target;
this.configureItem(c, position);
}
},

evant
15 Jun 2009, 11:38 PM
I think we should clarify the issues here, I think Condor might be talking about something slightly different.

Animal
15 Jun 2009, 11:39 PM
Yes, we are talking at cross-purposes.

This report is about an incorrect implementation of isValidParent

Animal
15 Jun 2009, 11:44 PM
The problem also exists in TableLayout.js and is what makes TableLayout still not dynamic.

Newly added Components may be added to a TableLayout after the fix which moved the renderAll call to the end of onLayout.

But already rendered Components which have been removed from elsewhere will not be inserted properly because isValidParent returns true indicating that they do not need moving - their parent element is already correct.

The default implementation of isValidParent should return false, not true if it's too complex to work out whether the parent is actually valid.

Condor
15 Jun 2009, 11:47 PM
I meant:

Ext.override(Ext.layout.FormLayout, {
renderItem : function(c, position, target){
if(c && !c.rendered && (c.isFormField || c.fieldLabel) && c.inputType != 'hidden'){
var args = this.getTemplateArgs(c);
if(typeof position == 'number'){
position = target.dom.childNodes[position] || null;
}
if(position){
c.formItem = this.fieldTpl.insertBefore(position, args, true);
}else{
c.formItem = this.fieldTpl.append(target, args, true);
}
c.actionMode = 'formItem';
c.render('x-form-el-'+c.id);
c.container = c.formItem;
c.actionMode = 'container';
c.positionEl = c.formItem;
}else {
Ext.layout.FormLayout.superclass.renderItem.apply(this, arguments);
}
},
isValidParent : Ext.layout.ContainerLayout.prototype.isValidParent
});
which fixes FormLayout.isValidParent.

And I agree with Animal that TableLayout also needs to fix isValidParent (probably using the same method).

ps. ColumnLayout isValidParent also needs some cleanup (should use getDomPositionEl).

Animal
15 Jun 2009, 11:56 PM
There's something not quite right there though.

That checks if the parent Element of the Component's domPositionEl is the Container. OK as far as that goes.

But the renderItem call immediately below uses it to check whether the Component needs inserting into the DOM.

If a Component is removed from position 0, and then inserted at position 10 in the same Container, the doLayout will not reinsert it because isValidParent will return true.

This is the remove, re-add problem that the poster who instigated all this was having.

I don't know if having the isValidParent method is useful at all.

Condor
16 Jun 2009, 12:02 AM
Yes, Ext.layout.ContainerLayout.renderItem should also check if a position is specified, e.g.

Ext.override(Ext.layout.ContainerLayout, {
renderItem : function(c, position, target){
if(c && !c.rendered){
c.render(target, position);
this.configureItem(c, position);
}else if(c && (!this.isValidParent(c, target) || !Ext.isEmpty(position))){
if(typeof position == 'number'){
position = target.dom.childNodes[position];
}
target.dom.insertBefore(c.getDomPositionEl().dom, position || null);
c.container = target;
this.configureItem(c, position);
}
}
});
(is this enough or should it actually check if the position is different from the current position?)

evant
16 Jun 2009, 12:04 AM
Looking through it, I was thinking the same thing, especially for the ones that just blindly return true. I might get rid of it and see what happens on my local copy.

Animal
16 Jun 2009, 12:11 AM
Yes, Ext.layout.ContainerLayout.renderItem should also check if a position is specified, e.g.

Ext.override(Ext.layout.ContainerLayout, {
renderItem : function(c, position, target){
if(c && !c.rendered){
c.render(target, position);
this.configureItem(c, position);
}else if(c && (!this.isValidParent(c, target) || !Ext.isEmpty(position))){
if(typeof position == 'number'){
position = target.dom.childNodes[position];
}
target.dom.insertBefore(c.getDomPositionEl().dom, position || null);
c.container = target;
this.configureItem(c, position);
}
}
});
(is this enough or should it actually check if the position is different from the current position?)

If that side of the else continues to have conditional insertion of the Component's positionEl, then yes, it will need to check if the position is also not correct.

I agree with Evan, that the isValidParent may not actually be necessary.

I think with layouts which use absolute positioning like BorderLayout, then it's valid, because all you need to check is whether the child's Element is already in the correct parent element. Positioning is done with pixel position.

But in most cases, the Component will need moving into position.

mjlecomte
18 Jun 2009, 9:39 AM
Just an update: this issue appears to be still OPEN as of now, but the plan is to defer this until after the release of 3.0 final.