Skip to content

Commit

Permalink
fix(jqLite): css should convert dash-separated properties to camelCase
Browse files Browse the repository at this point in the history
this fix is needed for Firefox or other browsers that strictly follow
dom/css spec which states that element.style should make properties
available in camelCased form.

Closes angular#569
  • Loading branch information
IgorMinar committed Sep 27, 2011
1 parent 084b83f commit d4226f2
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 2 deletions.
35 changes: 33 additions & 2 deletions src/jqLite.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,30 @@ function getStyle(element) {
return current;
}


/**
* Converts dash-separated names to camelCase. Useful for dealing with css properties.
*/
function camelCase(name) {
var parts = name.split('-');

This comment has been minimized.

Copy link
@mhevery

mhevery Sep 28, 2011

change to this as it is more terse.

function camelCase(string){
return string.replace(/\D\W/g, function(all, letter){ return letter.toUpperCase(); });
}

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Sep 28, 2011

Author Owner

this didn't work, but I did something similar. thanks for pointing this out.

This comment has been minimized.

Copy link
@mhevery

mhevery Sep 28, 2011

what did not work? I tested it and it worked for me.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar via email Sep 28, 2011

Author Owner

name = parts.shift();

if (name == '' && parts.length) {
// browser specific property starting with "-"
name = parts.shift();

if (name == 'moz') name = 'Moz';
}

forEach(parts, function(part) {
name += part.charAt(0).toUpperCase();
name += part.substr(1);
});

return name;
}

/////////////////////////////////////////////
function jqLiteWrap(element) {
if (isString(element) && element.charAt(0) != '<') {
Expand Down Expand Up @@ -247,20 +271,27 @@ forEach({
hasClass: JQLiteHasClass,

css: function(element, name, value) {
name = camelCase(name);

if (isDefined(value)) {
element.style[name] = value;
} else {
var val;

if (msie <=8) {
if (msie <= 8) {
// this is some IE specific weirdness that jQuery 1.6.4 does not sure why
val = element.currentStyle && element.currentStyle[name];
if (val === '') val = 'auto';
}

val = val || element.style[name];

return (val === '') ? undefined : val;
if (msie <= 8) {

This comment has been minimized.

Copy link
@esprehn

esprehn Sep 29, 2011

I thought you didn't support IE less than 8 now? There was a comment recently about this on the list.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Sep 29, 2011

Author Owner

it's <= not <

// jquery weirdness :-/
val = (val === '') ? undefined : val;
}

return val;
}
},

Expand Down
42 changes: 42 additions & 0 deletions test/jqLiteSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,25 @@ describe('jqLite', function(){
expect(jqLite(a).css('padding')).toBe('2px');
expect(jqLite(a).css('border')).toBeFalsy();
});


it('should correctly handle dash-separated and camelCased properties', function() {
var jqA = jqLite(a);

expect(jqA.css('z-index')).toBeOneOf('', 'auto');
expect(jqA.css('zIndex')).toBeOneOf('', 'auto');


jqA.css({'zIndex':5});

expect(jqA.css('z-index')).toBeOneOf('5', 5);
expect(jqA.css('zIndex')).toBeOneOf('5', 5);

jqA.css({'z-index':7});

expect(jqA.css('z-index')).toBeOneOf('7', 7);
expect(jqA.css('zIndex')).toBeOneOf('7', 7);
});
});


Expand Down Expand Up @@ -747,4 +766,27 @@ describe('jqLite', function(){
expect(element.find('span').eq(20).length).toBe(0);
});
});


describe('camelCase', function() {

it('should leave non-dashed strings alone', function() {
expect(camelCase('foo')).toBe('foo');
expect(camelCase('')).toBe('');
expect(camelCase('fooBar')).toBe('fooBar');
});


it('should covert dash-separated strings to camelCase', function() {
expect(camelCase('foo-bar')).toBe('fooBar');
expect(camelCase('foo-bar-baz')).toBe('fooBarBaz');
});


it('should covert browser specific css properties', function() {
expect(camelCase('-moz-foo-bar')).toBe('MozFooBar');
expect(camelCase('-webkit-foo-bar')).toBe('webkitFooBar');
expect(camelCase('-webkit-foo-bar')).toBe('webkitFooBar');
})
});
});

2 comments on commit d4226f2

@esprehn
Copy link

Choose a reason for hiding this comment

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

The motivation of this feature feels wrong. You should always specify the ng:style properties in camel case, not dashed form. It's the difference between working in the object world vs the string world. element.style['border-color'] doesn't work in compliant browsers and ng:style="{'border-color': '...'}" shouldn't work either.

That's not to say that jqLite shouldn't be compliant with jQuery, but expecting this to work in ng:style seems wrong.

@IgorMinar
Copy link
Owner Author

Choose a reason for hiding this comment

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

to be honest, I didn't realize that you were supposed to use element.style with camelCased properties until I looked into this bug. I think it's quite surprising and unexpected for most developers. what however made me implement this fix is that jquery already does this. so you should think of this as jqlite regression fix rather than ng:style fix.

Please sign in to comment.