Bug Tracker

Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#10550 closed bug (cantfix)

clone does not transfer checked state for input[type=checkbox] in IE9

Reported by: mattmanic Owned by:
Priority: high Milestone: 1.11.1/2.1.1
Component: attributes Version: 1.6.4
Keywords: Cc:
Blocked by: Blocking:

Description

The current code to clone the checked state of an input[type=checkbox] is as follows:

if ( src.checked ) {
  dest.defaultChecked = dest.checked = src.checked;
}

This does not work in IE9 as setting object.checked = true does not actually check the object, but simply sets that property on the object as true. To set the checkbox to true, you need to use

object.setAttribute('checked','checked').

I have written a patch, and a test which fails under IE9 for this, and will submit a pull request shortly and link to it from this ticket.

Matt

Change History (28)

comment:1 Changed 7 years ago by mattmanic

Pull request is at https://github.com/jquery/jquery/pull/558, commit is at https://github.com/mattheworiordan/jquery/commit/174c48e1f4bb8c0dcea034305a65aa9b493298d2

Please pull in that commit as it's a small and simple change, and already tested on IE6+, Firefox, Chrome and Safari.

comment:2 Changed 7 years ago by timmywil

Component: unfiledattributes
Priority: undecidedlow
Resolution: invalid
Status: newclosed

The property is what needs to be set. The checked attribute does not actually relate to the checked state of a checkbox except on initial pageload.

http://timmywillison.com/pres/attributes/#boolean-example

comment:3 Changed 7 years ago by mattmanic

Timmy, this issue is not resolved, and is replicable in IE9. I am really not sure why you closed this ticket, I wrote tests which confirm this is an issue, if you had run them you would have seen that.

I have written a jsFiddle now so you can see the problem for yourself (using IE9) at http://jsfiddle.net/JuFxk/10/](http://jsfiddle.net/JuFxk/10/

I have also updated my commit so that hopefully it follows the jQuery guidelines. Please see the pull request at https://github.com/jquery/jquery/pull/559

Thanks, Matt

comment:4 Changed 7 years ago by Rick Waldron

That test case is invalid and disproven: http://jsfiddle.net/rwaldron/cq2PB/

comment:5 Changed 7 years ago by timmywil

From the PR: Just appending the html of the node rids you of everything on the DOM level that was cloned from the node (events, data, properties, etc.) and recreates a new input based solely on the attributes supplied. Though the reason why you thought our test was faulty makes much more sense to me now (because in other browsers setting defaultChecked also writes an attribute into the html), the intention of clone is still intact. Keep the clone intact and everything works as expected. If needed, let's discuss this issue further in the ticket and not open more pull requests.

comment:6 Changed 7 years ago by mattmanic

Timmy, I am sorry, but I don't understand you reasoning.

When you clone an element, you would expect all of it's attributes to be cloned as well. That is how the clone method behaves for ALL other elements: examples, select retains the selected state, text input fields retain their value attribute, so why would it be OK for checkboxes to not retain their attribute?

This is not what users of JQuery would expect. I have cloned an entire form in my code base, and stored the HTML so that I can regenerate the form from this HTML source at any point (I cannot use JQuery objects in this instance, long story). Only the check boxes and radio buttons lose their state, all other elements retain their state completely. This is in ALL browsers other than IE9.

Perhaps what you're saying is that the clone method is doing what it should, however the html() method is not generating the correct HTML on a cloned element for some reason. Either way, there is a clear bug there that will cause problems for people because either clone() is not cloning correctly, or html() is not generating HTML correctly from cloned elements. Surely you would expect $(elem).clone().html() to generate HTML that reflects the state of elem and all its children?

I really cannot see a single example where you would want to clone all properties, but not clone the checked state if you converted that node to HTML. And if this only occurs in IE9, surely that is a browser specific issue that others will encounter that you need to address?

comment:7 Changed 7 years ago by mattmanic

Sorry, just for others who read this post, in response to rwaldron, I modified his code to show a case that does fail http://jsfiddle.net/cq2PB/16/ based on the code he wrote. So don't really see how this disproves my case, as arguably this just supports my case once again.

comment:8 Changed 7 years ago by timmywil

This is not a valid use case. It's a waste of code to use clone then try to create another node from .html. $(elem).clone().html() should not be relied upon to generate yet another real clone. To ensure that would be to open up a new realm of possibilities in behavior. The checked attribute in IE9 is not the only thing wrong with that and it is far from the only thing getting thrown out here. If you would like to create a string of html and simply match the checked state or the checked attribute, it should be done without the use of clone.

comment:9 Changed 7 years ago by mattmanic

Ok, so you're basically saying that html() is not a complete method and prone to be buggy, so I should not rely on it to generate HTML that accurately reflects the current node? Odd, but if that is what you're saying, then perhaps you should add notes in the documentation to that effect.

comment:10 Changed 7 years ago by timmywil

.html is doing it's job correctly. .html is simply not meant for cloning. If we need to duplicate all of the properties as attributes, we're going to be writing a lot of code. Even if we did want to ensure that the checked property came over in IE9 as an attribute, there would be several other issues to think about. Do we base it off the current state or the default state? What if there is no attribute on the original node? The html generated from the clone, even in IE9, is an accurate representation of the cloned node. In the use case provided so far, there's no reason to clone. $('div').append( $input.html() ); would accomplish the same thing.

comment:11 Changed 7 years ago by mattmanic

Ok, well unfortunately I'm clearly not going to convince you otherwise.

I do unfortunately believe that many other people who happen to have to use the html method (I know, it was odd that I needed it, but I did), may get some nasty surprises.

Thanks for your time anyway, I appreciate you responding to the issue so quickly.

