Skip to main content

Bug Tracker

Side navigation

#12127 closed bug (fixed)

Opened July 23, 2012 03:04PM UTC

Closed July 26, 2012 02:25AM UTC

Clone does not correctly copy checked state in IE10

Reported by: chatfielddaniel@googlemail.com Owned by: chatfielddaniel@googlemail.com
Priority: blocker Milestone: 1.8
Component: attributes Version: 1.7.2
Keywords: Cc:
Blocked by: Blocking:
Description

The bug is with IE not jQuery but viewing the source shows that a similar problem was in webkit and jQuery fixed the browser bug.

See: http://codepen.io/danielchatfield/pen/DxzKv

(I know that is native javaScript and not jQuery but it demonstrates the problem that needs to be corrected)

Attachments (0)
Change History (31)

Changed July 23, 2012 04:40PM UTC by dmethvin comment:1

owner: → chatfielddaniel@googlemail.com
status: newpending

Can you provide a jQuery example? Our feature detect for the same Chrome problem should fix it I'd think, but if you've found otherwise we'd like to fix it.

Changed July 23, 2012 04:46PM UTC by danielchatfield comment:2

Yep:

http://codepen.io/danielchatfield/pen/sDwJI

If you uncheck the checkbox and click clone the new checkbox should also be unchecked but in IE10 (and maybe other versions) it is checked.

Changed July 23, 2012 05:00PM UTC by danielchatfield comment:3

_comment0: Also - how do I take ownership of this ticket? It is owned by me email address but not my actual account.1343063282035493

Also - how do I take ownership of this ticket? It is owned by my email address but not my actual account.

Changed July 23, 2012 05:10PM UTC by dmethvin comment:4

resolution: → duplicate
status: pendingclosed
how do I take ownership of this ticket?

Normally you'd be the owner if you were planning to submit a patch. Did you want to do that?

As far as the bug goes, it happens in IE9 as well. May happen in oldIE as well but codepen won't run in those. Also i can't get codepen to change the version of jquery in IE9, it will do so in other browsers. My goal was to see whether this is a regression, but codepen isn't cooperating.

Working jsfiddle: http://jsfiddle.net/TBEXk/

Does not seem to be a regression. In fact it's a dup of #10550. I'm not sure why that was closed wontfix, looking at the trail, although it's an ambiguous case for sure. The checked ''property'' is false but the checked ''attribute'' still contains ''checked''. But like you say, we "fix" a similar Chrome situation.

If you'd like to submit a patch on #10550 or look at https://github.com/jquery/jquery/pull/558 to see if it can be resurrected, we can consider it.

Changed July 23, 2012 05:10PM UTC by dmethvin comment:5

Duplicate of #10550.

Changed July 23, 2012 05:13PM UTC by danielchatfield comment:6

Thanks for that - I currently work around it by checking whether it is checked using .is(':checked') before the clone and then making sure the checked attribute is either removed or added depending on its previous state. I imagine that would be far too expensive to just do to all clones.

Changed July 23, 2012 05:24PM UTC by dmethvin comment:7

Oh waitaminit, I remember why this was wontfixed. The problem is on the ''appending'' of the DOM elements, not the ''clone''. There is no easy central place in jQuery to fix this. You can monkey with the attribute but that produces HTML that isn't the same as the input, which brings its own set of problems.

Changed July 23, 2012 07:07PM UTC by danielchatfield comment:8

Just submitted a Pull request that fixes it:

The fix was already in place for IE6-8 but the test that fired the fix incorrectly passed in IE9, I have altered the test so that now it fails in IE9, IE10.

https://github.com/jquery/jquery/pull/871

Changed July 23, 2012 07:35PM UTC by rwaldron comment:9

I just commented on the patch - you didn't change any tests, you changed code in "support.js", which you don't need to do.

Changed July 23, 2012 07:38PM UTC by danielchatfield comment:10

Hi,

