PDA

View Full Version : [FIXED-341][3.x] Small bug in AnchorLayout



Animal
1 Dec 2009, 6:29 AM
When calculating how much margin round a child Component it must account for, AnchorLayout uses the wrong element.

It uses c.getEl(). This is correct 99% of the time, but not when the getEl method returns an inner element (eg TriggerField).

What is wanted there is the outermost element's margins.

Fix:



Ext.override(Ext.layout.AnchorLayout, {
onLayout : function(ct, target){
Ext.layout.AnchorLayout.superclass.onLayout.call(this, ct, target);

var size = this.getAnchorViewSize(ct, target);

var w = size.width, h = size.height;

if(w < 20 && h < 20){
return;
}

// find the container anchoring size
var aw, ah;
if(ct.anchorSize){
if(typeof ct.anchorSize == 'number'){
aw = ct.anchorSize;
}else{
aw = ct.anchorSize.width;
ah = ct.anchorSize.height;
}
}else{
aw = ct.initialConfig.width;
ah = ct.initialConfig.height;
}

var cs = ct.items.items, len = cs.length, i, c, a, cw, ch, el, vs;
for(i = 0; i < len; i++){
c = cs[i];
el = c.getDomPositionEl();
if(c.anchor){
a = c.anchorSpec;
if(!a){ // cache all anchor values
vs = c.anchor.split(' ');
c.anchorSpec = a = {
right: this.parseAnchor(vs[0], c.initialConfig.width, aw),
bottom: this.parseAnchor(vs[1], c.initialConfig.height, ah)
};
}
cw = a.right ? this.adjustWidthAnchor(a.right(w) - el.getMargins('lr'), c) : undefined;
ch = a.bottom ? this.adjustHeightAnchor(a.bottom(h) - el.getMargins('tb'), c) : undefined;

if(cw || ch){
c.setSize(cw || undefined, ch || undefined);
}
}
}
}
});


As an aside, since getting the computed style is found to be quite expensive, perhaps some way could be found to only get the margins once, instead of those two calls highlighted in green.

Animal
1 Dec 2009, 6:55 AM
In fact, the way Element.style.js::addStyles works, that will calculate the computed style 4 times. Once for each side.

A small start on this would be



Ext.override(Ext.Element, {

addStyles : function(sides, styles){
var val = 0,
m = sides.match(/\w/g),
st = [], s;

// Collect all required style attributes
for (var i=0, len=m.length; i<len; i++) {
m[i] && st.push(styles[m[i]]);
}


// Get all required style attributes in one shot
st = this.getStyles.apply(this, st);

// Add them together
for (var i in st) {
if (s = parseInt(st[i], 10)) {
val += Math.abs(s);
}
}
return val;
}
});

Animal
1 Dec 2009, 6:57 AM
No, because getStyles makes multiple calls to getStyle!!! That's a prime candidate for a performance refactor.

evant
1 Dec 2009, 7:59 PM
I'll fix the initial bug with getting the wrong el.

The other performance issues we can look at later, though I'm curious as to how this can be sped up. What is the issue with getStyles making multiple calls to getStyle?

Animal
1 Dec 2009, 11:20 PM
I think various people have done Firebug profiling and found that getStyle (getting one individual style attribute) was very heavily called, and an expensive call.

What I am suggesting is a way to "batch" requests for an Element's style so that for instance the following statements:



cw = a.right ? this.adjustWidthAnchor(a.right(w) - el.getMargins('lr'), c) : undefined;
ch = a.bottom ? this.adjustHeightAnchor(a.bottom(h) - el.getMargins('tb'), c) : undefined;


could only make one call to the underlying browser's method of getting the current computed style instead of 4.

A test someone once posted:

https://ninja.9star.com/public/ygrid/profile1.gif

meroy
2 Dec 2009, 6:07 AM
Usage (count) on is??? variables throughout ext-base and ext-all. Below, isStyle is a function though.



base all is???-usage (count)
---- --- ------------
4 13 isAir
4 15 isBorderBox
4 4 isChrome
10 37 isGecko
5 6 isGecko2
2 0 isGecko3
19 79 isIE
8 10 isIE6
5 10 isIE7
4 4 isIE8
2 1 isLinux
5 7 isMac
7 24 isOpera
7 8 isSafari
2 13 isSafari2
2 1 isSafari3
2 0 isSafari4
8 22 isStrict
4 14 isStyle
6 33 isWebKit
2 1 isWindows


When does it make sense to create a few unique functions in the bootstrap code and call via a reference. Why not take expensive functions like getStyle and create 2 or 3 to allow for better inline 'ing of code unique to a smaller set of browsers? Why not inline the chkCache function -- it's only one line and only call from 3 places? What's the overhead of calling chkCache via a function?

