PDA

View Full Version : Element.getParent() returns itself



dfenwick
28 Mar 2007, 8:59 PM
the findParent() call is supposed to find the nearest matching parent for a particular element. However, in findParent, it starts the query at itself, so if you have a set of nested divs, it will return itself on a simple query for divs.

Example:

I have something that looks like this:


<div>
<div id="foo"></div>
</div>

If you do this:


var myEl = Ext.get('foo').findParent('div', 10, true);

myEl is equal to the element returned from Ext.get('foo'). This is because the relevant code in core/Element.js does this:


findParent : function(simpleSelector, maxDepth, returnEl){
var p = this.dom, b = document.body, depth = 0, dq = Ext.DomQuery, stopEl;
maxDepth = maxDepth || 50;
if(typeof maxDepth != "number"){
stopEl = Ext.getDom(maxDepth);
maxDepth = 10;
}
while(p && p.nodeType == 1 && depth < maxDepth && p != b && p != stopEl){
if(dq.is(p, simpleSelector)){
return returnEl ? Ext.get(p) : p;
}
depth++;
p = p.parentNode;
}
return null;
},

The expected behavior of a function called findParent() should be to not look at yourself. So a proposed fix is to change it as such:



findParent : function(simpleSelector, maxDepth, returnEl){
var p = this.dom.parentNode, b = document.body, depth = 0, dq = Ext.DomQuery, stopEl;
maxDepth = maxDepth || 50;
if(typeof maxDepth != "number"){
stopEl = Ext.getDom(maxDepth);
maxDepth = 10;
}
while(p && p.nodeType == 1 && depth < maxDepth && p != b && p != stopEl){
if(dq.is(p, simpleSelector)){
return returnEl ? Ext.get(p) : p;
}
depth++;
p = p.parentNode;
}
return null;
},

Animal
29 Mar 2007, 12:40 AM
I agree that your proposal sounds more logical. But I think that needs to be an option.

The reason is that findParent is used in a couple of places in Ext to detect the enclosing node of a clicked target.

For example, clicking on an element inside a grid header, needs to look upwards and find the actual "td" which encapsulates the header. But the click might not be on a child element in the td - it might be in the td itsef. So it has to include the target node itself in its upward scan.

I don't know, what do you think Jack? Which way should the default findParent go? include current element or exclude?

jack.slocum
29 Mar 2007, 1:30 PM
This is one of the rare documented functions in ext:


/**
* Looks at this node and then at parent nodes for a match of the passed simple selector (e.g. div.some-class or span:first-child)
* @param {String} ss The simple selector to test
* @param {Number/String/HTMLElement/Element} maxDepth (optional) The max depth to
search as a number or element (defaults to 10 || document.body)
* @param {Boolean} returnEl (optional) True to return a Ext.Element object instead of DOM node
* @return {HTMLElement}
*/

And Animal, you hit it right on. It's used in many places by Ext and I'm sure in other people code. The name may not be the best though but changing it at this point is not possible.

dfenwick
29 Mar 2007, 5:24 PM
This is one of the rare documented functions in ext:


/**
* Looks at this node and then at parent nodes for a match of the passed simple selector (e.g. div.some-class or span:first-child)
*/

And Animal, you hit it right on. It's used in many places by Ext and I'm sure in other people code. The name may not be the best though but changing it at this point is not possible.

I actually looked further at this after I posted the bug and I realized it was used all over the place. I left the bug out there because I guarantee it will end up being one of those FAQ style questions in the future. I completely misinterpreted the function description to mean "Given this node, look at all parents of this node and find a match given this criteria."

The function name itself is what will lead to confusion, as the majority (all?) of the APIs I've ever looked at in the past, and it's been a LOT of APIs, finds parents when given a child. It can wait until after 1.0 final to get looked at. Something as simple as adding a 4th parameter (excludeSelf?) that allows a caller to not include itself in the search would be acceptable.

Animal
29 Mar 2007, 11:33 PM
Is there time to implement



* @param {Event} e The event for which to find a matching enclosing Element
* @param {String} ss The simple selector to test
* @param {Number/String/HTMLElement/Element} maxDepth (optional) The max depth to
search as a number or element (defaults to 10 || document.body)
* @param {Boolean} returnEl (optional) True to return a Ext.Element object instead of DOM node
* @return {HTMLElement}
findEnclosingElement(e, selector, number, relurnEl)


then change the usage inside Ext to use that, and have findParentElement do exactly what it says?

It could become one of those constant FAQ questions.

heidtmare
30 Mar 2007, 5:37 AM
yeah, there is a definite need for a findParentNode() and findParentElement() distinction.

jack.slocum
3 Apr 2007, 11:26 AM
The names look pretty redundant but:


/**
* Looks at parent nodes for a match of the passed simple selector (e.g. div.some-class or span:first-child)
* @param {String} ss The simple selector to test
* @param {Number/String/HTMLElement/Element} maxDepth (optional) The max depth to
search as a number or element (defaults to 10 || document.body)
* @param {Boolean} returnEl (optional) True to return a Ext.Element object instead of DOM node
* @return {HTMLElement}
*/
findParentNode : function(simpleSelector, maxDepth, returnEl){
var p = Ext.fly(this.dom.parentNode, '_internal');
return p ? p.findParent(simpleSelector, maxDepth, returnEl) : null;
},

jack.slocum
15 Apr 2007, 8:46 PM
You guys will be happy to find a new function "up" in the 1.0 build that is similar to findParentNode but returns an element by default:

el.up('li").addClass('foo');

end-user
26 Oct 2007, 8:32 AM
Is there a getPrevSibling() method that will return the Ext element instead of the HTML one? (like up() vs findParentNode())

tryanDLS
26 Oct 2007, 9:17 AM
Please don't resurrect 6 month old bug thread to ask a new question. Post a new question in the appropriate forum.