23565 2013-10-18 18:50:34 +0000 cloneNode IDL does not match reality 2013-12-20 19:25:19 +0000 1 1 1 Unclassified WebAppsWG DOM unspecified PC All RESOLVED FIXED P2 normal --- 1 bzbarsky annevk bugs d erik.arvidsson jonas mike Ms2ger VYV03354 www-dom public-webapps-bugzilla oldest_to_newest 94949 0 bzbarsky 2013-10-18 18:50:34 +0000 The spec has: Node cloneNode(optional boolean deep = true); but the web depends on passing undefined being treated as false. In particlar, CKEditor depends on it. See https://bugzilla.mozilla.org/show_bug.cgi?id=926305 94995 1 annevk 2013-10-21 10:48:27 +0000 So we want cloneNode() and importNode() to have different signatures? Note that cloneNode() in Chrome defaults to true as well. 95000 2 bzbarsky 2013-10-21 13:19:18 +0000 Er, I somehow missed importNode. importNode needs to change too. > Note that cloneNode() in Chrome defaults to true as well. Exactly. But cloneNode(undefined) uses false, not true. 95005 3 annevk 2013-10-21 14:09:17 +0000 Bah bah bah. https://github.com/whatwg/dom/commit/70fdc5f9030c22c5ceca9e875e06f8b305529ef8 95006 4 erik.arvidsson 2013-10-21 14:16:28 +0000 OK. I'll update Blink. Should be safe since the default to true case is still hot out of the oven. 95009 5 bzbarsky 2013-10-21 14:52:12 +0000 > OK. I'll update Blink. There should be no need to change anything in Blink. This testcase: <body><script> alert(document.body.cloneNode(undefined).childNodes.length); alert(document.body.cloneNode().childNodes.length); </script> already alerts "0" and then "2" in my Chrome dev build (32.0.1671.3), which is what the DOM spec _used_ to require before the WebIDL changes to how undefined is handled, and is also what the DOM spec should imo specify once this bug is fixed. Anne, your changes aren't what this bug was asking for. What this bug is asking for is: Node cloneNode(); // deep clone specified in prose Node cloneNode(boolean deep); and similar for importNode. Reopening, since as far as I can tell the intent of the desired change was totally misunderstood. 95010 6 bzbarsky 2013-10-21 14:53:30 +0000 In particular, the web compat requirement here is that cloneNode(undefined) do a shallow clone. There is no strong web compat requirement of any sort on cloneNode() with no arguments, since that's only been non-throwing in Gecko for a short time. During that time, though, it's consistently done a deep clone. 95011 7 bzbarsky 2013-10-21 14:54:29 +0000 > already alerts "0" and then "2" Er, "0" and then "1", since I got rid of the whitespace before <script>. ;) 95012 8 d 2013-10-21 15:03:15 +0000 > There is no strong web compat requirement of any sort on cloneNode() with no arguments In that case it would ideally be the same as cloneNode(undefined), right? I.e. shallow clone? I think that's what Anne's changes implement? 95013 9 annevk 2013-10-21 15:06:56 +0000 Yeah, I don't want omitted and undefined to be different if we can avoid it. And since WebKit already had this as false for both cases... 95014 10 Ms2ger 2013-10-21 15:33:41 +0000 I disagree. It would be better for cloneNode() to throw than to do a shallow clone. 95015 11 annevk 2013-10-21 15:37:55 +0000 We cannot throw for undefined and we want undefined to be false so omitted should also be false. 95031 12 bzbarsky 2013-10-21 19:18:40 +0000 > In that case it would ideally be the same as cloneNode(undefined), right? Well, maybe. There may well be a web compat requirement here anyway, I just can't tell. Or put another way, I'm not willing to change the Gecko behavior to have cloneNode with no arguments do a shallow clone, because I'm worried about that causing compat problems at this point and I don't think theoretical purity is worth breaking compat in this case. 95032 13 erik.arvidsson 2013-10-21 19:37:07 +0000 bz: How long have you been shipping with optional meaning true? Since you used to throw for no arguments until pretty recently most code out there always pass an argument. Also IE and WebKit never changed to treat optional as true. I think this is as close to safe as it can get on the web. 95033 14 bzbarsky 2013-10-21 19:52:18 +0000 > bz: How long have you been shipping with optional meaning true? Since Firefox 13. So in final releases since June 5, 2012. > until pretty recently Over a year is pushing it for "pretty recently". > I think this is as close to safe as it can get on the web. I disagree. :( 95034 15 bzbarsky 2013-10-21 19:56:50 +0000 That is, I think there is a good chance of Gecko making this change breaking content, due to content-sniffing and Gecko-only codepaths. :( We would _definitely_ break a number of Firefox extensions; I just checked and a bunch of them use cloneNode() with no arguments and expect deep clones. We might be willing to go through all that breakage if there were a very strong reason to, but in this case, I just don't think the reasons are strong enough. 95035 16 erik.arvidsson 2013-10-21 20:24:32 +0000 (In reply to Boris Zbarsky from comment #15) > We might be willing to go through all that breakage if there were a very > strong reason to, but in this case, I just don't think the reasons are > strong enough. Compat with IE and Safari seems like reasons strong enough to me. 95053 17 bzbarsky 2013-10-21 21:49:26 +0000 Mmm, maybe. I just don't see an obvious way for us to go about making this change without causing problems for our users, unfortunately. It's not clear to me why we should be doing that instead of IE and Safari changing here. 95054 18 bzbarsky 2013-10-21 21:50:33 +0000 And I should note that at least for me my willingness to implement speculative spec proposals in the DOM spec is at an all-time low due to my experiences with this situation and similar. :( 95079 19 annevk 2013-10-22 09:40:15 +0000 bz, I'm sorry. I wish we had seen this one coming and I wish we had thought longer about IDL when it was still fresh, but it seems the only way we learn is extremely slow iteration with lots of drama along the way. 95152 20 bzbarsky 2013-10-23 06:03:10 +0000 That doesn't answer my question from comment 17, note. 95153 21 bzbarsky 2013-10-23 06:14:20 +0000 And one other note. The compat with Safari/IE argument is no stronger than it was two weeks ago. So I don't buy that it's particularly compelling in this case, if it wasn't then.... 95169 22 annevk 2013-10-23 10:00:47 +0000 So these are the options as far as I can tell: 1) cloneNode() -> cloneNode(true) cloneNode(undefined) -> cloneNode(false) 2) cloneNode() -> cloneNode(false) cloneNode(undefined) -> cloneNode(false) The latter is the preferred way of treating undefined. It also matches WebKit and IE. The former matches Gecko and Chrome and will require some IDL hack long term to make it work. Chrome only recently switched from 2. Gecko threw for the cloneNode() case until one year ago. It seems to me the safest is to go with 2. 95529 23 bzbarsky 2013-10-29 22:11:15 +0000 On the other hand, for the common case of cloning, which is deep clones, the API would be _way_ better if it could be just cloneNode(). The only real issue here is that there is legacy content that depends on cloneNode(undefined) doing a shallow clone. While we need to support that legacy content, I believe that the default behavior for no arguments passed should be the sane one. That's why the mailing list discussion initially decided the default here should be "true" to start with. Does the argument for consistency between undefined and not passed really trump the web developer ergonomics argument here? > and will require some IDL hack long term to make it work. That's putting spec purity ahead of developers, which is just wrong from a priority of constituencies perspective... 95531 24 bzbarsky 2013-10-29 23:57:15 +0000 And again, it seems to me like we're allowing the compat between undefined and omitted edge case tail wag the API design dog here... 95538 25 annevk 2013-10-30 13:38:15 +0000 As far as I can tell the developer ergonomics argument applies both ways. Having undefined and omitted not equal each other will be found surprising and lead to subtle bugs. 95539 26 bugs 2013-10-30 13:54:07 +0000 What is surprising if passing an argument has different behavior than not passing an argument? 97824 27 Ms2ger 2013-12-20 07:53:14 +0000 https://github.com/w3c/web-platform-tests/pull/473 97829 28 bzbarsky 2013-12-20 19:25:19 +0000 We're going to align with the new spec here, fwiw, since it seems like no one else is interested in doing something different....