Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

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

Closed
wants to merge 6 commits into from

5 participants

Hiroshi Hoaki Rick Waldron Mike Sherov Dave Methvin Corey Frang
Hiroshi Hoaki

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({
42 42
 					return elem.style.opacity;
43 43
 				}
44 44
 			}
  45
+		},
  46
+
  47
+		backgroundPosition: {
  48
+			get: function( elem ) {
  49
+				var ret = [
  50
+					curCSS( elem, "backgroundPositionX" ),
  51
+					curCSS( elem, "backgroundPositionY" )
  52
+				];
  53
+				return ret[0] && ret[1] ? ret.join(' ') : undefined;
1
Rick Waldron Collaborator
rwaldron added a note April 21, 2012

No single quotes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Rick Waldron
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?

Hiroshi Hoaki

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%".

Mike Sherov
Collaborator

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

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

Mike Sherov
Collaborator

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

Dave Methvin
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.

Mike Sherov
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.

Rick Waldron
Collaborator

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

Hiroshi Hoaki

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!

Dave Methvin
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?

Dave Methvin
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

Hiroshi Hoaki

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

Mike Sherov
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))
  49
+			get: function( elem, computed ) {
  50
+				if ( !computed ) {
  51
+					return elem.style.backgroundPosition;
  52
+				}
  53
+
  54
+				var ret = curCSS( elem, "backgroundPosition" ),
  55
+					posX, posY, i = 0, len;
  56
+
  57
+				if ( ret !== "0% 0%" || !window.attachEvent ) {
  58
+					return ret;
  59
+				}
  60
+
  61
+				ret = [];
  62
+				posX = curCSS( elem, "backgroundPositionX" );
  63
+				posY = curCSS( elem, "backgroundPositionY" );
  64
+				glue = rmultiplebg.exec( posX );
1
Mike Sherov Collaborator
mikesherov added a note April 26, 2012

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))
  57
+				if ( ret !== "0% 0%" || !window.attachEvent ) {
  58
+					return ret;
  59
+				}
  60
+
  61
+				ret = [];
  62
+				posX = curCSS( elem, "backgroundPositionX" );
  63
+				posY = curCSS( elem, "backgroundPositionY" );
  64
+				glue = rmultiplebg.exec( posX );
  65
+				posX = posX.split( rmultiplebg );
  66
+				posY = posY.split( rmultiplebg );
  67
+
  68
+				for ( len = posX.length; i < len; ++i ) {
  69
+					ret[i] = [posX[i], posY[i]].join(" ");
  70
+				}
  71
+
  72
+				return glue ? ret.join(glue[0]) : ret.join("");
1
Mike Sherov Collaborator
mikesherov added a note April 26, 2012

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
Hiroshi Hoaki

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

Mike Sherov
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.

Hiroshi Hoaki

@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({
42 43
 					return elem.style.opacity;
43 44
 				}
44 45
 			}
  46
+		},
  47
+
  48