comment:12 Changed 7 years ago by timmywil

I'm happy to debate. If this is proves to be a common issue, we can certainly reconsider.

comment:13 Changed 7 years ago by timmywil

Resolution: invalid
Status: closedreopened

However, I originally closed this without seeing the issue you were proposing so I'll close this wontfix instead of invalid. We can reopen in the future if needed.

comment:14 Changed 7 years ago by timmywil

Resolution: wontfix
Status: reopenedclosed

comment:15 Changed 7 years ago by dmethvin

I think it would be fair to say that just about all attributes mirrored as properties have inconsistent behavior in one or more browsers. It just isn't reasonable to expect a browser to mirror dynamic property changes back to the corresponding attributes when an element is cloned. For example, it doesn't do that for user-typed input in text boxes in any browser, as far as I know.

comment:16 Changed 7 years ago by mattmanic

dmethvin, for user-typed input in text boxes, that is user generated or dynamically updated after the node is created. The example I have given creates a node where checked=checked at the outset, and is copied by the clone command (as expected), but because checked is not set using setAttribute, but using .checked instead, with IE 9 only when you use .html() the checked state is ignored.

See this example in IE9, only the checkboxes and radio button states are not checked when cloned and converted to HTML. http://jsfiddle.net/JuFxk/14/

I have given this whole thing much more thought, and I really do think this is a bug because of the following:

Matt

comment:17 Changed 7 years ago by response.write@…

Hi there, I had the exact same issue and ended up writing a hack. I totally agree in with the issue raised by mattmatic - please guys check out the jsfiddle test - he has a point!

.clone() === perfect .html() === perfect .clone().html() === buggy (for IE9, checkbox='checked')!

comment:18 Changed 7 years ago by matias

That this is a wontfix does not make sense to me at all and I strongly urge you to reconsider this. But if you still believe that not cloning the checked attribute in IE9 is perfectly logical, maybe you should update the docs to reflect that?

My suggestions;

"This is useful (except in IE9) for moving copies of the elements to another location in the DOM."

"This is useful for moving copies (or almost copies when using IE9) of the elements to another location in the DOM."

comment:19 Changed 7 years ago by grace@…

Why on earth won't you address this issue? Mattmanic has suggested a simple fix, and the problem is replicable. What more do you need?

comment:20 Changed 7 years ago by luke@…

I also concur with matias here. I'm trying to clone all form fields that are within a TR and place them inside a form outside of the containing table (because of course forms in TRs don't work well). The issue arises because I have a select element, and using .clone() (without needing .html()) combined with .appendTo() does not retain the currently selected item.

Then of course, I need to write specific cases for select statements, which bloats the code quite a lot. It's a bit ambiguous for the input fields to retain their values, and not the selects. It's misleading to say the least - if it's not to be fixed it certainly should be documented.

comment:21 Changed 7 years ago by dmethvin

#12127 is a duplicate of this ticket.

comment:22 Changed 7 years ago by danielchatfield

Hi everyone.

Unless I have misunderstood something the code to fix this was already in the source:

jQuery.support.noCloneCheck is meant to be true when checked state is not copied across clone

and then cloneFixAttributes() makes sure the checked state is the same as the source element.

The problem was that the test was failing to identify the issue with IE9 and IE10 and that the fix in cloneFixAttributes() was only designed to fix the case where it was checked.

I will submit a pull request in a few minutes.

comment:23 Changed 6 years ago by jeff@…

I've always considered jQuery to be browser independent, and it technically copies the loaded html for ie,firefox and chrome just the same. But in firefox and chrome it also copies the current state, but it's not listed in html. (for example if you set the hidden attribute the the loaded state is sent via a post, but if not hidden the currect active state is sent.) In ie it does not copy the current state. It there a work around to copy the current state to the html?

comment:24 Changed 6 years ago by anonymous

I have the same issue! I had to set a class before cloning, and then search for the class afterwards and set checked checked. So it's obviously an issue!

comment:25 Changed 6 years ago by dmethvin

Resolution: wontfix
Status: closedreopened

Reopening for us to take a look. We're only doing this special case for checkboxes but it seems like we're stuck doing it.

comment:26 Changed 5 years ago by dmethvin

Milestone: None1.11.1/2.1.1
Priority: lowhigh
Status: reopenedopen

comment:27 Changed 5 years ago by dmethvin

Resolution: cantfix
Status: openclosed

jQuery doesn't control the serialized HTML string representation of some DOM in the document when you use the .html() method, that's the browser's job and some do it better than others. Once it is a serialized HTML string representation, it's pretty much impossible for jQuery to go through that arbitrarily large serialized HTML string representation of a DOM, compare it to the DOM from where it came, and "fix" the serialized HTML string representation to match the DOM. To do that, we'd need an HTML parser in jQuery. We don't have an HTML parser in jQuery, which is why we use the .innerHTML method from the browser.

Alternatively we could go through the DOM before serialization and attempt to find all the checkboxes, then set their attributes (vs properties). That's again a very expensive operation for a special case. If you know you are cloning checkboxes and need the cloned representation to reflect the dynamic value, you can easily set it yourself.

Sorry. If you have some idea for a fix please let us know. Complaining that we aren't fixing it won't help since I don't think we can fix it.

Last edited 5 years ago by dmethvin (previous) (diff)

comment:28 Changed 5 years ago by dcsoft

Could you please update the documentation to note the limitations? It affects more than just clone(). For example, I was using .replaceWith() to copy a <input type="checkbox"> to a different part of the DOM, and it did not copy the checked property. It helps to know what is preserved OK and what is not.

Note: See TracTickets for help on using tickets.