There are so many is??? throughout Ext JS. Look at the numbers above. What will happen in 3 to 5 years when we have isIE9, isIEx, isSafari5, isSafarix and so forth?

Another idea: Is it possible to create tight code for a smaller set -- perhaps 2 or 3 for given routines and then have the bootstrap null out the unused routines at runtime to keep the memory footprint small? This will help eliminate many is??? checks. At least do this for the very expensive routines.

meroy
2 Dec 2009, 6:30 AM
For example: Why can't the bootstrap figure this out once and create a function reference to the one to be used going forward?

Create removeNodeIE and removeNodeOthers and have the bootstrap point removeNode to one of them. Then null/destroy the one not being used. This will remove 2 checks. There are so many checks similar to this in red below throughout Ext JS and Core. It's not possible to handle all cases now, but at least consider for the functions that are called often.

Off topic: For batching, consider hashes of hashes. It will allow via one call to manage a given hash key and will cut down the time to process instead of iterating the entire hash data structure created as a one level.



removeNode : isIE && !isIE8 ? function(){
var d;
return function(n){
if(n && n.tagName != 'BODY'){
(Ext.enableNestedListenerRemoval) ? Ext.EventManager.purgeElement(n, true) : Ext.EventManager.removeAll(n);
d = d || DOC.createElement('div');
d.appendChild(n);
d.innerHTML = '';
delete Ext.elCache[n.id];
}
}
}() : function(n){
if(n && n.parentNode && n.tagName != 'BODY'){
(Ext.enableNestedListenerRemoval) ? Ext.EventManager.purgeElement(n, true) : Ext.EventManager.removeAll(n);
n.parentNode.removeChild(n);
delete Ext.elCache[n.id];
}
}

Animal
2 Dec 2009, 6:40 AM
I agree that some functions should be conditionally defined in bootstrap code and inserted into their owning prototypes/objects.

Stuff like dealing with style is particularly different between IE and W3 compliant browsers, so should be conditionally created. No point in either sniffing the browser, or testing for a feature every time. The browser isn't going to change.

meroy
2 Dec 2009, 6:44 AM
Is this determined once and only during the initial load? Therefore, no longer performing checks on isIE and !isIE8 for subsequent calls to removeNode?



removeNode : isIE && !isIE8 ? function(){


All these is??? vars got to me -- there are so many all over the place.

To help @animals' case -- will hashes of hashes help improve the logic for obtaining styles on elements?

meroy
2 Dec 2009, 6:58 AM
Just not sure how JavaScript is handling that and if setting the function to use initially for removeNode. This is just an example seen in Ext. There's no need for JavaScript to perform the isIE checks every time removeNode is called in this case when it could be handled during bootstrap. Pick the few functions than can easily be broken into 2 to 3 smaller and tighter functions and define/use a reference. Also remove the one(s) not being used. Make this easy for down the road when we have isIE(x), isGecko(x), isSafari(x), isNewBrowser(x). Create hash lookups for bootstrap for these for knowing which to null/delete during the initial runtime. Make it fast. Perhaps, not sure if possible, but have these in a different js file by browser class type and have the boot strap load/eval the js file. For phase one, do it in Ext. See how much larger the code size is -- should not be much -- just do this for the critical functions.

The same can be done to garbageCollect. There's no need for all non-IE browsers to check if isIE every time GC is called.

meroy
2 Dec 2009, 7:12 AM
Just seeing a hash structure helps me in this case -- I've taken this from the perldsc man page: man perldsc and look for HASHES OF HASHES



%HoH = (
flintstones => {
lead => "fred",
pal => "barney",
},
jetsons => {
lead => "george",
wife => "jane",
"his boy" => "elroy",
},
simpsons => {
lead => "homer",
wife => "marge",
kid => "bart",
},
);


I see flintstones, jetsons and simpsons being the dom element ids. I see lead(s), pal, wife(s), "his boy" and kid being the style or attribute names. Not sure if this helps.

Animal
3 Dec 2009, 5:42 AM
This problem is also manifesting in isValidParent implementations.

When a Component's main getEl() element is wrapped, the isValidParent tests it and always reports that its not correctly placed and needs to be re-housed.

Again, isValidParent needs to use the outermost element, getPositionEl().

Animal
3 Dec 2009, 5:45 AM
Actually, it's only BoxLayout that has this error in.

evant
3 Dec 2009, 5:55 AM
Also looks like ColumnLayout should be calling getPositionEl when grabbing margins.

Animal
3 Dec 2009, 5:57 AM
Yes. I just did a search for "c.getEl()" and those were the only occurrences - BoxLayout and ColumnLayout.

evant
3 Dec 2009, 6:19 AM
Yep, this is now in SVN.