Last Comment Bug 390261 - document.adoptNode() throws NOT_IMPLEMENTED in Gecko 1.8
: document.adoptNode() throws NOT_IMPLEMENTED in Gecko 1.8
Status: RESOLVED FIXED
: fixed1.8.1.13
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 1.8 Branch
: All All
: P2 normal (vote)
: mozilla1.9beta4
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-30 22:45 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2008-07-31 02:46 PDT (History)
12 users (show)
jonas: blocking1.9+
mtschrep: blocking1.8.1.13+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.42 KB, patch)
2008-01-25 10:25 PST, Peter Van der Beken [:peterv]
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.13+
Details | Diff | Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2007-07-30 22:45:34 PDT
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 [:bz] 2007-07-30 22:48:04 PDT
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 -- probably not reading bugmail closely) 2007-12-04 06:36:29 PST
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] 2007-12-04 11:31:21 PST
(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 2007-12-05 20:26:37 PST
Why is this a 1.9+ P1 bug?
Comment 5 Mike Schroepfer 2007-12-05 21:23:19 PST
Setting TM to M11 to reflect that this is not a b2 blocker...
Comment 6 Mike Shaver (:shaver -- probably not reading bugmail closely) 2007-12-06 08:19:24 PST
(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, jst@mozilla.com) 2007-12-06 12:52:09 PST
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 -- probably not reading bugmail closely) 2007-12-06 13:11:46 PST
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] 2007-12-06 13:39:38 PST
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 2008-01-25 09:10:03 PST
(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] 2008-01-25 10:25:23 PST
Created attachment 299237 [details] [diff] [review]
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) 2008-01-28 17:36:11 PST
Is this going to make beta 3, Peter?
Comment 13 Mike Schroepfer 2008-01-29 20:58:50 PST
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] 2008-02-21 01:48:45 PST
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] 2008-02-21 11:20:07 PST
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] 2008-02-22 11:19:28 PST
Comment on attachment 299237 [details] [diff] [review]
v1

approved for 1.8.1.13, a=dveditz for release-drivers

Note You need to log in before you can comment on or make changes to this bug.