This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 23565 - cloneNode IDL does not match reality
Summary: cloneNode IDL does not match reality
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: DOM (show other bugs)
Version: unspecified
Hardware: PC All
: P2 normal
Target Milestone: ---
Assignee: Anne
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-18 18:50 UTC by Boris Zbarsky
Modified: 2013-12-20 19:25 UTC (History)
8 users (show)

See Also:


Attachments

Description Boris Zbarsky 2013-10-18 18:50:34 UTC
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
Comment 1 Anne 2013-10-21 10:48:27 UTC
So we want cloneNode() and importNode() to have different signatures?

Note that cloneNode() in Chrome defaults to true as well.
Comment 2 Boris Zbarsky 2013-10-21 13:19:18 UTC
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.
Comment 4 Erik Arvidsson 2013-10-21 14:16:28 UTC
OK. I'll update Blink. Should be safe since the default to true case is still hot out of the oven.
Comment 5 Boris Zbarsky 2013-10-21 14:52:12 UTC
> 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.
Comment 6 Boris Zbarsky 2013-10-21 14:53:30 UTC
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.
Comment 7 Boris Zbarsky 2013-10-21 14:54:29 UTC
> already alerts "0" and then "2" 

Er, "0" and then "1", since I got rid of the whitespace before <script>.  ;)
Comment 8 Domenic Denicola 2013-10-21 15:03:15 UTC
> 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?
Comment 9 Anne 2013-10-21 15:06:56 UTC
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...
Comment 10 Ms2ger 2013-10-21 15:33:41 UTC
I disagree. It would be better for cloneNode() to throw than to do a shallow clone.
Comment 11 Anne 2013-10-21 15:37:55 UTC
We cannot throw for undefined and we want undefined to be false so omitted should also be false.
Comment 12 Boris Zbarsky 2013-10-21 19:18:40 UTC
> 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.
Comment 13 Erik Arvidsson 2013-10-21 19:37:07 UTC
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.
Comment 14 Boris Zbarsky 2013-10-21 19:52:18 UTC
> 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.  :(
Comment 15 Boris Zbarsky 2013-10-21 19:56:50 UTC
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.
Comment 16 Erik Arvidsson 2013-10-21 20:24:32 UTC
(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.
Comment 17 Boris Zbarsky 2013-10-21 21:49:26 UTC
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.
Comment 18 Boris Zbarsky 2013-10-21 21:50:33 UTC
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.  :(
Comment 19 Anne 2013-10-22 09:40:15 UTC
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.
Comment 20 Boris Zbarsky 2013-10-23 06:03:10 UTC
That doesn't answer my question from comment 17, note.
Comment 21 Boris Zbarsky 2013-10-23 06:14:20 UTC
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....
Comment 22 Anne 2013-10-23 10:00:47 UTC
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.
Comment 23 Boris Zbarsky 2013-10-29 22:11:15 UTC
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...
Comment 24 Boris Zbarsky 2013-10-29 23:57:15 UTC
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...
Comment 25 Anne 2013-10-30 13:38:15 UTC
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.
Comment 26 Olli Pettay 2013-10-30 13:54:07 UTC
What is surprising if passing an argument has different behavior than
not passing an argument?
Comment 28 Boris Zbarsky 2013-12-20 19:25:19 UTC
We're going to align with the new spec here, fwiw, since it seems like no one else is interested in doing something different....