I'll add a test but there was an issue with the code in support.js:

jQuery.support.noCloneCheck was false in IE9 and IE10 even though IE9 and IE10 should have failed this.

Changed July 23, 2012 08:21PM UTC by rwaldron comment:11

Gotcha.

Either way, there still needs to be a test added to /test/unit/manipulation.js that supports this change (and it all needs to pass in IE6-8 as well)

Changed July 23, 2012 08:24PM UTC by danielchatfield comment:12

I have never written a test before so I'm just working out how to do it - where abouts in that file should I add the test? The bottom?

Changed July 23, 2012 08:39PM UTC by rwaldron comment:13

All of the correct values are present when I run this in IE9:

http://jsfiddle.net/rwaldron/CCaBQ/

"checked" is a property and is set correctly, attributes !== properties

Changed July 23, 2012 08:43PM UTC by danielchatfield comment:14

Replying to [comment:13 rwaldron]:

All of the correct values are present when I run this in IE9: http://jsfiddle.net/rwaldron/CCaBQ/ "checked" is a property and is set correctly, attributes !== properties

The problem is when you clone a checkbox where the checked attribute is set and the checked property is false. On the cloned element the checked property will be true in IE9.

Changed July 23, 2012 09:05PM UTC by rwaldron comment:15

http://jsfiddle.net/rwaldron/CCaBQ/

Open that in any browser (try Chrome or Firefox). checked="checked", checked="true", checked="false"... these all mean the same thing; if the "checked" (boolean) attribute exists, then the box is checked, no matter what — it's very existence causes this.

Changed July 23, 2012 09:16PM UTC by danielchatfield comment:16

Let me explain:

I have a form with an input which is checked by default (because the checked attribute is set). I uncheck it (by clicking on it) - this does NOT remove the checked attribute (check the chrome inspector), but rather sets the checked property to false. If you clone this element in chrome you get another checkbox which has the checked attribute but the checked property is false (so it is not checked) - this is what you would expect. In IE however it either fails to copy across the checked property or some event gets fired that shouldn't which means it gets overwritten because of the presence of the attribute. I realise it is counter intuitive because the selector [checked="checked"] selects the checked one but at a DOM level that just isn't what is happening (hence why when you clone an unchecked checkbox it will have the checked attribute if the original one did)

The presence of a checked attribute does not actually mean the input is checked - it means it will be checked by default. If you add the checked attribute to an element the browser will, in accordance with the spec, set the checked property to true.

If you give me 2 minutes I'm almost ready to submit another PULL request - this time with a test.

Changed July 23, 2012 09:20PM UTC by rwaldron comment:17

No rush, I'm just trying fully understand your issue so I can help

Changed July 23, 2012 09:52PM UTC by danielchatfield comment:18

I'm struggling to make a test that is working. Could you please look at this:

http://jsfiddle.net/bKKGj/2/

Clicking clone should create a copy of the checkbox (and be unchecked), this works in all browsers except IE9, IE10 where the copy is checked. I'm not sure I'm doing the tests right because everything is passing.

Changed July 23, 2012 10:15PM UTC by danielchatfield comment:19

I am at a loss here - I really don't know why this test isn't working.

See: http://jsfiddle.net/bKKGj/8/

All browser except IE will return true (with current jQuery). And all including IE with my changes. However When I wrap that in a test like so:

test("checked state is cloned with clone()", function(){
	elem = jQuery.parseHTML('<input type="checkbox" checked="checked"/>')[0];
	elem.checked = false;
	result = !jQuery(elem).clone().attr('id','clone')[0].checked;
	ok( result, 'Checked state correctly cloned' );
});

IE passes it with jQuery 1.7.2 - what is wrong?

Changed July 23, 2012 10:17PM UTC by danielchatfield comment:20

STUPID ME - after removing my changes I didn't rebuild it.

All was working after all.

