Thank you for reporting this bug. We will make it our priority to review this report.
  1. #1
    Ext User
    Join Date
    Dec 2009
    Posts
    5
    Vote Rating
    0
    andrebraga is on a distinguished road

      0  

    Exclamation [FIXED][CORE 3.0r114] Bug report and patch

    [FIXED][CORE 3.0r114] Bug report and patch


    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.
    Code:
    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));
    Attached Files
    Last edited by mystix; 4 Dec 2009 at 7:56 AM. Reason: POST CODE IN [code][/code] TAGS. see http://extjs.com/forum/misc.php?do=bbcode#code

  2. #2
    Ext User
    Join Date
    Dec 2009
    Posts
    5
    Vote Rating
    0
    andrebraga is on a distinguished road

      0  

    Exclamation Another bug

    Another bug


    Ext.Element.cache was renamed. This was missed here...
    Code:
    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);
     	    },
     	    
     		/**
    Attached Files
    Last edited by mystix; 4 Dec 2009 at 7:55 AM. Reason: POST CODE IN [code][/code] TAGS. see http://extjs.com/forum/misc.php?do=bbcode#code

  3. #3
    Sencha - Ext JS Dev Team evant's Avatar
    Join Date
    Apr 2007
    Location
    Sydney, Australia
    Posts
    16,921
    Vote Rating
    632
    evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute

      0  

    Default


    Confirmed the second.

    Can you post a test case that demonstrates the issue in your first post?
    Evan Trimboli
    Sencha Developer
    Twitter - @evantrimboli
    Don't be afraid of the source code!

  4. #4
    Ext User
    Join Date
    Dec 2009
    Posts
    5
    Vote Rating
    0
    andrebraga is on a distinguished road

      0  

    Thumbs up


    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...)

    Code:
    <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!

  5. #5
    Sencha - Ext JS Dev Team evant's Avatar
    Join Date
    Apr 2007
    Location
    Sydney, Australia
    Posts
    16,921
    Vote Rating
    632
    evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute

      0  

    Default


    This has been fixed in SVN, core rev 120.
    Evan Trimboli
    Sencha Developer
    Twitter - @evantrimboli
    Don't be afraid of the source code!

  6. #6
    Ext User
    Join Date
    Dec 2009
    Posts
    5
    Vote Rating
    0
    andrebraga is on a distinguished road

      0  

    Default


    You forgot to land this fix:

    Code:
    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!

  7. #7
    Sencha - Ext JS Dev Team evant's Avatar
    Join Date
    Apr 2007
    Location
    Sydney, Australia
    Posts
    16,921
    Vote Rating
    632
    evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute

      0  

    Default


    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.
    Evan Trimboli
    Sencha Developer
    Twitter - @evantrimboli
    Don't be afraid of the source code!

  8. #8
    Ext User
    Join Date
    Dec 2009
    Posts
    5
    Vote Rating
    0
    andrebraga is on a distinguished road

      0  

    Default


    Quote Originally Posted by evant View Post
    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.

Thread Participants: 1