Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 6 commits into from

5 participants

@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>
src/css.js
@@ -42,6 +42,16 @@ jQuery.extend({
return elem.style.opacity;
}
}
+ },
+
+ backgroundPosition: {
+ get: function( elem ) {
+ var ret = [
+ curCSS( elem, "backgroundPositionX" ),
+ curCSS( elem, "backgroundPositionY" )
+ ];
+ return ret[0] && ret[1] ? ret.join(' ') : undefined;
@rwaldron Collaborator

No single quotes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rwaldron
Collaborator

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

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
Collaborator

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

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

@mikesherov
Collaborator

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

@dmethvin
Owner

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
Collaborator

@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
Collaborator

@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
Owner

@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
Owner

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
Collaborator

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.

src/css.js
((7 lines not shown))
+ get: function( elem, computed ) {
+ if ( !computed ) {
+ return elem.style.backgroundPosition;
+ }
+
+ var ret = curCSS( elem, "backgroundPosition" ),
+ posX, posY, i = 0, len;
+
+ if ( ret !== "0% 0%" || !window.attachEvent ) {
+ return ret;
+ }
+
+ ret = [];
+ posX = curCSS( elem, "backgroundPositionX" );
+ posY = curCSS( elem, "backgroundPositionY" );
+ glue = rmultiplebg.exec( posX );
@mikesherov Collaborator

did you run this through jshint? glue is leaking to global. also, can't you do:

glue = rmultiplebg.exec( posX ) || [""]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/css.js
((15 lines not shown))
+ if ( ret !== "0% 0%" || !window.attachEvent ) {
+ return ret;
+ }
+
+ ret = [];
+ posX = curCSS( elem, "backgroundPositionX" );
+ posY = curCSS( elem, "backgroundPositionY" );
+ glue = rmultiplebg.exec( posX );
+ posX = posX.split( rmultiplebg );
+ posY = posY.split( rmultiplebg );
+
+ for ( len = posX.length; i < len; ++i ) {
+ ret[i] = [posX[i], posY[i]].join(" ");
+ }
+
+ return glue ? ret.join(glue[0]) : ret.join("");
@mikesherov Collaborator

with the change i proposed on L64, you can just do:

return ret.join(glue[0]);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@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
Collaborator

@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.

src/css.js
@@ -42,6 +43,34 @@ jQuery.extend({
return elem.style.opacity;
}
}
+ },
+
+ backgroundPosition: {
+ get: function( elem, computed ) {
+ if ( !computed ) {
+ return elem.style.backgroundPosition;
+ }
+
+ var ret = curCSS( elem, "backgroundPosition" ),
+ posX, posY, glue, i = 0, len;
+
+ if ( ret !== "0% 0%" || !window.attachEvent ) {
@gnarf Owner
gnarf added a note

What does window.attachEvent have to do with this? Surely there is a better support test for this?

@rewish
rewish added a note

window.attachEvent will ignore a browser other than IE, but I think this code is should be improved.

@mikesherov Collaborator

seems like a test for elem.style.backgroundPositionX would be better than a test for window.attachEvent, right?

@rewish
rewish added a note

What about a test for document.documentMode === 9? it would also work.

@dmethvin Owner

Those kind of tests are not good ones in general, because they are unrelated to the problem being solved. What if IE10 has this problem as well? Some test that checks for the broken condition is best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mikesherov
Collaborator

@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.

src/css.js
@@ -42,6 +43,34 @@ jQuery.extend({
return elem.style.opacity;
}
}
+ },
+
+ backgroundPosition: {
+ get: function( elem, computed ) {
+ if ( !computed ) {
+ return elem.style.backgroundPosition;
+ }
+
+ var ret = curCSS( elem, "backgroundPosition" ),
+ posX, posY, glue, i = 0, len;
@rwaldron Collaborator

Move the initialized, but not assigned to their own line - thanks.

var posX, posY, glue, len, 
    ret = curCSS( elem, "backgroundPosition" ),
    i = 0;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/css.js
((12 lines not shown))
+ var ret = curCSS( elem, "backgroundPosition" ),
+ posX, posY, glue, i = 0, len;
+
+ if ( ret !== "0% 0%" || !window.attachEvent ) {
+ return ret;
+ }
+
+ ret = [];
+ posX = curCSS( elem, "backgroundPositionX" );
+ posY = curCSS( elem, "backgroundPositionY" );
+ glue = rmultiplebg.exec( posX ) || [""];
+ posX = posX.split( rmultiplebg );
+ posY = posY.split( rmultiplebg );
+
+ for ( len = posX.length; i < len; ++i ) {
+ ret[i] = [posX[i], posY[i]].join(" ");
@rwaldron Collaborator

[ posX[i], posY[i] ]

@mikesherov Collaborator

why not just ret[i] = posX[ i ] + " " + posY[ i ];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/css.js
((15 lines not shown))
+ if ( ret !== "0% 0%" || !window.attachEvent ) {
+ return ret;
+ }
+
+ ret = [];
+ posX = curCSS( elem, "backgroundPositionX" );
+ posY = curCSS( elem, "backgroundPositionY" );
+ glue = rmultiplebg.exec( posX ) || [""];
+ posX = posX.split( rmultiplebg );
+ posY = posY.split( rmultiplebg );
+
+ for ( len = posX.length; i < len; ++i ) {
+ ret[i] = [posX[i], posY[i]].join(" ");
+ }
+
+ return ret.join(glue[0]);
@rwaldron Collaborator

return ret.join( glue[0] );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/css.js
((6 lines not shown))
+ backgroundPosition: {
+ get: function( elem, computed ) {
+ if ( !computed ) {
+ return elem.style.backgroundPosition;
+ }
+
+ var ret = curCSS( elem, "backgroundPosition" ),
+ posX, posY, glue, i = 0, len;
+
+ if ( ret !== "0% 0%" || !window.attachEvent ) {
+ return ret;
+ }
+
+ ret = [];
+ posX = curCSS( elem, "backgroundPositionX" );
+ posY = curCSS( elem, "backgroundPositionY" );
@mikesherov Collaborator

posY = curCSS( elem, "backgroundPositionY" ).split( rmultiplebg );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/css.js
((9 lines not shown))
+ return elem.style.backgroundPosition;
+ }
+
+ var ret = curCSS( elem, "backgroundPosition" ),
+ posX, posY, glue, i = 0, len;
+
+ if ( ret !== "0% 0%" || !window.attachEvent ) {
+ return ret;
+ }
+
+ ret = [];
+ posX = curCSS( elem, "backgroundPositionX" );
+ posY = curCSS( elem, "backgroundPositionY" );
+ glue = rmultiplebg.exec( posX ) || [""];
+ posX = posX.split( rmultiplebg );
+ posY = posY.split( rmultiplebg );
@mikesherov Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mikesherov
Collaborator

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
Collaborator

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

@mikesherov
Collaborator

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
Collaborator

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
Collaborator

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

@rwaldron
Collaborator

@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
Collaborator

@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 mikesherov commented on the diff
src/css.js
@@ -139,6 +141,33 @@ jQuery.extend({
return elem.style.opacity;
}
}
+ },
+
+ backgroundPosition: {
+ get: function( elem, computed ) {
+ if ( !computed ) {
+ return elem.style.backgroundPosition;
@mikesherov Collaborator

over here you can just say return; without a value, and it'll know to look up the property normally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mikesherov
Collaborator

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
Collaborator

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

@mikesherov
Collaborator

@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
Collaborator

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
Collaborator

@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
Owner

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

@mikesherov
Collaborator

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

@dmethvin
Owner

Okay, I'll close it.

@dmethvin dmethvin closed this
@getdave getdave referenced this pull request in markdalgleish/stellar.js
Open

IE 8 background-position issue in 0.6.2 had to rever to 0.4.0 #34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 8, 2012
  1. @rewish

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

    rewish authored
    Always return the background-position value.
  2. @rewish

    Fix single quotes to double quotes

    rewish authored
    "jQuery Core Style Guidelines" violations
  3. @rewish
  4. @rewish

    Fix leaking to global

    rewish authored
  5. @rewish
  6. @rewish

    Optimize code size

    rewish authored
This page is out of date. Refresh to see the latest.
Showing with 45 additions and 0 deletions.
  1. +29 −0 src/css.js
  2. +16 −0 test/unit/css.js
View
29 src/css.js
@@ -8,7 +8,9 @@ var curCSS, iframe, iframeDoc,
rnumnonpx = /^-?(?:\d*\.)?\d+(?!px)[^\d\s]+$/i,
rrelNum = /^([\-+])=([\-+.\de]+)/,
rmargin = /^margin/,
+ rmultiplebg = /,\s?/,
elemdisplay = {},
+
cssShow = { position: "absolute", visibility: "hidden", display: "block" },
cssExpand = jQuery.cssExpand,
@@ -139,6 +141,33 @@ jQuery.extend({
return elem.style.opacity;
}
}
+ },
+
+ backgroundPosition: {
+ get: function( elem, computed ) {
+ if ( !computed ) {
+ return elem.style.backgroundPosition;
@mikesherov Collaborator

over here you can just say return; without a value, and it'll know to look up the property normally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+
+ var posY, glue, i, len,
+ propName = "backgroundPosition",
+ ret = curCSS( elem, propName );
+
+ if ( ret !== "0% 0%" ) {
+ return ret;
+ }
+
+ ret = curCSS( elem, propName + "X" );
+ posY = curCSS( elem, propName + "Y" ).split( rmultiplebg );
+ glue = rmultiplebg.exec( ret ) || [""];
+ ret = ret.split( rmultiplebg );
+
+ for ( i = 0, len = ret.length; i < len; ++i ) {
+ ret[i] += " " + posY[i];
+ }
+
+ return ret.join( glue[0] );
+ }
}
},
View
16 test/unit/css.js
@@ -779,3 +779,19 @@ test( "cssHooks - expand", function() {
});
});
+
+test( "css('backgroundPosition') should be valid value", function() {
+ jQuery( "<style type='text/css'>" +
+ "#test-backgroundPosition {background-position: 10px 20px;}" +
+ "#test-backgroundPositionCSS3 {background-position: 10px 20px, 30px 40px;}" +
+ "</style>" ).appendTo( "head" );
+
+ jQuery( "<div id='test-backgroundPosition' style='background-color:#FFF' />" ).appendTo( "#qunit-fixture" );
+ notEqual( jQuery( "#test-backgroundPosition" ).css( "backgroundPosition" ), "0% 0%", "css('background-position') expands properly with not 0% 0%" );
+
+ // This test case is only supported in IE9
+ if ( document.documentMode && document.documentMode === 9 ) {
+ jQuery( "<div id='test-backgroundPositionCSS3' style='background-color:#FFF' />" ).appendTo( "#qunit-fixture" );
+ equal( jQuery( "#test-backgroundPositionCSS3" ).css( "backgroundPosition" ), "10px 20px, 30px 40px", "css('background-position') of CSS3 expands properly with 10px 20px, 30px 40px" );
+ }
+});
Something went wrong with that request. Please try again.