PDA

View Full Version : [FIXED][CORE 3.0r114] Bug report and patch



andrebraga
3 Dec 2009, 11:10 AM
Quick bugfixes for doMethod (src/adapter/ext-base-anim-extra.js) and setWidth, setHeight (src/core/Element.style.js).


doMethod: moved the declaration of 'len' inside the conditional. It's not guaranteed that the animation has start and endpoints otherwise, so 'start' might be undefined, which will throw an error that won't be caught.

setWidth, setHeight: negative return values are left unchecked, which broke IE < 8 in situations experienced by me.

Major and generalised whitespace cleanup is needed throughout the source (no uniform style of indentation where spaces and tabs are mixed; and plenty of trailing spaces), but not done in order not to disrupt the readability of this couple of otherwise simple patches.


diff -r 96dfcc38b840 -r 29aaf0c0773a src/adapter/ext-base-anim-extra.js
--- a/src/adapter/ext-base-anim-extra.js Tue Dec 01 23:21:47 2009 +0000
+++ b/src/adapter/ext-base-anim-extra.js Thu Dec 03 17:00:41 2009 -0200
@@ -194,9 +194,10 @@
var me = this,
val,
floor = Math.floor,
- i, len = start.length, v;
+ i, v;

if(colorRE.test(attr)){
+ var len = start.length;
val = [];

for(i=0; i<len; i++) {
diff -r 96dfcc38b840 -r 29aaf0c0773a src/core/Element.style.js
--- a/src/core/Element.style.js Tue Dec 01 23:21:47 2009 +0000
+++ b/src/core/Element.style.js Thu Dec 03 17:00:41 2009 -0200
@@ -339,6 +339,7 @@
setWidth : function(width, animate){
var me = this;
width = me.adjustWidth(width);
+ if(parseFloat(width) < 0) width = 0;
!animate || !me.anim ?
me.dom.style.width = me.addUnits(width) :
me.anim({width : {to : width}}, me.preanim(arguments, 1));
@@ -368,6 +369,7 @@
setHeight : function(height, animate){
var me = this;
height = me.adjustHeight(height);
+ if(parseFloat(height) < 0) height = 0;
!animate || !me.anim ?
me.dom.style.height = me.addUnits(height) :
me.anim({height : {to : height}}, me.preanim(arguments, 1));

andrebraga
3 Dec 2009, 12:02 PM
Ext.Element.cache was renamed. This was missed here...


diff -r 29aaf0c0773a -r b3b71a5972a0 src/core/Element.insertion.js
--- a/src/core/Element.insertion.js Thu Dec 03 17:00:41 2009 -0200
+++ b/src/core/Element.insertion.js Thu Dec 03 17:44:35 2009 -0200
@@ -81,8 +81,7 @@
* @return {Ext.Element} this
*/
replaceWith: function(el){
- var me = this,
- Element = Ext.Element;
+ var me = this;
if(el.nodeType || el.dom || typeof el == 'string'){
el = GETDOM(el);
me.dom.parentNode.insertBefore(el, me.dom);
@@ -90,10 +89,10 @@
el = DH.insertBefore(me.dom, el);
}

- delete Element.cache[me.id];
+ delete Ext.elCache[me.id];
Ext.removeNode(me.dom);
me.id = Ext.id(me.dom = el);
- return Element.cache[me.id] = me;
+ return Ext.Element.addToCache(me);
},

/**

evant
3 Dec 2009, 8:29 PM
Confirmed the second.

Can you post a test case that demonstrates the issue in your first post?

andrebraga
9 Dec 2009, 7:30 AM
Sure, and sorry for the delay, I'm just too swamped by work at the moment.

(BTW, I know the first image will flicker after the transition to the second is completed. But this is not the issue here and I'm too busy with the other project to fix it right now...)



<style type="text/css">
/*<!CDATA[[*/
#banner, #banner div.centra {
height: 271px;
}

#banner div.center {
background-position: center 238px;
}

#banners {
height: 232px;
}

#banners img {
width: 900px;
height: 232px;
border: 0;
}

#banners div {
position: absolute;
top: 0;
left: 0;
width: 900px;
height: 232px;
visibility: hidden;
}

#banners div.vis {
visibility: visible !important;
}

#bt-banners {
position: absolute;
right: 25px;
bottom: 59px;
height: 22px;
border: solid black;
border-top-width: 0;
border-bottom-width: 0;
border-left-width: 1px;
border-right-width: 1px;
font-size: 14px;
text-align: center;
}

#bt-banners span {
display: block;
float: left;
width: 22px;
height: 22px;
line-height: 22px;
border: solid black;
border-top-width: 0;
border-bottom-width: 0;
border-left-width: 1px;
border-right-width: 1px;
font-weight: bold;
color: white;
background-color: #83715f;
cursor: pointer;
}

#bt-banners span.sel {
background-color: #4d331a;
}
/*]]>*/
</style>

