Fix camelCasing properties for IE ( see #93 ) #94

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

mibalan commented Jul 24, 2012

Inspired from jQuery's own camelCase() - in IE, properties get camelCased as 'msPropertyName', not 'MsPropertyName'

Owner

LeaVerou commented Jul 24, 2012

I’m afraid that won’t work either. IE supports some properties with ms and some others with Ms, so we need to feature test which one is actually supported. Feature testing for properties is simple, i.e. propertyName in someElement.style.

Also, if we fix that issue, we better not make it IE specific, other browsers could eventually run into this.

The way I’d fix it would be:

  1. Create both camelCased properties (with and without leading hyphen)
  2. Check which one is supported
  3. Return that one.

mibalan commented Jul 25, 2012

Makes a lot of sense :)
I'll give it a try sometime today.

mibalan commented Jul 26, 2012

OK, looks like it's even nastier than that. When trying to read the CSS of an element in jQuery, the property-name is de-camel-cased . As such, if the camel-cased property name uses the lowercase prefix it will be get de-camel-cased to a property name that does not begin with a hyphen. And from there, all hell breaks loose (as WebKit supports both variants - setting webkitPropertyName and reading WebkitPropertyName).

mibalan commented Jul 26, 2012

OK, looks like if we favor uppercase prefixes, jQuery won't break. Not entirely sure how they cope with ms/Ms prefixes, but seems solid.

Should be ready to merge :)

Owner

LeaVerou commented Jul 26, 2012

Hi there,

Thanks for your continued efforts on this.

Some small remarks:

  • If I merge this now, it will add 4 commits to the history, for this small change. In addition, the incremental changes make it very hard for me to review the code before merging. Would it be too much to ask you to send a new pull request with all the changes in one commit?
  • It’s not a good idea to create a new div for every property you test. Create it once and check all properties on on it. I think there’s already one there anyway.
  • Of course we will favor properly CamelCased prefixes, not because otherwise we'd break jQuery, but because it’s how they should be. the lowercase ones are just a temporary workaround.

mibalan commented Jul 27, 2012

OK, I'll looks for the already created div and squash the commits into one and then come back :)

mibalan commented Jul 27, 2012

Hopefully, this is it!

  • Used document.documentElement instead of creating a new <div/>. There was one laying around, but in a different closure, didn't want to mess with it. Besides, document.documentElement seems to do the job nicely
  • Squished the 5 revisions into one
  • Removed comments about breaking jQuery :)
Owner

LeaVerou commented Aug 6, 2012

Hi,

Sorry for the delay, I'm swamped these days.

Some comments:

  • Please follow the formatting conventions of the rest of the code. It seems you've used two spaces instead of one tab for indentation.
  • dashPrefixed could be simply prefixed.
  • When you have something of the format:
if (x) {
    return y
} else {
    return z
}

it's much simpler and shorter to just write

return x? y : z;
  • Personally, I think prefixedUpper makes more sense than upperPrefixed but that might just be personal preference. Perhaps you could even play with the idea of what's happening and use prefixedCamelCase and PrefixedCamelCase. But those are a bit too long.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment