Ticket #4328 (closed bug: fixed)

Opened 6 years ago

Last modified 5 years ago

IE6 with fx.off and highlight throws a script error

Reported by: chrisbarr Owned by:
Priority: critical Milestone: 1.8
Component: ui.effects.core Version: 1.7
Keywords: fx.off off highlight ie6 Cc:
Blocking: Blocked by:

Description

I found that when using IE6 with $.fx.off=true set and when a .effect("highlight") is called on a page load or something else other than a click it throws a script error.

It says "invalid property value" for jQuery UI when it hits this point for color animations in jQuery.effects:

fx.elem.style[attr] = "rgb(" + [

Math.max(Math.min(parseInt((fx.pos*(fx.end[0]-fx.start[0]))+fx.start[0],10),255),0),

Math.max(Math.min(parseInt((fx.pos*(fx.end[1]-fx.start[1]))+fx.start[1],10),255),0),

Math.max(Math.min(parseInt((fx.pos*(fx.end[2]-fx.start[2]))+fx.start[2],10),255),0)

].join(",") + ")";

I put together a demo here:  http://jsbin.com/omeme/edit

Change History

comment:1 Changed 6 years ago by scott.gonzalez

  • Milestone changed from 1.next to 1.8

comment:2 Changed 6 years ago by doublerebel

 Fixed in Ticket 4324 attachment

To avoid redundancy, I have attached my effects core at Ticket 4324. It includes fixes for the color animation bug found in IE6+. I can't say for sure why this bug happens, but I have the fix.

Please contact me with any questions.

comment:3 Changed 6 years ago by epascarello

why the bug happens?

Well in the case of this bug it is pretty easy to see why it happens in IE.

The state is 1 in the first pass. If never goes into the if statement. IE stores the value as HEX not the rgb tupal. The value of start is just zero and the value of end is a HEX value.

I was working on the bug in the beginning of this week and got this to work

/*
 * jQuery Color Animations
 * Copyright 2007 John Resig
 * Released under the MIT and GPL licenses.
 */

// We override the animation for all of these color styles
$.each(['backgroundColor', 'borderBottomColor', 'borderLeftColor', 'borderRightColor', 'borderTopColor', 'color', 'outlineColor'], function(i,attr){
		$.fx.step[attr] = function(fx) {

				if ( fx.state == 0 ) {
						fx.start = getColor( fx.elem, attr );
						fx.end = getRGB( fx.end );
				}
				else if(fx.state == 1 &&  fx.end.charAt(0) === "#"){  //IE bug where color code is still in hash, not tupal
			        fx.start = [0,0,0];
					fx.end = getRGB( fx.end );
				}

				fx.elem.style[attr] = "rgb(" + [
						Math.max(Math.min( parseInt((fx.pos * (fx.end[0] - fx.start[0])) + fx.start[0],10), 255), 0),
						Math.max(Math.min( parseInt((fx.pos * (fx.end[1] - fx.start[1])) + fx.start[1],10), 255), 0),
						Math.max(Math.min( parseInt((fx.pos * (fx.end[2] - fx.start[2])) + fx.start[2],10), 255), 0)
				].join(",") + ")";
			};
});

I still do not think that is the right solution [even though it does work], just have not had time to go through all of the test cases I can think of.

comment:4 Changed 6 years ago by dunghopper

As epascarello noted, it seems that fx.state == 1 in the first pass. From looking at the jQuery code, this is the case if "gotoEnd" is passed to update() the first time... in other words, if the animation finishes before it starts because the duration was very short, or there was just too much other code to run, blocking the animation.

Since the state is not reliable, I suggest we ignore it and just check fx.start instead (since that is really what we are interested in... if it is not initialized, we need to initialize it regardless of the value of fx.state.)

I suggest:

// We override the animation for all of these color styles
$.each(['backgroundColor', 'borderBottomColor', 'borderLeftColor', 'borderRightColor', 'borderTopColor', 'color', 'outlineColor'], function(i,attr){
		$.fx.step[attr] = function(fx) {
				if ( !fx.start ) {
						fx.start = getColor( fx.elem, attr );
						fx.end = getRGB( fx.end );
				}
				fx.elem.style[attr] = "rgb(" + [
						Math.max(Math.min( parseInt((fx.pos * (fx.end[0] - fx.start[0])) + fx.start[0],10), 255), 0),
						Math.max(Math.min( parseInt((fx.pos * (fx.end[1] - fx.start[1])) + fx.start[1],10), 255), 0),
						Math.max(Math.min( parseInt((fx.pos * (fx.end[2] - fx.start[2])) + fx.start[2],10), 255), 0)
				].join(",") + ")";
			};
});

That still may not be complete, but I think it will prevent many instances of this bug.

comment:5 Changed 6 years ago by ZaDarkSide

I suggest:

// We override the animation for all of these color styles
$.each(['backgroundColor', 'borderBottomColor', 'borderLeftColor', 'borderRightColor', 'borderTopColor', 'color', 'outlineColor'], function(i,attr){
		$.fx.step[attr] = function(fx) {
				fx.start = getColor( fx.elem, attr );
				fx.end = getRGB( fx.end );
				fx.elem.style[attr] = "rgb(" + [
						Math.max(Math.min( parseInt((fx.pos * (fx.end[0] - fx.start[0])) + fx.start[0],10), 255), 0),
						Math.max(Math.min( parseInt((fx.pos * (fx.end[1] - fx.start[1])) + fx.start[1],10), 255), 0),
						Math.max(Math.min( parseInt((fx.pos * (fx.end[2] - fx.start[2])) + fx.start[2],10), 255), 0)
				].join(",") + ")";
			};
});

I just commented:

//if ( fx.state == 0 ) {
	/*...*/
//}

comment:6 follow-up: ↓ 7 Changed 6 years ago by wewals

I just commented:

//if ( fx.state == 0 ) {
	/*...*/
//}

I have looked into this a bit as well. Going with this method will not work as it throws off the easing because start gets reset to the current color each iteration instead of only being set once.

dunghopper's recomendation seems like a good options. I haven't checked into it as much as I would like though.

comment:7 in reply to: ↑ 6 Changed 6 years ago by ZaDarkSide

Replying to wewals:

The problem is that dunghopper's recomendation still throws an error and the whole script stops, my suggestion was tested and works for me as expected.

comment:8 Changed 6 years ago by scott.gonzalez

  • Status changed from new to closed
  • Resolution set to fixed

Fixed in r2697. The .effect() method in effects.core.js now honors jQuery.fx.off so individual effects don't need to worry about it.

Note: See TracTickets for help on using tickets.