390261 - document.adoptNode() throws NOT_IMPLEMENTED in Gecko 1.8

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

In 1.9 we're throwing DOM_WRONG_DOCUMENT if you try to use a node in a different document.  The "fix" should be calling document.adoptNode.  This works in 1.9, except that in Gecko 1.8, adoptNode throws NOT_IMPLEMENTED.

So now we have the situation where we broke compat, but the fix isn't backwards-compatible -- requiring authors to implement two separate firefox paths.  This kinda sucks (especially since they can't test for whether adoptNode exists at all, they'd have to do gecko version sniffing).

I suggest that we do a small patch for 1.8 that makes adoptNode a noop.  Because it currently throws, and because IE doesn't implement it, I think we can safely assume that the current callers are near-zero so this should be safe.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Per DOM spec, for what it's worth, just returning NS_OK and setting the out param to null would even be correct.  The question is whether we want to set the out param to the input node...

Comment 2

[Mike Shaver (:shaver emeritus)]

This compatibility pain adds to the already significant frustration of having to add adoptNode to something that was already working in Firefox 2, and in other browsers as well.  Requesting blocking-1.9 for this 1.8-targetted patch because I think we need to solve it before we get Fx3 into the wild.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #1)
> Per DOM spec, for what it's worth, just returning NS_OK and setting the out
> param to null would even be correct.  The question is whether we want to set
> the out param to the input node...

I would set it to the input node; otherwise we don't fix anything, unless we want to try to train authors to write something like...

  var newNode = doc.adoptNode(node);
  newNode = newNode ? newNode : node;

seems like a lot of pain for not much gain; the trunk will still return null in some situations (right?) but that's not going to happen in normal usage.

Comment 4

[Mike Schroepfer]

Why is this a 1.9+ P1 bug?

Comment 5

[Mike Schroepfer]

Setting TM to M11 to reflect that this is not a b2 blocker...

Comment 6

[Mike Shaver (:shaver emeritus)]

(TBH, I think we should consider just backing out the document-splitting behaviour: Safari and Opera have both followed our old lead, and I don't think they're likely to switch back.  We were originally following IE's lead on this 2 years ago, but the web's application mix has changed a lot in that time, and I think we're inflicting pain on developers and users without any benefit.)

Comment 7

[Johnny Stenback (:jst)]

The benefit here, beyond spec and IE compatibility, is that there's a large number of web-developers out there develop using Firefox, and then port to IE etc. With the code as it is now, we'll inflict less pain for the developers in that category, and thus the benefit. But having said that, there's sites out there that break now because of this change (probably more than we know about at this point), so I'm leaning towards reverting to the old behavior and advocating for changing the specs.

Comment 8

[Mike Shaver (:shaver emeritus)]

That benefit sounds exactly like "IE compatibility", to me, I must confess. :)

If we didn't have this stricture in the spec, or if it were a more sane "MAY" rather than "MUST", I don't think anyone would be arguing for putting it to its current state.  That leads me to believe that the spec fragment here doesn't serve the needs of its constituents (implementers and consumers of the API), at least as far as the client side of the web goes.

I can imagine hypotheticals in which you are looking to inject a DOM out of a web3d plugin's DOM, but even then we could silently adoptNode where we detect that it's needed and throw if that fails.  Permitting implementations to be easier to use, while providing best practices to ease the authoring of extremely portable code, seems like the only sane path forward.  We could certainly warn in Firefox when we see a cross-document import, if that doesn't have significant performance impact.

Comment 9

[Peter Van der Beken [:peterv]]

I don't think we should have the discussion about WRONG_DOCUMENT_ERR on trunk in this bug. Regardless of what we decide for that, fixing adoptNode to not throw will make it easier to write cross-brower code. AdoptNode with fallback to importNode would work on almost all browsers then.

Comment 10

[Mike Schroepfer]

(In reply to comment #9)
> I don't think we should have the discussion about WRONG_DOCUMENT_ERR on trunk
> in this bug. Regardless of what we decide for that, fixing adoptNode to not
> throw will make it easier to write cross-brower code. AdoptNode with fallback
> to importNode would work on almost all browsers then.
> 

So what's the right next step on this?

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: v1

Sorry, I keep forgetting about this. The next step is probably getting this patch in. Still need to test it.

Comment 12

[Damon Sicore (:damons)]

Is this going to make beta 3, Peter?

Comment 13

[Mike Schroepfer]

Adding for the next 1.8 release (1.8.1.13) and leaving on the 1.9 blocker list to make sure it lands in 1.8 before we ship 1.9.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 299237 [details] [diff] [review]
v1

finally found some time to go over the remaining adoptNode failures in the DOM testsuite, they're all because of other broken/missing features (Attr's specified property, attr's AppendChild, entities).

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 299237 [details] [diff] [review]
v1

Simple fix to make adoptNode not throw on the branch, so it's possible to use adoptNode with both Firefox 2 and 3.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 299237 [details] [diff] [review]
v1

approved for 1.8.1.13, a=dveditz for release-drivers