+		backgroundPosition: {
  49
+			get: function( elem, computed ) {
  50
+				if ( !computed ) {
  51
+					return elem.style.backgroundPosition;
  52
+				}
  53
+
  54
+				var ret = curCSS( elem, "backgroundPosition" ),
  55
+					posX, posY, glue, i = 0, len;
  56
+
  57
+				if ( ret !== "0% 0%" || !window.attachEvent ) {
5
Corey Frang Owner
gnarf added a note May 04, 2012

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

Hiroshi Hoaki
rewish added a note May 05, 2012

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

Mike Sherov Collaborator
mikesherov added a note May 25, 2012

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

Hiroshi Hoaki
rewish added a note May 25, 2012

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

Dave Methvin Owner
dmethvin added a note May 25, 2012

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
Mike Sherov
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({
42 43
 					return elem.style.opacity;
43 44
 				}
44 45
 			}
  46
+		},
  47
+
  48
+		backgroundPosition: {
  49
+			get: function( elem, computed ) {
  50
+				if ( !computed ) {
  51
+					return elem.style.backgroundPosition;
  52
+				}
  53
+
  54
+				var ret = curCSS( elem, "backgroundPosition" ),
  55
+					posX, posY, glue, i = 0, len;
1
Rick Waldron Collaborator
rwaldron added a note May 25, 2012

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))
  54
+				var ret = curCSS( elem, "backgroundPosition" ),
  55
+					posX, posY, glue, i = 0, len;
  56
+
  57
+				if ( ret !== "0% 0%" || !window.attachEvent ) {
  58
+					return ret;
  59
+				}
  60
+
  61
+				ret = [];
  62
+				posX = curCSS( elem, "backgroundPositionX" );
  63
+				posY = curCSS( elem, "backgroundPositionY" );
  64
+				glue = rmultiplebg.exec( posX ) || [""];
  65
+				posX = posX.split( rmultiplebg );
  66
+				posY = posY.split( rmultiplebg );
  67
+
  68
+				for ( len = posX.length; i < len; ++i ) {
  69
+					ret[i] = [posX[i], posY[i]].join(" ");
2
Rick Waldron Collaborator
rwaldron added a note May 25, 2012

[ posX[i], posY[i] ]

Mike Sherov Collaborator
mikesherov added a note May 25, 2012

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))
  57
+				if ( ret !== "0% 0%" || !window.attachEvent ) {
  58
+					return ret;
  59
+				}
  60
+
  61
+				ret = [];
  62
+				posX = curCSS( elem, "backgroundPositionX" );
  63
+				posY = curCSS( elem, "backgroundPositionY" );
  64
+				glue = rmultiplebg.exec( posX ) || [""];
  65
+				posX = posX.split( rmultiplebg );
  66
+				posY = posY.split( rmultiplebg );
  67
+
  68
+				for ( len = posX.length; i < len; ++i ) {
  69
+					ret[i] = [posX[i], posY[i]].join(" ");
  70
+				}
  71
+
  72
+				return ret.join(glue[0]);
1
Rick Waldron Collaborator
rwaldron added a note May 25, 2012

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))
  48
+		backgroundPosition: {
  49
+			get: function( elem, computed ) {
  50
+				if ( !computed ) {
  51
+					return elem.style.backgroundPosition;
  52
+				}
  53
+
  54
+				var ret = curCSS( elem, "backgroundPosition" ),
  55
+					posX, posY, glue, i = 0, len;
  56
+
  57
+				if ( ret !== "0% 0%" || !window.attachEvent ) {
  58
+					return ret;
  59
+				}
  60
+
  61
+				ret = [];
  62
+				posX = curCSS( elem, "backgroundPositionX" );
  63
+				posY = curCSS( elem, "backgroundPositionY" );
1
Mike Sherov Collaborator
mikesherov added a note May 25, 2012

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))
  51
+					return elem.style.backgroundPosition;
  52
+				}
  53
+
  54
+				var ret = curCSS( elem, "backgroundPosition" ),
  55
+					posX, posY, glue, i = 0, len;
  56
+
  57
+				if ( ret !== "0% 0%" || !window.attachEvent ) {
  58
+					return ret;
  59
+				}
  60
+
  61
+				ret = [];
  62
+				posX = curCSS( elem, "backgroundPositionX" );
  63
+				posY = curCSS( elem, "backgroundPositionY" );
  64
+				glue = rmultiplebg.exec( posX ) || [""];
  65
+				posX = posX.split( rmultiplebg );
  66
+				posY = posY.split( rmultiplebg );
1
Mike Sherov Collaborator
mikesherov added a note May 25, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Mike Sherov
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.

Rick Waldron
Collaborator

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

Mike Sherov
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.

Rick Waldron
Collaborator

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

Hiroshi Hoaki
rewish commented June 07, 2012

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

Mike Sherov
Collaborator

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

Rick Waldron
Collaborator

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

Hiroshi Hoaki
rewish commented June 07, 2012

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

Mike Sherov
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!

Hiroshi Hoaki
rewish commented June 08, 2012

@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                                  
Mike Sherov mikesherov commented on the diff June 08, 2012
src/css.js
@@ -139,6 +141,33 @@ jQuery.extend({
139 141
 					return elem.style.opacity;
140 142
 				}
141 143
 			}
  144
+		},
  145
