Bug Tracker

Ticket #12127 (closed bug: fixed)

Opened 2 years ago

Last modified 2 years ago

Clone does not correctly copy checked state in IE10

Reported by: chatfielddaniel@… Owned by: chatfielddaniel@…
Priority: blocker Milestone: 1.8
Component: attributes Version: 1.7.2
Keywords: Cc:
Blocking: Blocked by:

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)

Change History

comment:1 Changed 2 years ago by dmethvin

  • Owner set to chatfielddaniel@…
  • Status changed from new to pending

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.

comment:2 Changed 2 years ago by danielchatfield

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.

comment:3 Changed 2 years ago by danielchatfield

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

Last edited 2 years ago by danielchatfield (previous) (diff)

comment:4 Changed 2 years ago by dmethvin

  • Status changed from pending to closed
  • Resolution set to duplicate

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.

comment:5 Changed 2 years ago by dmethvin

Duplicate of #10550.

comment:6 Changed 2 years ago by danielchatfield

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.

comment:7 Changed 2 years ago by dmethvin

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.

comment:8 Changed 2 years ago by danielchatfield

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

comment:9 Changed 2 years ago by rwaldron

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.

comment:10 Changed 2 years ago by danielchatfield

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.

comment:11 Changed 2 years ago by rwaldron

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)

comment:12 Changed 2 years ago by danielchatfield

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?

comment:13 follow-up: ↓ 14 Changed 2 years ago by 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

comment:14 in reply to: ↑ 13 Changed 2 years ago by danielchatfield

Replying to 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.

comment:15 Changed 2 years ago by rwaldron

 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.

comment:16 Changed 2 years ago by danielchatfield

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.

comment:17 Changed 2 years ago by rwaldron

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

comment:18 Changed 2 years ago by danielchatfield

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.

comment:19 Changed 2 years ago by danielchatfield

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?

comment:20 Changed 2 years ago by danielchatfield

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

All was working after all.

comment:21 Changed 2 years ago by danielchatfield

comment:22 Changed 2 years ago by rwaldron

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

comment:23 Changed 2 years ago by danielchatfield

Yep - it is different

comment:24 Changed 2 years ago by rwaldron

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

comment:25 Changed 2 years ago by danielchatfield

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.

Last edited 2 years ago by danielchatfield (previous) (diff)

comment:26 Changed 2 years ago by Daniel Chatfield

  • Resolution changed from duplicate to fixed

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

Changeset: 569d064fc93459695cb6eb6fd09e5ba3fda62f03

comment:27 Changed 2 years ago by Dave Methvin

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

comment:28 Changed 2 years ago by dmethvin

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:29 Changed 2 years ago by dmethvin

  • Priority changed from undecided to blocker
  • Status changed from reopened to open
  • Component changed from unfiled to attributes
  • Milestone changed from None to 1.8

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

comment:30 Changed 2 years ago by danielchatfield

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

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

comment:31 Changed 2 years ago by Daniel Chatfield

  • Status changed from open to closed
  • Resolution set to fixed

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

Changeset: 155855b2a9bd95219871210ae7dcacd2a5f7e117

Note: See TracTickets for help on using tickets.