Skip to content

Commit

Permalink
Prevent IE from throwing errors when setting RGBA values. Fixes #5509.
Browse files Browse the repository at this point in the history
  • Loading branch information
@jeresig
Colin Snover authored and jeresig committed Oct 9, 2010
1 parent a2aefbf commit 2ca3659
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 2 deletions.
6 changes: 5 additions & 1 deletion src/css.js
View file
Expand Up @@ -88,7 +88,11 @@ jQuery.extend({

// If a hook was provided, use that value, otherwise just set the specified value
if ( !hooks || !("set" in hooks) || (value = hooks.set( elem, value )) !== undefined ) {
style[ name ] = value;
// Wrapped to prevent IE from throwing errors when 'invalid' values are provided
// Fixes bug #5509
try {
style[ name ] = value;
} catch(e) {}
}

} else {
Expand Down
12 changes: 11 additions & 1 deletion test/unit/css.js
View file
Expand Up @@ -64,7 +64,7 @@ test("css(String|Hash)", function() {
});

test("css(String, Object)", function() {
expect(21);
expect(22);

ok( jQuery('#nothiddendiv').is(':visible'), 'Modifying CSS display: Assert element is visible');
jQuery('#nothiddendiv').css("display", 'none');
Expand Down Expand Up @@ -104,6 +104,16 @@ test("css(String, Object)", function() {

equals( ret, div, "Make sure setting undefined returns the original set." );
equals( div.css("display"), display, "Make sure that the display wasn't changed." );

// Test for Bug #5509
var success = true;
try {
jQuery('#foo').css("backgroundColor", "rgba(0, 0, 0, 0.1)");
}
catch (e) {
success = false;
}
ok( success, "Setting RGBA values does not throw Error" );
});

if(jQuery.browser.msie) {
Expand Down

13 comments on commit 2ca3659

@sroussey
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um... aren't setting up try/catch blocks expensive? Particularly for IE where it will invoke the catch block and setup a whole new scope. Perhaps validation code is just as expensive, so it is irrelevant?

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 2ca3659 Oct 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't get this. Why we have to "support" any invalid value by silently ignore errors? Wouldn't be more logical to throw error when rgba is used? At least, such behavior would be consistent across different browsers.

If this is about "progressive enhancement", wouldn't be more adequate to introduce jQuery.support.rgba and to convert rgba values to rgb values if the first one is not supported?

@jeresig
Copy link
Member

@jeresig jeresig commented on 2ca3659 Oct 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkatic: I disagree - we want to support rgba (and other, newer, CSS properties) for those browsers that can handle them - at the same time we want to make sure that IE doesn't barf on them. This is decidedly different from the case where we throw an error in all browsers when .attr("type", ...) is done on an input element - since that is functionality that is expected to work everywhere, but won't - and we need to inform the user.

Actually supporting rgba is a tangential issue. We could do it in jQuery but it doesn't really seem worthwhile - especially if it isn't working in the stylesheet to begin with.

@jdalton
Copy link
Member

@jdalton jdalton commented on 2ca3659 Oct 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the error is suppressed and the style is never applied. Won't that leave devs scratching their heads?
If a dev is setting a style value with rgba they probably don't care about IE support in the first place amiright?

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 2ca3659 Oct 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton: I think the John's point is: If rgba in css is simply not applied, than same behavior is expected in JS too. Maybe because rgba could be used as a "progressive enhancement" hack...

@jeresig
Copy link
Member

@jeresig jeresig commented on 2ca3659 Oct 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton: No - it's the same as if you apply an unknown style in any other browser. .css("foobar", "baz") (we don't throw an exception).

@jdalton
Copy link
Member

@jdalton jdalton commented on 2ca3659 Oct 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also swallows other errors that might happen when setting a style property and not just rgba.
Devs may find error messages beneficial during the development process.

@jeresig
Copy link
Member

@jeresig jeresig commented on 2ca3659 Oct 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton: Although they're far more likely to find them baffling and unnecessary - as the bevvy of tickets in our bug tracker indicates :) The user is expecting it to do nothing in IE - this change handles that particular case (we already do this for height/width, for example - negative numbers are turned into 0).

@jdalton
Copy link
Member

@jdalton jdalton commented on 2ca3659 Oct 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeresig - Right I get that (and auto converting rgb would be fine). I just don't dig swallowing all potential errors :/

@jdalton
Copy link
Member

@jdalton jdalton commented on 2ca3659 Oct 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note:

@jdalton: No - it's the same as if you apply an unknown style in any other browser. .css("foobar", "baz") (we don't throw an exception).

Actually that doesn't seem to error in any browser I have recently tested with or without the try-catch (before this addition if it did error, jQuery would have errored too).

The difference is background-color is a known property backed by the w3c spec that should allow rgb (css2) and possibly rgba (css3). Instead of punting (swallowing the error) you could add the functionality for cross-browser consistency or disallow the syntax for all browsers continuing with consistency or allow it to error so devs can use that as a feature test and fork to add the functionality if they want it.

@jeresig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton: Nothing that we're doing is preventing a user from writing a feature test. In fact the original version of this patch had support for jQuery.support.rgba: http://github.com/csnover/jquery/commit/824fb5607a5ce4a844c1db68bcc6b8d193c14499 (I opted not to include it as - and with the case of every support test in jQuery - we need to use it in core). In jQuery 1.4.3 we've added in a new extensible CSS architecture which allows anyone to develop plugins to handle the oddball CSS properties - and potentially provide backwards compatibility for older browsers (allowing people to implement background position fixes, background-color fixes, etc.).

@jdalton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh 'jQuery.cssHooks'. Nice!

@krisselden
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is an old thread, but the try/catch deopts the entire style function which is used in animations, the try/catch should be extracted into its own function.

This is still in the 1.9.x branch.

Please sign in to comment.