Skip to content

[1.9] Prevent Clearing Cloned Style to Affect Source Element's Style. Fixes #8908 #886

wants to merge 76 commits into from

Clearing a Cloned Element's Style Causes the Original Element's Style to be Cleared


In IE9 and IE10 you can run into the situation when clearing ("") a cloned element's style will also clear out the original element's corresponding style.

The actual jQuery Core Ticket #8908 isolates the bug when using the background-image style.

While I was trying to recreate and research this issue I had a feeling that there may be other styles that have the same issue, so I've created a test suite checking 252 CSS1-3 styles and testing various assertions.

Along the way, I also found some additional inconsistencies. These findings are technically unrelated to the #8908 issue so I will list those inconsistencies somewhere else and we can discuss those later.

Source Element's Style Cleared When Cloned Element's Style Cleared

IE9 & IE10

After testing 252 various styles only the following 8 ended up being problematic.

  • background-attachment
  • background-color
  • background-image
  • background-position
  • background-repeat
  • background-clip
  • background-origin
  • background-size

IE 6/7/8, Chrome, Firefox, Opera

Thankfully these browsers did not have the problem described in this issue when tested.

Proposed Solutions

1. Use Default Values when Trying to Empty Style

The following fix resolves the weird issue by substituting and empty string with it's default value. An object is used to store the style name and associated default value. If the value happens to be an empty string then it will look in the object for a matching key and if there is a match it will use that instead.

// css.js
+ // These styles were identified to be problems in IE9/10
+ var defaultValue = {
+     "background-attachment": "scroll",
+     "background-color": "transparent",
+     "background-image": "none",
+     "background-position": "0% 0%",
+     "background-repeat": "repeat",
+     "background-clip": "border-box",
+     "background-origin": "padding-box",
+     "background-size": "auto auto"
+ };

+ value = ( value === "" && defaultValue[ name ] !== undefined ) ? defaultValue[ name ] : value;

try {
    style[ name ] = value;
} catch(e) {}


  • This is very explicit in describing which styles are problematic in IE


  • There is extra code executed when clearing the style for a nonIE browser. This could be a problem because updating a style via the .css() method is very common
  • There is a considerable amount of code to account for all problematic styles and this will bloat the jQuery source

2. Strange Side Effect That Breaks the Clone Connection