Changed July 23, 2012 10:33PM UTC by danielchatfield comment:21

Changed July 24, 2012 08:16PM UTC by rwaldron comment:22

Can we get a confirmation -- is this different from http://bugs.jquery.com/ticket/12132

Changed July 24, 2012 08:17PM UTC by danielchatfield comment:23

Yep - it is different

Changed July 24, 2012 08:19PM UTC by rwaldron comment:24

Please review the discussion on the PR as well https://github.com/jquery/jquery/pull/870

Changed July 24, 2012 09:54PM UTC by danielchatfield comment:25

_comment0: Well I can confirm that that with that change the test that I added would still fail. They might be related but neither my change or that one would fix both problems so one would assume they are isolated problems. \ \ Now that I have studied the source a bit more I know a little more about the problem: \ \ In IE6-8 when a checkbox or radio was cloned the checked state was not copied. To fix this noCloneCheck was added to jQuery.support, and cloneFixAttributes was modified to make sure checked state was correctly cloned. In IE9 this was half fixed - it does correctly carry across the checked property however if the checked attribute exists on the source element then some browser event would get fired (which shouldn't) that would incorrectly set the checked property to true. The existing test does not pick this up because until ie9 a browser either correctly cloned it all the time or never, I have modified the test so that rather than checking whether a 'true' checked value is cloned it tests whether a 'false' one is; as ie6-8 will still fail this and now so will ie9 and ie10. I then modified the code that was designed to carry across the checked state so that it clones it whether it is checked or not. 1343166904279390

Well I can confirm that with that change the test that I added would still fail. They might be related but neither my change or that one would fix both problems so one would assume they are isolated problems.

Now that I have studied the source a bit more I know a little more about the problem:

In IE6-8 when a checkbox or radio was cloned the checked state was not copied. To fix this noCloneCheck was added to jQuery.support, and cloneFixAttributes was modified to make sure checked state was correctly cloned. In IE9 this was half fixed - it does correctly carry across the checked property however if the checked attribute exists on the source element then some browser event would get fired (which shouldn't) that would incorrectly set the checked property to true. The existing test does not pick this up because until ie9 a browser either correctly cloned it all the time or never, I have modified the test so that rather than checking whether a 'true' checked value is cloned it tests whether a 'false' one is; as ie6-8 will still fail this and now so will ie9 and ie10. I then modified the code that was designed to carry across the checked state so that it clones it whether it is checked or not.

Changed July 25, 2012 01:36PM UTC by Daniel Chatfield comment:26

resolution: duplicatefixed

Fix #12127. IE9/10 checks fall off the box on clone. Close gh-873.

Changeset: 569d064fc93459695cb6eb6fd09e5ba3fda62f03

Changed July 25, 2012 06:40PM UTC by Dave Methvin comment:27

Revert "Fix #12127. IE9/10 checks fall off the box on clone. Close gh-873."

This reverts commit 569d064fc93459695cb6eb6fd09e5ba3fda62f03.

Causing test fails in Safari, IE6, and IE7.

Changeset: de213be3728a339e1f2b98712a0869f8afb9da67

Changed July 25, 2012 06:41PM UTC by dmethvin comment:28

resolution: fixed
status: closedreopened

Changed July 25, 2012 06:45PM UTC by dmethvin comment:29

component: unfiledattributes
milestone: None1.8
priority: undecidedblocker
status: reopenedopen

We'll need to find a solution to those test fails, still hoping to get this into 1.8 final.

Changed July 25, 2012 08:04PM UTC by danielchatfield comment:30

Got it this time - didn't actually need to alter support.js

https://github.com/jquery/jquery/pull/875

Changed July 26, 2012 02:25AM UTC by Daniel Chatfield comment:31

resolution: → fixed
status: openclosed

Fix #12127, fer real. IE9/10 check state on clone. Close gh-875.

Changeset: 155855b2a9bd95219871210ae7dcacd2a5f7e117