42976 - Implement cloneNode() for HTML Document nodes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Taras Tielkes]

CCing original reporter in DOM n.g.

John, could you add a tiny testcase that shows what goes wrong?

Comment 1

[hand]

Thanks Taras

    alert(document.cloneNode(true));

returns null

I assume it is the same for all XML/XUL/HTML documents.

Comment 2

[Johnny Stenback (:jst)]

This is implemented for XML documents, but not HTML nor XUL.

Comment 3

[Johnny Stenback (:jst)]

This bug has been marked "future" because the original netscape engineer working
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way
-- please attach your concern to the bug for reconsideration.

Comment 4

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 5

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 6

[Alex Vincent [:WeirdAl]]

HTMLDocument.cloneNode(false) returns null, while XULDocument.cloneNode(false)
throws an exception (DOM_NOT_SUPPORTED).  Not nice.

Comment 7

[Gabriele Best [:Gabri]]

I think it should be included in 1.9.1

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Taking.

Comment 9

[Eli Friedman]

Attachment: WIP patch

Attached implements cloneNode() for HTML.  I remember this being a lot more difficult the last time I looked at it, but this doesn't seem too complicated.  This patch does two things: one is to move the implementation of CloneNode up from nsXMLDocument to nsDocument, and the other is to add an implementation of nsHTMLDocument::Clone.  The version of nsHTMLDocument::Clone in this patch is adapted from nsXMLDocument::Clone; I'm not sure if there is anything special about HTML documents that needs to be taken into account.

I really don't know what sort of issues this is likely to run into, so I don't know what tests to write other than just a basic test to check that this works.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 336404 [details] [diff] [review]
WIP patch

Indeed, this should be almost enough. You could move SetLoadedAsData() from nsXMLDocument.h to nsDocument.h and set the cloned document to be a data document.

The test could contain checks that the clone of XHTML document is still XHTML document and clone of HTML document is still HTML document (ie. case handling of tag names should stay correct).
You could also check that .open()/.write()/.close() do something reasonable with cloned HTML documents. (I think .write() won't work, but the test could be todo() and there could be a followup bug)

Comment 11

[Eli Friedman]

(In reply to comment #10)
> (From update of attachment 336404 [details] [diff] [review])
> Indeed, this should be almost enough.

Okay, cool.

> You could move SetLoadedAsData() from
> nsXMLDocument.h to nsDocument.h and set the cloned document to be a data
> document.

Okay.

> The test could contain checks that the clone of XHTML document is still XHTML
> document and clone of HTML document is still HTML document (ie. case handling
> of tag names should stay correct).

Okay.  Actually, the posted version fails that test... and from there, I discovered that there's a lot of other state that I think it makes sense to copy.  I'll post another WIP in a bit.

Planned tests: make sure the the following don't change with cloning: tag name case behavior, innerHTML (reading and writing), document.dir, document.compatMode, document.contentType (XHTML, HTML, and plain text), document.querySelectorAll(":lang(ja)") (+ meta Content-Language), document.characterSet, document.xmlStandalone, document.xmlEncoding.  (Note that XMLDocument doesn't currently pass all of these tests.)

> You could also check that .open()/.write()/.close() do something reasonable
> with cloned HTML documents.

With my patch, they do nothing; they bail out early when there isn't a docshell. It might be nicer to throw a NS_ERROR_NOT_IMPLEMENTED or something like that, but I'm definitely not going to mess with it more than that for the moment; it's quite complex.

Comment 12

[Eli Friedman]

(In reply to comment #11)
> Planned tests: make sure the the following don't change with cloning: tag name
> case behavior, innerHTML (reading and writing), document.dir,
> document.compatMode, document.contentType (XHTML, HTML, and plain text),
> document.querySelectorAll(":lang(ja)") (+ meta Content-Language),
> document.characterSet, document.xmlStandalone, document.xmlEncoding.  (Note
> that XMLDocument doesn't currently pass all of these tests.)

Actually, I'm not sure if cloning a document is supposed to pass all of these tests; any thoughts?

Comment 13

[Eli Friedman]

Attachment: WIP 2

Current state of my tree; I still need to address some of the XXX's and add a mochitest.

Comment 14

[Eli Friedman]

Attachment: Patch v1

Okay, I think this is basically finished.  The only thing I'm not sure about is whether we should really be copying everything that the patch copies.

Oh, I just realized we aren't cloning documents that inherit from XMLDocument quite correctly... that might be better to address in a followup, though.  (Note to self: two followups so far: cloning stuff like SVG docs, and document.write in clones.)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 336761 [details]
Patch v1

>diff -r fb086cc13695 content/base/src/nsContentUtils.cpp
...
>@@ -3476,7 +3475,7 @@
> 
>   document->SetFragmentParser(parser);
> 
>-  return NS_OK;
>+  return rv;
What behavior does this change? Do you have a testcase for this (or is it even possible to have one)?

>+nsresult
>+nsDocument::CloneDocHelper(nsDocument* clone) const
>+{
>+  // Init document
>+  nsresult rv = clone->Init();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Set URI/principal
>+  clone->nsDocument::SetDocumentURI(nsIDocument::GetDocumentURI());
>+  // Must set the principal first, since SetBaseURI checks it.
>+  clone->SetPrincipal(NodePrincipal());
>+  rv = clone->SetBaseURI(nsIDocument::GetBaseURI());
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Set scripting object
>+  PRBool hasHadScriptObject = PR_TRUE;
>+  nsIScriptGlobalObject* scriptObject =
>+    GetScriptHandlingObject(hasHadScriptObject);
>+  NS_ENSURE_STATE(scriptObject || !hasHadScriptObject);
>+  clone->SetScriptHandlingObject(scriptObject);
>+
>+  // Make the clone a static document
>+  clone->SetLoadedAsData(PR_TRUE);
'static'? I would call it a data document.

>+  // State from nsIDocument
>+  // XXX Not sure if we want to copy all of this.
>+  clone->mCharacterSet = mCharacterSet;
>+  clone->mCharacterSetSource = mCharacterSetSource;
>+  clone->mCompatMode = mCompatMode;
>+  clone->mBidiOptions = mBidiOptions;
>+  clone->mContentLanguage = mContentLanguage;
>+  clone->mContentType = mContentType;
>+  clone->mSecurityInfo = mSecurityInfo;
>+  clone->mDocumentLoadGroup = mDocumentLoadGroup;
I'm not sure about the loadgroup. Data documents shouldn't really load anything anyway - at least not
currently. And even xmlDocument.load uses caller's loadgroup.

>+  // State from nsDocument
>+  // XXX: Should we copy mHeaderData? It seems like we should copy
>+  // some things, like Content-Style-Type.
Currently even Content-Style-Type doesn't matter, because it is handled only
on style elements in non-data-documents. So ok to me if not copied for now.
If we want to start to clone documents for example for printing, then that information is needed, but
such clone documents are anyway a bit special, I think

>+  clone->mIsRegularHTML = mIsRegularHTML;
>+  clone->mXMLDeclarationBits = mXMLDeclarationBits;
>+  // XXX: Should we copy mBaseTarget?
I think so. That way clone documents <a> elements' .target keep returning the same
value as in the original document. (Though there is still problem with nsGkAtoms::htmlBaseTarget properties, but that should be handled in element cloning. So perhaps a different bug.)

>+  // State from nsHTMLDocument
>+  // XXX Should we copy mLoadFlags?
Used only by WyciwygChannel. If mSecurityInfo is copied, then also this one should be.
So I think better to copy, even if it isn't used atm.

>diff -r fb086cc13695 content/xul/document/src/nsXULDocument.cpp
....
>-    // The destructor of nsDocument will delete references to style
>-    // sheets, but we don't want that if we're a popup document, so
>-    // then we'll clear the stylesheets array here to prevent that
>-    // from happening.
>-    if (mIsPopup) {
>-        mStyleSheets.Clear();
>-        mStyleAttrStyleSheet = nsnull;
>-        mAttrStyleSheet = nsnull;
>-    }
This is not for this bug.

Almost r+ :)

Comment 16

[Eli Friedman]

(In reply to comment #15)
> (From update of attachment 336761 [details])
> >diff -r fb086cc13695 content/base/src/nsContentUtils.cpp
> ...
> >@@ -3476,7 +3475,7 @@
> > 
> >   document->SetFragmentParser(parser);
> > 
> >-  return NS_OK;
> >+  return rv;
> What behavior does this change? Do you have a testcase for this (or is it even
> possible to have one)?

Ah, sorry, I didn't mean to include that in this diff.  I was just using it for debugging an issue with clones and innerHTML.  This doesn't cause any significant behvior change.

> >+  // Make the clone a static document
> >+  clone->SetLoadedAsData(PR_TRUE);
> 'static'? I would call it a data document.

Okay.

> >+  // State from nsIDocument
> >+  // XXX Not sure if we want to copy all of this.
> >+  clone->mCharacterSet = mCharacterSet;
> >+  clone->mCharacterSetSource = mCharacterSetSource;
> >+  clone->mCompatMode = mCompatMode;
> >+  clone->mBidiOptions = mBidiOptions;
> >+  clone->mContentLanguage = mContentLanguage;
> >+  clone->mContentType = mContentType;
> >+  clone->mSecurityInfo = mSecurityInfo;
> >+  clone->mDocumentLoadGroup = mDocumentLoadGroup;
> I'm not sure about the loadgroup. Data documents shouldn't really load anything
> anyway - at least not
> currently. And even xmlDocument.load uses caller's loadgroup.

Okay, makes sense.

> >+  // State from nsDocument
> >+  // XXX: Should we copy mHeaderData? It seems like we should copy
> >+  // some things, like Content-Style-Type.
> Currently even Content-Style-Type doesn't matter, because it is handled only
> on style elements in non-data-documents. So ok to me if not copied for now.

Right... makes sense.

> >+  clone->mIsRegularHTML = mIsRegularHTML;
> >+  clone->mXMLDeclarationBits = mXMLDeclarationBits;
> >+  // XXX: Should we copy mBaseTarget?
> I think so. That way clone documents <a> elements' .target keep returning the
> same
> value as in the original document.

Okay.

> (Though there is still problem with
> nsGkAtoms::htmlBaseTarget properties, but that should be handled in element
> cloning. So perhaps a different bug.)

Yeah... we can't clone the properties while we're cloning the document because there wouldn't be any content for the properties to refer to.

> >+  // State from nsHTMLDocument
> >+  // XXX Should we copy mLoadFlags?
> Used only by WyciwygChannel. If mSecurityInfo is copied, then also this one
> should be.
> So I think better to copy, even if it isn't used atm.

Okay.

> >diff -r fb086cc13695 content/xul/document/src/nsXULDocument.cpp
> ....
> >-    // The destructor of nsDocument will delete references to style
> >-    // sheets, but we don't want that if we're a popup document, so
> >-    // then we'll clear the stylesheets array here to prevent that
> >-    // from happening.
> >-    if (mIsPopup) {
> >-        mStyleSheets.Clear();
> >-        mStyleAttrStyleSheet = nsnull;
> >-        mAttrStyleSheet = nsnull;
> >-    }
> This is not for this bug.

Sorry about that; I was sure I had cleaned those bits out of the diff.

> Almost r+ :)

Okay, thanks.

Comment 17

[Eli Friedman]

Attachment: Patch v2

Patch with review comments fixed.  (I also added a test for the correct resolution of <a href>.)

Comment 18

[Mauricio Collares [:collares]]

Should this have the checkin-needed keyword?

Comment 19

[Eli Friedman]

(In reply to comment #18)
> Should this have the checkin-needed keyword?

No... I just haven't had access to my dev computer since the patch was granted sr.  I'm planning to checkin today.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #19)
> I'm planning to checkin today.

Any news?  Or should somebody else land it for you?

Comment 21

[Eli Friedman]

(In reply to comment #20)
> (In reply to comment #19)
> > I'm planning to checkin today.
> 
> Any news?  Or should somebody else land it for you?

Bleh, I was meaning to check in, and didn't get around to it. When I checked last night, the tree was closed, and the tree is burning right now.  It really isn't easy to check stuff in if you're unlucky :)

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Pushed and filed a follow-up bug 455856.
author	[e-mail]
	Thu Sep 18 14:15:47 2008 +0300 (at Thu Sep 18 14:15:47 2008 +0300)
changeset 19384	516d3c5c3da6
parent 19383	0879f08590a9
Bug 42976, cloneNode() for HTML Document nodes, r=smaug,sr=jst

Comment 23

[Boris Zbarsky [:bzbarsky]]

Why are we copying the script handling object?  I would have thought the right thing when cloning a document would be to create a document with no script anything, and with a disabled scriptloader/cssloader, etc...