<script type="text/javascript">
/*<!CDATA[[*/
anim = (function(){
return {
qtty : null,
atop : null,
next : null,
itvl : null,
hdlr : null,
task: new Ext.util.DelayedTask(function(){
Ext.get('c-'+anim.atop).stopFx().show(
{
duration: 1,
block: true,
callback: function(el) {el.radioClass('vis')}
}
)
}),
trans: function(e) {
if ('undefined' != typeof e) {
clearInterval(this.hdlr);
this.init();
}
if (e != this.atop) {
var prxm = e || this.next();
Ext.get('c-'+this.atop).stopFx().hide({duration: 1, block: true});
Ext.get('s-'+this.atop).stopFx().animate(
{
backgroundColor:{from: '#4d331a', to: '#83715f'}, block: true
}, 1, null, 'easeIn', 'color'
);
this.atop = prxm;
this.task.delay(333);
Ext.get('s-'+this.atop).stopFx().animate(
{
backgroundColor:{from: '#83715f', to: '#4d331a'}, block: true
}, 1, null, 'easeIn', 'color'
);
}
},
init: function(i) {
this.qtty = this.qtty || Ext.query('#banners div').length,
this.atop = this.atop || parseInt(Ext.query('#banners div.vis')[0].id.match(/-(\d+)/)[1]),
this.next = this.next || function() {return ((this.atop + 1) % (this.qtty + 1)) || 1},
this.itvl = i || this.itvl || 10000;
this.hdlr = setInterval('anim.trans()', this.itvl);
}
}
})();
/*]]>*/
</script>

<div id="banner">
<div class="center">
<div id="banners">
<div id="c-2">
<a href="">
<img src="banner1.jpg" alt="" />
</a>
</div>
<div id="c-1" class="vis">
<a href="">
<img src="banner2.jpg" alt="" />
</a>
</div>
</div>
<div id="bt-banners">
<span id="s-1" class="sel" onmouseover="anim.trans(1)">1</span>
<span id="s-2" onmouseover="anim.trans(2)">2</span>
</div>
<script type="text/javascript">
/*<!CDATA[[*/
Ext.onReady(function(){anim.init(9000)});
/*]]>*/
</script>
</div>
</div>


Live example (but with the mentioned patches already applied, so it might not be of any help) here: http://www.bybrazilartesanatos.com.br/site/

Thanks!

evant
13 Dec 2009, 9:29 PM
This has been fixed in SVN, core rev 120.

andrebraga
14 Dec 2009, 3:14 PM
You forgot to land this fix:



diff -r 96dfcc38b840 -r 29aaf0c0773a src/core/Element.style.js
--- a/src/core/Element.style.js Tue Dec 01 23:21:47 2009 +0000
+++ b/src/core/Element.style.js Thu Dec 03 17:00:41 2009 -0200
@@ -339,6 +339,7 @@
setWidth : function(width, animate){
var me = this;
width = me.adjustWidth(width);
+ if(parseFloat(width) < 0) width = 0;
!animate || !me.anim ?
me.dom.style.width = me.addUnits(width) :
me.anim({width : {to : width}}, me.preanim(arguments, 1));
@@ -368,6 +369,7 @@
setHeight : function(height, animate){
var me = this;
height = me.adjustHeight(height);
+ if(parseFloat(height) < 0) height = 0;
!animate || !me.anim ?
me.dom.style.height = me.addUnits(height) :
me.anim({height : {to : height}}, me.preanim(arguments, 1));


This one happened under IE7- when using a modified version of the Lightbox example based on the one found on Typo3. I hope you can verify that this bug happens, but nevertheless I think it's trivial and harmless enough to warrant landing on trunk.

Thanks!

evant
14 Dec 2009, 6:17 PM
It's not really a bug with the code. If you're trying to set a negative width, it indicates you've probably got a bug elsewhere.

This has been raised before and that was the general consensus.

andrebraga
16 Dec 2009, 4:36 AM
It's not really a bug with the code. If you're trying to set a negative width, it indicates you've probably got a bug elsewhere.

This has been raised before and that was the general consensus.

Could be, however this "bug" is not on my code but manifests itself when running the Typo3 Lightbox, only on a post-3.0-release version of Core, and only under IE < 8. Since this is likely a renderer bug, any performance gains from not implementing this very simple verification at the library level are necessarily going to be negated by client code that will have to implement it, because, well, someone will have to do the sanity checking anyway.

I've proofread that lightbox code (it's only slightly different from the sample lightbox code on the Core page) and found nothing that could lead to negative widths except for, indeed, unpredictable renderer bugs.

I can recognise defeat when I'm faced with it, so I won't waste anyone's time fighting for this rather trivial fix given that I can easily keep local patches, but since it's been reported before on a different situation, has virtually zero impact on performance, and not sanity checking parameters is bad programming practice under any circumstances, even on a performance-oriented library like Ext, I urge you to reconsider this decision. As the saying goes, "the thing with very quick but slightly buggy code is that it ends up producing slightly wrong results very quickly, which adds up to something completely wrong fast enough!" :)


Thanks,
A.