Implement cloneNode() for HTML Document nodes

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
19 years ago
5 years ago

People

(Reporter: taras.tielkes, Assigned: sharparrow1)

Tracking

(Blocks: 1 bug, {dom1})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

19 years ago
CCing original reporter in DOM n.g.

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

Updated

19 years ago
QA Contact: gerardok → desale

Comment 1

19 years ago
Thanks Taras

    alert(document.cloneNode(true));

returns null

I assume it is the same for all XML/XUL/HTML documents.
This is implemented for XML documents, but not HTML nor XUL.
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.
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Keywords: dom1
Component: DOM Level 1 → DOM Core

Comment 4

18 years ago
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala

Updated

17 years ago
Priority: P3 → P4
Summary: Implement cloneNode() for the Document node → Implement cloneNode() for XUL and HTML Documents
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
Status: ASSIGNED → NEW
QA Contact: stummala → ian
HTMLDocument.cloneNode(false) returns null, while XULDocument.cloneNode(false)
throws an exception (DOM_NOT_SUPPORTED).  Not nice.
Priority: P4 → --
Target Milestone: Future → ---

Updated

12 years ago
Summary: Implement cloneNode() for XUL and HTML Documents → Implement cloneNode() for XUL and HTML Document nodes

Updated

11 years ago
Blocks: 410460
No longer blocks: 410460
Blocks: 410460
I think it should be included in 1.9.1
Flags: wanted1.9.1?
Taking.
Assignee: general → Olli.Pettay

Updated

10 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
(Assignee)

Comment 9

10 years ago
Created attachment 336404 [details] [diff] [review]
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 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)
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

10 years ago
Created attachment 336600 [details] [diff] [review]
WIP 2

Current state of my tree; I still need to address some of the XXX's and add a mochitest.
Assignee: Olli.Pettay → sharparrow1
Attachment #336404 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 14

10 years ago
Created attachment 336761 [details]
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.)
Attachment #336600 - Attachment is obsolete: true
Attachment #336761 - Flags: review?(Olli.Pettay)
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+ :)
Attachment #336761 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 16

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

Comment 17

10 years ago
Created attachment 336908 [details] [diff] [review]
Patch v2

Patch with review comments fixed.  (I also added a test for the correct resolution of <a href>.)
Attachment #336761 - Attachment is obsolete: true
Attachment #336908 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

10 years ago
Attachment #336908 - Flags: superreview?(jst)

Updated

10 years ago
Attachment #336908 - Flags: superreview?(jst) → superreview+
Should this have the checkin-needed keyword?
(Assignee)

Comment 19

10 years ago
(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.
(In reply to comment #19)
> I'm planning to checkin today.

Any news?  Or should somebody else land it for you?
(Assignee)

Comment 21

10 years ago
(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 :)
Keywords: checkin-needed
Pushed and filed a follow-up bug 455856.
author	sharparrow1@yahoo.com
	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
Blocks: 455856
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Summary: Implement cloneNode() for XUL and HTML Document nodes → Implement cloneNode() for HTML Document nodes
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...

Updated

10 years ago
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.