This solution is a little more strange and not so obvious. I ran into this fix as I was building up my unit tests and asserting a bunch of things I thought should be true. One of those assertions was verifying that when setting a style using the $elem.css( "someStyle", "someValue" ) method that it actually worked. So, I used the getter ($elem.css( "someValue" ) method, but by doing so I found the problem going away just by calling the getter! And with that the following solution came about.

You may notice a slightly strange piece of code ( window.getComputedStyle( elem, null ) || {} ), but that was needed to get around an Opera bug I had when working with XML nodes. Technically I could have made the whole fix into one statement, but the minification process does a decent job of squishing down the fix to look like (b.nodeType===1&&a.getComputedStyle&&(i=(a.getComputedStyle(b,null)||{}).backgroundImage).

// clone.js
    clone: function( elem, dataAndEvents, deepDataAndEvents ) {
        var srcElements,
-           clone;
+           clone,
+           fix8908;

        if ( || jQuery.isXMLDoc(elem) || !rnoshimcache.test( "<" + elem.nodeName + ">" ) ) {
+           // Fixes #8909 - By accessing one of the element's computed styles it breaks the
+           // connection with the cloned element's styles in IE9/10
+           if ( elem.nodeType === 1 && window.getComputedStyle ) {
+               fix8908 = ( window.getComputedStyle( elem, null ) || {} ).backgroundImage;
+           }
            clone = elem.cloneNode( true );
        // IE<=8 does not properly clone detached, unknown element nodes
        } else {


  • The amount of code for this fix is relatively small
  • This code is limited to the clone execution path, which happens infrequently when compared to the .css() method approach in option #1.


  • This relies on a weird quirk in IE9/10 to get around the problem, but it seems to be the most succinct.

3. Have IE9 & IE10 Fix this Bug

It would be ideal if no patch had to be made to jQuery at all and IE9 and IE10 could apply a fix instead. Seeing that the issue is found in IE9 though may rule out this option entirely.


  • No patch for jQuery would be needed


  • Even if the bug was fixed immediately for IE9 & IE10 there would still be an issue with versions of IE9 that didn't have the IE fix.

Unit Test

The following Unit Test exposes the problem in IE9 and IE10. In browsers that don't support these styles I am skipping over them and asserting that they are not an issue.

test( "Clearing a Cloned Element's Style Shouldn't Clear the Original Element's Style (#8908)", function() {
    expect( 8 );

    var baseUrl = document.location.href.replace( /([^\/]*)$/, "" );
    var styles = [
        { name: "background-attachment", value: [ "fixed" ], expected: [ "scroll" ] },
        { name: "background-color", value: [ "rgb(255, 0, 0)", "rgb(255,0,0)" ], expected: [ "transparent" ] },
        { name: "background-image", value: [ 'url("test.png")', 'url(' + baseUrl + 'test.png)', 'url("' + baseUrl + 'test.png")' ], expected: [ "none" ] },
        { name: "background-position", value: [ "5% 5%" ], expected: [ "0% 0%" ] },
        { name: "background-repeat", value: [ "repeat-y" ], expected: [ "repeat" ] },
        { name: "background-clip", value: [ "content-box" ], expected: [ "border-box" ] },
        { name: "background-origin", value: [ "content-box" ], expected: [ "padding-box" ] },
        { name: "background-size", value: [ "80px 60px" ], expected: [] }

    jQuery.each( styles, function(index, style) {
        var $source, source, $clone;

        style.expected = style.expected.concat( [ "", "auto" ] );
        $source = jQuery( "<div />" );
        source = $source[ 0 ];
        if ([ ] === undefined ) {
            ok( true, +  ": style isn't supported and therefore not an issue" );
            return true;
        $source.css(, style.value[0] );
        $clone = $source.clone();
        $clone.css(, "" );

        ok( ~jQuery.inArray( $source.css( ), style.value ),
            "Clearning clone.css() doesn't affect source.css(): " + +
            "; result: " + $source.css( ) +
            "; expected: " + style.value.join( "," ) );


Based on the research I have done testing 252 CSS styles I have isolated this issue to only 8 styles in IE9 and IE10.

I've recommended solving this issue 3 different ways, but in the end the 2nd option seems best to me since it only is executed in the clone execution path and is significantly smaller than the 1st option. The 3rd option of having IE fix this issue seems too late since the issue also affects IE9.

jQuery Foundation member
dmethvin commented Aug 8, 2012

Nice detective work! 👍 I think we'll have to land something in 1.9/2.0 since IE9 is not gonna get fixed and I think it's even too late for an IE10 fix at this point since they have gone RTM with Windows 8.

The call to getComputedStyle on the clone source element worries me from a performance perspective because I think that will cause a reflow in many cases. It would be good to perf test it and if needed use a feature test for the bug to ensure that only the guilty browsers are hurt by any performance hit.

There's a very similar-sounding bug regarding .clone() in IE7 ... I wonder if it could be solved the same way?

The commits are kind of messed up, probably because you submitted the earlier pull from your master, but we can cherry-pick d5d8622 off there. If you haven't already you should probably clean up your master if you haven't already.


Yeah, I wondered if the PR came in clean or not. Arg. Yeah, I'll try what is listed on that SO link.

The fix comes down to adding the following before the call to elem.cloneNode( true );

// Fixes #8909 - By accessing one of the element's computed styles it breaks the
// connection with the cloned element's styles in IE9/10
if ( elem.nodeType === 1 && window.getComputedStyle ) {
    fix8908 = ( window.getComputedStyle( elem, null ) || {} ).backgroundImage;

As one of the cons I mentioned performance because it would run that code for any browser. I wasn't sure a good way to isolate just to IE9/10.

Good point about causing a reflow. I'm not sure about that one, but I can try to dig into that further.

I'll take a look at #9777 as you mentioned. It maybe be related, but as for the above bug it didn't show itself in IE7.

I figured this PR would probably wait since 1.8 is about finalized. Thanks for your feedback.

If you have any ideas on how to isolate this to IE9/10 I am all ears ;)

@rwaldron rwaldron and 2 others commented on an outdated diff Aug 8, 2012
@@ -585,11 +585,17 @@ jQuery.extend({
var srcElements,
- clone;
+ clone,
+ fix8908;
jQuery Foundation member
rwaldron added a note Aug 8, 2012

I'd like to avoid having numbered identifiers. This seems like something that belongs in the support module.

elijahmanor added a note Aug 8, 2012

@rwldrn Yeah, I wasn't too thrilled about that either, but the fix depends on getting a style off of the object returned from getComputedStyle(). The reason I went ahead and saved it off to a variable was to get around the Expected an assignment or function call and instead saw an expression. JSHint error. I could save it off to an existing variable such as i, but that felt weird too.

I am thinking of pulling out a feature detection into the module to test for this strange bug so that it can be scoped to only IE9/10 and not affect the performance of other browsers.

With this support feature, I will still need to save the style to a variable for JSHint to like it. Would you recommend renaming the variable, reusing another variable, or something else?

jQuery Foundation member
rwaldron added a note Aug 8, 2012

This is a strange thing... It's really bothersome that IE10 actually released a browser that allows a clone's style property to retain reference to the source element.

Anyway, yeah this belongs in, maybe called ;)

jQuery Foundation member
dmethvin added a note Aug 8, 2012

Instead of having the ticket number in the variable name, usually we mention the ticket in a short comment. So you can just pick a short name or even reuse i if it isn't used in the code path.

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

Changes Since the Initial Pull Request

Source Code Changes

I make a new test in the support module called clearCloneStyle that is true if the browser clears only the clone's style appropriate, but is false if it incorrectly clear's the original element's style as IE9 and IE10 do with 8 different styles. I picked the shortest style name of the 8 to test to keep the code short. The feature test intentionally doesn't use jQuery in order to keep the test as fast as possible.

support.clearCloneStyle = (function() {
    var source = document.createElement( "div" ),
        styleName = "backgroundClip",
        value = "content-box",
        clone;[ styleName ] = value;
    clone = source.cloneNode( true );[ styleName ] = "";

    return[ styleName ] === value;

I am using the support.clearCloneStyle property in the fix right before the clone in manipulation.js

if ( || jQuery.isXMLDoc(elem) || !rnoshimcache.test( "<" + elem.nodeName + ">" ) ) {
+    // Fixes #8909 - By accessing one of the element's computed styles it breaks the
+    // connection with the cloned element's styles in IE9/10
+    if ( ! && elem.nodeType === 1 &&
+        window.getComputedStyle ) {
+        i = ( window.getComputedStyle( elem, null ) || {} ).backgroundPosition;
+    }

    clone = elem.cloneNode( true );

As I was retesting the code in IE6/7/8/9/10 I noticed that one of the styles (backgroundPosition) was not passing in IE9 :( The weird getComputedStyle call fixed all of the 8 broken styles in IE10, but only 7 of them in IE9. So, I had to add the following code in the style method of the css.js file to fix that outlier.

// If a hook was provided, use that value, otherwise just set the specified value
if ( !hooks || !("set" in hooks) || (value = hooks.set( elem, value, extra )) !== undefined ) {
+    // IE9/10 Clearing Cloned Style Clear's Original Style. Fixes bug #8908
+    value = ! && value === "" && 
+        name.match( /backgroundPosition/ ) ? "0% 0%" : value;

    // Wrapped to prevent IE from throwing errors when 'invalid' values are provided
    // Fixes bug #5509
    try {
        style[ name ] = value;
    } catch(e) {}

Browsers Tested

I have run the above changes through the following browsers


  • Chrome 22d/21.0b/20/19/18/17/16/15/14
  • Firefox 15.0b/14/13/12/11/10/9/8/7/6/5/4/3.6/3
  • IE 10/9/8/7/6
  • Opera 12.5b/12.0/11.6/11.5/11.1/10.0
  • Safari 5.1/5.0/4.0


  • Chrome 20.0b/19/18/17/16/14
  • Firefox 14.0b/13/12/11/10/9/8/7/6/5
  • Opera 12/11.6/11.1
  • Safari 5.1/5.0/4.0

Unit Test Changes

As I was testing the code in all the above browsers I added another assertion to the Unit Test to check if the style that was being cleared on the clone element was indeed being reset. Because of that addition assertion I made the following changes...

  • Added a unit test to verify that the clone was indeed reset correctly
  • Add the #ff0000 value to match the backgroundColor for Opera 11.1
  • When a background-image is cleared in Firefox it sets the value to the full image path that it is inheriting
  • Added the -1000px 0% value to match the backgroundPosition for Firefox 5.0
  • Switched background-clip to padding-box because Safari 5.0 doesn't support that value

Summary of Changes

  • I added the check and used it before the elem.cloneNode( true ) to limit when a call to getComputedStyle occurs (IE9 / 10)
  • I had to add another fix in the style method to fix the backgroundPosition style that wasn't fixed by the getComputedStyle above. This code path is also limited by the check.
  • I added another assertion to the Unit Test to verify that clearing out the style indeed resets it to an expected default value.
@fracmak fracmak commented on the diff Oct 15, 2012
@@ -204,6 +204,9 @@ jQuery.extend({
// If a hook was provided, use that value, otherwise just set the specified value
if ( !hooks || !("set" in hooks) || (value = hooks.set( elem, value, extra )) !== undefined ) {
+ // IE9/10 Clearing Cloned Style Clear's Original Style. Fixes bug #8908
+ value = ! && value === "" && name.match( /backgroundPosition/ ) ? "0% 0%" : value;
fracmak added a note Oct 15, 2012

Is this a regex because of backgroundPositionX and backgroundPositionY? Is that value still work for those? Also, can you move the regex out to it's own variable at the top of the file with the other regex's so it can get cached?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
jQuery Foundation member

Isn't keeping the tests for browsers with JSON.parse the best way to make sure we have the same behaviour across browsers? Or am I missing something here?

jQuery Foundation member

We can only compare our results to json.parse on browser that have the native method. I agree it's not optimal to test this everywhere but where we actually use it. Perhaps we dump this test's attempt to compare since we don't care about the native result.

jQuery Foundation member

Oh, I get what you mean now. Why not just put the tests with the expected result using jQuery.parseJSON only? Browsers with JSON.parse support will use this code path, others will use our polyfil. We'd have some sort of comparison... just not in the same browser but between browsers. Makes sense?

jQuery Foundation member

I'm with @jaubourg on this one... JSON.parse is just another component we abstract, and will have to normalize if a flaw is discovered in some environment's native implementation.

dmethvin and others added some commits Oct 19, 2012
@dmethvin dmethvin Combine parseJSON tests and fix style.
We only care about the result of parseJSON so there's no reason to feature detect the entire test.
@gibson042 gibson042 Fix #4262: faster .eq(), closes gh-1000. b5084b4
Sai Wong Fix #12610, remove unneeded window.event. Close gh-968. 5228f0a
@mikesherov mikesherov adding awesome new contributors to AUTHORS.txt a7a8aad
@fracmak fracmak Fixes #12518, removes an offsetWidth on focus/blur events for an <IE9…
… bug that caused a performance hit. Closes gh-958
@jonathansampson jonathansampson Fix attribute names in aliased form property test. Close gh-951.
Test expects input elements having name='id', name='name', and name='target'. Additionally, these should have id='id', id='name', and id='target' respectively. No element was provided with id='id' or name='id', but rather one element had two name attributes (illegal) with the values 'id' and 'name' respectively.
Sai Wong Fix #12048. Set attributes for XML fragments. Close gh-965. 2b0e720
@rwaldron rwaldron Don't expose jQuery.deletedIds. Close gh-889. 8076a33
@markelog markelog Alternate fix for #11426; check responseText. Close gh-843. cafb542
Marcel Greter Fix #12107. Let .proxy() curry args without overwriting context. Close de9ff7c
jQuery Foundation member

You might as well remove the definitions of jQuery.deletedIds and jQuery.uuid while you're in there. 1.9 approaches!

dmethvin and others added some commits Oct 20, 2012
jQuery Foundation member

Very deep ;-)

jQuery Foundation member

Haha that was previously much deeper ;) a little copy pasta there

jQuery Foundation member

Maybe additionally assert that the return value is strictEqual to $div? That way it doesn't just assert it being a no-op, but it also actually being chainable.

edit: Done in pull #1008.

jQuery Foundation member

We should encourage new tests to be in their own call to test( ... )

jQuery Foundation member

Had it that way first, but decided it belonged in the same test block. Next time we won't. Good to know. Thanks!

Krinkle and others added some commits Oct 17, 2012
@Krinkle Krinkle Implement expectation test instead of using _removeData. Close gh-997.
* Removed inline usage of QUnit.reset() because it is messing with the
  expectation model as reset does .empty() which does a recursive cleanData
  on everything in #qunit-fixture, so any expectJqData above .reset() would
  fail negatively.

  Instead of calling reset inline, either updated the following assertions to
  take previous assertions' state into account, or broke the test() up into
  2 tests at the point where it would call QUnit.reset.

* After introducing the new memory leak discovery a whole bunch of tests were
  failing as they didn't clean up everything. However I didn't (yet) add
  QUnit.expectJqData calls all over the place because in most if not all of
  these cases it is valid data storage. For example in test "data()", there
  will be an internal data key for "parsedAttrs". This particular test isn't
  intending to test for memory leaks, so therefor I made the new discovery
  system only push failures when the test contains at least 1 call to

  When not, we'll assume that whatever data is being stored is acceptable
  because the relevant elements still exist in the DOM anyway (QUnit.reset
  will remove the elements and clean up the data automatically).

  I did add a "Always check" mode in the test suite that will
  trigger it everywhere. Maybe one day we'll include a call to everywhere,
  but for now I'm keeping the status quo: Only consider data left in storage
  to be a problem if the test says so ("opt-in").

* Had to move #fx-tests inside the fixture because ".remove()" test would
  otherwise remove stuff permanently and cause random other tests to fail
  as "#hide div" would yield an empty collection.
  (Why wasn't this in the fixture in the first place?)

  As a result moving fx-tests into the fixture a whole bunch of tests failed
  that relied on arbitrary stuff about the document-wide or fixture-wide
  state (e.g. number of divs etc.). So I had to adjust various tests to
  limit their sample data to not be so variable and unlimited...

* Moved out tests for expando cleanup into a separate test.

* Fixed implied global variable 'pass' in effects.js that was causing
  "TypeError: boolean is not a function" in *UNRELATED* dimensions.js that
  uses a global variable "pass = function () {};" ...

* Removed spurious calls to _removeData. The new test exposed various failures
  e.g. where div[0] isn't being assigned any data anyway.
  (queue.js and attributes.js toggleClass).

* Removed spurious clean up at the bottom of test() functions that are
  already covered by the teardown (calling QUnit.reset or removeClass to
  supposedly undo any changes).

* Documented the parentheses-less magic line in toggleClass. It appeared that
  it would always keep the current class name if there was any (since the
  assignment started with "this.className || ...".

  Adding parentheses + spacing is 8 bytes (though only 1 in gzip apparently).
  Only added the comment for now, though I prefer clarity with logical
  operators, I'd rather not face the yayMinPD[1] in this test-related commit.

* Updated QUnit urlConfig to the new format (raw string is deprecated).

* Clean up odd htmlentities in test titles, QUnit escapes this.
  (^\s+test\(.*)(&gt\;) → $1>
  (^\s+test\(.*)(&lt\;) → $1<

[1] jQuery MinJsGz Release Police Department (do the same, download less)
@dmethvin dmethvin Update authors. 5be4c10
@markelog markelog Fix #10416. Don't trust computed styles on detached elements. Close g… bea5ecb
@yiminghe yiminghe Fix #12685. Handle inconsistent opacity for ie < 9. Close gh-1005. c78a3ba
@mikesherov mikesherov Fix #12009. $().find( DOMElement ) should pushStack properly. Close g… e8f9151
jQuery Foundation member

Seems like we'd be better off with jQuery.unique than this.

jQuery Foundation member

I presumed this was faster because in this case we already knew the prevLength.

jQuery Foundation member

The previous code wasn't using it either, and i made the same assumption as @mikesherov. I have absolutely no evidence to back up whether it's faster or whether it being somewhat faster makes a difference, but it FEELS like it should. 👻

jQuery Foundation member

True, but it also checks the entirety of prevLength on every outer iteration. I can't say for sure that jQuery.unique would be faster in the average case (not that I would be surprised, either), but it would be smaller, clearer, and always return elements in DOM order (see for a demonstration of the current behavior).

jQuery Foundation member

If this is a correctness issue (re: DOM order), we should fix it.

jQuery Foundation member

Although, we should open a separate issue for that :-)

jQuery Foundation member
jQuery Foundation member

.selector is only relevant for the string case... why not early return this.pushStack( jQuery( selector ).filter(function() { ... }) ) if selector is not a string and skip the else block entirely?

jQuery Foundation member

Not a bad idea.

jQuery Foundation member

Yeah I agree that makes more sense. I can do a followup.

jQuery Foundation member

Damn, forgot to add another test!

jQuery Foundation member

Dunno if it's the trailing comma, but jshint is throwing a fit about the unit tests now.

jQuery Foundation member

Yep, where in JS it is sometimes tolerated, in JSON it is always invalid. Depending on the jshint implementation it will ignore the entire file and consider it invalid JSON because the parser used is unable parse the JSON.

jQuery Foundation member

Color me ashamed. Fix forthcoming (if not already in).

jQuery Foundation member

Trailing comma!

jQuery Foundation member

unnecessary semi.

jQuery Foundation member

Trailing comma!

jQuery Foundation member

unnecessary semi.

jQuery Foundation member

Lol, yes of course, that's what I meant 😮

jQuery Foundation member

Trailing comma!

jQuery Foundation member

unnecessary semi.

jQuery Foundation member
Krinkle commented on f651bf8 Nov 2, 2012

This broke 300+ tests in module "effects" and 5 in "core" (in all browsers).

EDIT: Follow-up: 68f001e.

jQuery Foundation member

That sounds like a record!

jQuery Foundation member

sorry about that :-(

jQuery Foundation member
dmethvin commented Nov 2, 2012

@elijahmanor can you rebase this when you get a chance? It's gotten a bit stale but I'm ready to land it for 1.9. Let me know if you need any help.

jQuery Foundation member
Krinkle commented on 80d45a6 Nov 5, 2012

Do we have tests for jQuery( .., methods );? Or did you remove the only (indirect) tests for that?

jQuery Foundation member
jQuery Foundation member

Yep! 😄

jQuery Foundation member

There are still calls to jQuery.access() which pass 7 parameters. For example, .data() uses all 7 params and sets pass to false. We used to determine if function values should be executed based on pass, but now it's based purely on value. This breaks .data( "foo", function() {} ).

jQuery Foundation member
gnarf commented on 8773067 Nov 8, 2012

Ack sorry I got your name typed wrong @lukemelia

No worries. Thanks for getting this fix in!


@dmethvin I just pulled in the changes and fixed the differences. Unfortunately I saw you said "rebase" after I pushed ;( So there are tons of commits shown.

jQuery Foundation member

Didn't actually close.

@timmywil timmywil closed this Nov 16, 2012
jQuery Foundation member

I haven't landed this yet, I just created a branch for @elijahmanor to review.

@dmethvin dmethvin reopened this Nov 16, 2012
jQuery Foundation member

Woops, read the close, thought it needed to be closes/closed. :/


Great, just saw, that this little bug is fixed now. Can you also add the exact same propHook for the src property?

jQuery Foundation member

@aFarkas, can you create a ticket for that?

@dmethvin dmethvin closed this in 5904468 Nov 18, 2012
jQuery Foundation member

I realized there was no need to fix for backgroundPositionX/Y since they're not standard or supported across browsers (Firefox is the holdout, possibly Opera as well).


It is a pity.

jQuery Foundation member
mgol commented on 9dd0b01 Feb 6, 2014

@markelog Can you chime in in this bug? It's supposed to be fixed long ago.

@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@markelog markelog Ref #8908, gh-886. Avoid clone identity crisis in IE9/10. Close gh-1036. ead9dcc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.