no need to dasherize and use getPropertyValue as all invocations of curCSS are already camelcased! #805

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Member

mikesherov commented May 31, 2012

Running "compare_size:files" (compare_size) task
Sizes - compared to master
    250875      (-223)  dist/jquery.js                                         
     91944      (-101)  dist/jquery.min.js                                     
     33111       (-59)  dist/jquery.min.js.gz              
Owner

dmethvin commented May 31, 2012

Wow.

Member

gibson042 commented May 31, 2012

Niiice.

Member

markelog commented May 31, 2012

You sure?
Example from doc – http://api.jquery.com/css/

Internet Explorer's DOM implementation refers to the float property as styleFloat, while W3C standards-compliant browsers refer to it as cssFloat. The .css() method accounts for such differences, producing the same result no matter which term we use. For example, an element that is floated left will return the string left for each of the following three lines:
$('div.left').css('float');
$('div.left').css('cssFloat');
$('div.left').css('styleFloat');

Third case is already broken since jQuery 1.5, for not ie < 9, interestingly enough, second is broken too, for all browsers since jQuery 1.5, for not ie < 9, it was returning empty string, for ie < 9 it was undefined, but it was fixed in 1.6. With this pull, second case will be broken once again, but now only for ie < 9.

Either it should be fixed, or doc should be updated, personally, i think second scenario should be applied, fact that third case was broken along time ago, means that no one used it, presumably, for the second case it's the same story.

Member

mikesherov commented May 31, 2012

http://jsfiddle.net/mgjBW/

So basically, "float" already gets translated by cssProps to "cssFloat", which exists as a property on getComputedStyle. The reason we had to translate from "cssFloat" to "float" weirdly was because getPropertyValue("cssFloat") would fail, even though .cssFloat exists! So in fact, I haven't broken case 2.

With regards to oldIE, the getComputedStyle code path isn't executed.

I had the same concerns as you @Orkel, and I appreciate the second set of eyes, but this PR addresses that issue.

Member

mikesherov commented May 31, 2012

I suppose I can fix styleFloat too, but that's not something I broke with this PR.

Member

mikesherov commented May 31, 2012

I re-read your post @Orkel more carefully now. You're saying that I broke ('cssFloat') for IE<9. That's interesting, and wasn't something I was sure we supported. I suppose I can conditionally add two more cssProps to translate cssFloat to styleFloat in IE<9, and styleFloat to cssFloat elsewhere.

Owner

dmethvin commented May 31, 2012

I suspect most people are just using float and don't even realize we do that mapping behind the scenes. I would prefer changing the docs.

Member

mikesherov commented Jun 1, 2012

K, just so you know, I can squeeze the cssFloat and styleFloat conversions in for 11 bytes. I'm fine either way. Make the call, @dmethvin.

dmethvin closed this in 888635f Jun 1, 2012

Owner

dmethvin commented Jun 1, 2012

I've simplified the docs to just recommend the consistent use of "float", which is no doubt what most people are doing anyway.

@markelog markelog pushed a commit to markelog/jquery that referenced this pull request Jun 29, 2012

@mikesherov @dmethvin mikesherov + dmethvin No need to dasherize and use getPropertyValue, closes gh-805.
All invocations of curCSS are already camelcased. See the PR for discussion.
1c657d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment