document.adoptNode() throws NOT_IMPLEMENTED in Gecko 1.8

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
DOM: Core & HTML
P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: vlad, Assigned: peterv)

Tracking

({fixed1.8.1.13})

1.8 Branch
mozilla1.9beta4
fixed1.8.1.13
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.13 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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...
(Assignee)

Updated

10 years ago
Assignee: nobody → peterv
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.
Flags: blocking1.9?
(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.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1

Comment 4

10 years ago
Why is this a 1.9+ P1 bug?

Comment 5

10 years ago
Setting TM to M11 to reflect that this is not a b2 blocker...
Target Milestone: --- → mozilla1.9 M11
(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.)
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.
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.
(Assignee)

Comment 9

10 years ago
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

10 years ago
(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?
(Assignee)

Comment 11

10 years ago
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.
Is this going to make beta 3, Peter?

Comment 13

9 years ago
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.
Flags: blocking1.8.1.13+
Priority: P1 → P2
Whiteboard: [needs patch]
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4
(Assignee)

Comment 14

9 years ago
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).
Attachment #299237 - Flags: superreview?(bzbarsky)
Attachment #299237 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
Hardware: PC → All
Whiteboard: [needs patch]
Attachment #299237 - Flags: superreview?(bzbarsky)
Attachment #299237 - Flags: superreview+
Attachment #299237 - Flags: review?(bzbarsky)
Attachment #299237 - Flags: review+
(Assignee)

Comment 15

9 years ago
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.
Attachment #299237 - Flags: approval1.8.1.13?
Comment on attachment 299237 [details] [diff] [review]
v1

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #299237 - Flags: approval1.8.1.13? → approval1.8.1.13+
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed1.8.1.13
Resolution: --- → FIXED

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.