Fix return value of elem.css('background-position')

[rewish]

Fixed the undefined value in "IE<9", And fixed a background-position bug in IE9.

Details of the background-position bug in IE9:

1. Set the background-position value by non-inline CSS
2. Set the background-* value
3. The background-position value is changed to "0% 0%"

<style>
#foo {
  background-position: 10px 20px;
}
</style>

<div id="foo"></div>

<script>
var foo = jQuery( "#foo" );
alert( foo.css( "background-position" ) ); // 10px 20px
foo.css( "background-color", "#FFF" );
alert( foo.css( "background-position" ) ); // 0% 0% !!!
</script>

[rwaldron]

This issue was addressed in a ticket on the bug tracker and a valid patch with many tests was landed here 6aa4095ed62e3e37dae4c39c00fb627a3b282307

This looks very special case and jQuery has decided that it will no longer land special case code for oldIEs - can you explain the improvement or benefit over the previous patch?

[rewish]

I think it would cause problem due to a background-position bug in IE9.

In such cases, for example:

var foo = jQuery("#foo"),
    bar = jQuery("#bar");
bar.css( "background-color", "#FFF" );
foo.css( "background-position", bar.css("background-position") || [
  bar.css("background-position-x"),
  bar.css("background-position-y")
].join(" ") );

The background-position value of foo is "0% 0%".

[mikesherov]

@rewish, have you tested this in all browsers? http://bugs.jquery.com/ticket/9621#comment:1

" Not all browsers support backgroundPositionX/Y.... "

[mikesherov]

It may be that they all support it now though, just want to make sure.

[dmethvin]

I think we've been consistent with this. Shorthand properties are supported as setters but not as getters. So why is this an exception? If we go through the trouble of synthesizing two string values separated by spaces, you'll often need to split them up to process X and Y separately. In the case you've mentioned there, copying the values, it would be just as easy to individually get/set the X/Y position.

[mikesherov]

@dmethvin, I've also been trying to find it in the bug tracker, but I know there have been a few wontfix's related exactly to this, for the exact reasons you've already mentioned.

[rwaldron]

@dmethvin it happens so infrequently that I actually forgot. That seems like reason enough to close.

[rewish]

I'm sorry you guys for giving misleading to background-position. That's not important. I'd like to say is that IE9 returns wrong value(0% 0%) , not undefined. This bug could be critical when debugging. It's hard to notice because of the wrong value. This pull request influences wide range of the code. So if you don't like it, we could suggest different ideas. Thanks.

// Completely fix the failed test cases for old browsers if it's merged!

[dmethvin]

@rewish, it doesn't matter that .css("background-position") is broken because getting shorthand properties is a case we don't support; the same goes for .css("border") for example. If you want the background position, the correct way to do it would be do get the individual values as you are doing in the patch. Or do you have a test case that indicates that way doesn't work?

[dmethvin]

I think I may be off base a bit; although background-position does take two args @timmywil is right that not all browsers support breaking out the x/y properties (IE and Webkit do though). So technically it's not the same as .css("border") and may not be considered a shorthand property. Yet CSS3 is heading towards even more of a mess that turns it into a shorthand property without breakouts, I am not sure how we will handle this: https://bugzilla.mozilla.org/show_bug.cgi?id=522607

[rewish]

@dmethvin Will be 0% 0% when you open the Test Case in IE9

I fixed the cssHooks and test case. In IE<9, undefined values return. In IE9, values return correctly. For the others, no influences.

Also supported CSS3.

Try it!

[mikesherov]

i hate background position so much... anyway, this is lots of code for an edge case. If it could be significantly reduced, i may support it, but i don't want to lead you down that path, @rewish, without some more input from others.

I'll go ahead and leave notes in the diff, but please don't take that as a sign that the pull will be accepted.

[rewish]

@mikesherov Thanks for the advice. You mean that I should adopt ideas from other people, right? and also I'd like to ask you that what part is for an edge case or what part do you want to cut? Just let me make sure.

I'd like to improve !window.attachEvent. What do you think of that?

[mikesherov]

@rewish, I meant that this code change, while addressing an issue, addresses an issue that might not happen a lot in the wild. jQuery tries to remain small by weighing the frequency of a bug against the amount of code it takes to fix it.

In this case, it's a lot of code for a small problem, and I want to make sure the other jQuery team members are on board with possibly accepting this fix into the codebase before I suggest further ways to reduce the size of the code.

[rewish]

@mikesherov Now got it and agree with you. well, I'll wait for the decision from jQuery team.

[mikesherov]

@rewish, I spoke to @dmethvin about this pull request, and if we can make this change small enough, we can get it pulled in. I'll go ahead and make some suggestions to see if we can get this to be much smaller.

[mikesherov]

I made a lot of line notes. I'd also just like to say that it seems as if we can save more bytes by doing this stuff in a loop, and by caching the string backgroundPosition into a variable.