+
  146
+		backgroundPosition: {
  147
+			get: function( elem, computed ) {
  148
+				if ( !computed ) {
  149
+					return elem.style.backgroundPosition;
1
Mike Sherov Collaborator
mikesherov added a note June 08, 2012

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
Mike Sherov
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.

Rick Waldron
Collaborator

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

Mike Sherov
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.

Rick Waldron
Collaborator

Thanks for looking into that :)

Hiroshi Hoaki
rewish commented June 09, 2012

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

Mike Sherov
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!

Dave Methvin
Owner

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

Mike Sherov
Collaborator

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

Dave Methvin
Owner

Okay, I'll close it.

Dave Methvin dmethvin closed this June 16, 2012
Dave Smith getdave referenced this pull request in markdalgleish/stellar.js May 10, 2013
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
This page is out of date. Refresh to see the latest.
29  src/css.js
@@ -8,7 +8,9 @@ var curCSS, iframe, iframeDoc,
8 8
 	rnumnonpx = /^-?(?:\d*\.)?\d+(?!px)[^\d\s]+$/i,
9 9
 	rrelNum = /^([\-+])=([\-+.\de]+)/,
10 10
 	rmargin = /^margin/,
  11
+	rmultiplebg = /,\s?/,
11 12
 	elemdisplay = {},
  13
+
12 14
 	cssShow = { position: "absolute", visibility: "hidden", display: "block" },
13 15
 
14 16
 	cssExpand = jQuery.cssExpand,
@@ -139,6 +141,33 @@ jQuery.extend({
139 141
 					return elem.style.opacity;
140 142
 				}
141 143
 			}
  144
+		},
  145
+
  146
+		backgroundPosition: {
  147
+			get: function( elem, computed ) {
  148
+				if ( !computed ) {
  149
+					return elem.style.backgroundPosition;
  150
+				}
  151
+
  152
+				var posY, glue, i, len,
  153
+					propName = "backgroundPosition",
  154
+					ret = curCSS( elem, propName );
  155
+
  156
+				if ( ret !== "0% 0%" ) {
  157
+					return ret;
  158
+				}
  159
+
  160
+				ret = curCSS( elem, propName + "X" );
  161
+				posY = curCSS( elem, propName + "Y" ).split( rmultiplebg );
  162
+				glue = rmultiplebg.exec( ret ) || [""];
  163
+				ret = ret.split( rmultiplebg );
  164
+
  165
+				for ( i = 0, len = ret.length; i < len; ++i ) {
  166
+					ret[i] += " " + posY[i];
  167
+				}
  168
+
  169
+				return ret.join( glue[0] );
  170
+			}
142 171
 		}
143 172
 	},
144 173
 
16  test/unit/css.js
@@ -779,3 +779,19 @@ test( "cssHooks - expand", function() {
779 779
 	});
780 780
 
781 781
 });
  782
+
  783
+test( "css('backgroundPosition') should be valid value", function() {
  784
+	jQuery( "<style type='text/css'>" +
  785
+				"#test-backgroundPosition {background-position: 10px 20px;}" +
  786
+				"#test-backgroundPositionCSS3 {background-position: 10px 20px, 30px 40px;}" +
  787
+				"</style>" ).appendTo( "head" );
  788
+
  789
+	jQuery( "<div id='test-backgroundPosition' style='background-color:#FFF' />" ).appendTo( "#qunit-fixture" );
  790
+	notEqual( jQuery( "#test-backgroundPosition" ).css( "backgroundPosition" ), "0% 0%", "css('background-position') expands properly with not 0% 0%" );
  791
+
  792
+	// This test case is only supported in IE9
  793
+	if ( document.documentMode && document.documentMode === 9 ) {
  794
+		jQuery( "<div id='test-backgroundPositionCSS3' style='background-color:#FFF' />" ).appendTo( "#qunit-fixture" );
  795
+		equal( jQuery( "#test-backgroundPositionCSS3" ).css( "backgroundPosition" ), "10px 20px, 30px 40px", "css('background-position') of CSS3 expands properly with 10px 20px, 30px 40px" );
  796
+	}
  797
+});
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.