[rwaldron]

This needs a rebase and a final decision on inclusion. @mikesherov - can you summarize your thoughts about this patch?

[mikesherov]

I'm willing to take this one home with @rewish, but it needs some work and commitment from @rewish to make the final changes. It's a pretty big edge case at the moment, but I was willing to work through it so we could have another contributor! At this point it's pretty stagnant. I'm fine to close, log as a bug, and revisit at a later date if @rewish doesn't want to continue optimizing this to it's smallest possible size.

[rwaldron]

Thanks @mikesherov that's exactly what I needed to know - let's give @rewish some time to respond before closing

[rewish]

@mikesherov Thanks line notes! and I'm sorry for being late.

I want to commit a fixes I can think of. Log as a bug and close if you do not like a fixes.

Thank you.

[mikesherov]

@rewish, thanks for updating the pull request. I'll let you know shortly if I have any further comments or requests.

[rwaldron]

@rewish did you rebase? I'm still getting conflicts - I'll wait on this...

[rewish]

I'm sorry did not rebase. I re-push after rebase.

[mikesherov]

@rewish, thanks again for doing this work. What is the size diff of this compared to master now? My only other thoughts on how to make this any smaller would be:

• Doing the "X" and "Y" work in a loop, so essentially two loops.
• changing elem.style.backgroundPosition to elem.style[propname]
• changing ++i to i++, believe it or not, this will probably help gzip
• initializing i to 0 in the initial var declaration.

We're looking for smallest size here. I'm afraid that it won't get small enough to justify inclusion. Let me know what the smallest size diff is using these tactics, and we be able to tell @rwldrn whether or not he can merge this in.

Again, thanks for your patience and diligence!

[rewish]

@mikesherov Thank you too and thanks for advices!

Let me ask you some questions.

Doing the "X" and "Y" work in a loop, so essentially two loops.

What do you mean?

changing ++i to i++, believe it or not, this will probably help gzip

Wow! really? Please let me know if there is a page where you can see!

Size in 0cc3201

Running "compare_size:files" (compare_size) task
Sizes - compared to master
    252010      (+634)  dist/jquery.js                                         
     92315      (+292)  dist/jquery.min.js                                     
     33282      (+110)  dist/jquery.min.js.gz                                  

[mikesherov]

So, I think to wrap up this discussion, I tinkered with the good work you have here @rewish, and I just tried to see if I could get it to be as small as possible, without running an exhaustive set of unit tests against it... I landed on:

        backgroundPosition: {
            get: function( elem, computed ) {
                if ( computed ) {

                    var posY,
                        i = 0,
                        propName = "backgroundPosition",
                        ret = curCSS( elem, propName );

                    if ( ret !== "0% 0%" ) {
                        return ret;
                    }

                    ret = curCSS( elem, propName + "X" ).split( ", " );
                    posY = curCSS( elem, propName + "Y" ).split( ", " );

                    for ( ; i < ret.length; i++ ) {
                        ret[ i ] += " " + posY[ i ];
                    }

                    return ret.join( ", " );
                }
            }
        }

Even at this point, it's still +83 against current master, which just seems a bit too big for inclusion. I welcome you to file this as a bug at http://bugs.jquery.com

On top of that, if you're interested in working on other bugs, there are plenty to work through at http://bugs.jquery.com, just make sure you read all of the instructions in the README at http://github.com/jquery/jquery so whatever work you do has the best chance for inclusion.

I just want to reiterate that even though this pull request won't get merged, I appreciate the hard work and effort you put into it. Feel free to reach out to me if you have any other questions.

[rwaldron]

@mikesherov is there any benefit to storing ", " in a variable?

[mikesherov]

@rwldrn, no, that's +8 bytes, actually. In cases of small strings repeated <=3 times in close proximity, gzip will favor the repeated string over having another var to compress.

[rwaldron]

Thanks for looking into that :)

[rewish]

@mikesherov Thanks for smaller code.

I'll propose to waste all discussion. Seems like solved by using document.documentElement.currentStyle, because this bug is caused by document.defaultView.getComputedStyle.

Reverse the conditional expression for curCSS:

if ( document.documentElement.currentStyle ) {
    curCSS = Using to document.documentElement.currentStyle
} else if ( document.defaultView && document.defaultView.getComputedStyle ) {
    curCSS = Using to document.defaultView.getComputedStyle
}

But, I have not tested this code on all browsers. Please tell me if there is a problem.

[mikesherov]

@rewish, if you'd like to try this implementation, please run the test suite in all of our supported browsers, and report back if it passes. Thanks again!

[dmethvin]

Ouch, this is still +113. Does it seem important enough for that?

[mikesherov]

This is not mergeable as is. @rewish had a different fix he was going to try, but this PR is now invalid.

[dmethvin]

Okay, I'll close it.