17726 - splitText() on CDATASection should make new CDATASection

Status: VERIFIED FIXED

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

Description

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

DESCRIPTION:  Right now, splitText() on a CDATASection is creating a new Text
node rather than a new CDATASection node.  It should *probably* create a new
CDATASection node instead, although the DOM spec isn't crystal clear on this
point.

STEPS TO REPRODUCE:
 * load http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/CDATASection
 * press the button

ACTUAL RESULTS:
 * some test lines are red

EXPECTED RESULTS:
 * all lines should be green, and the test should run until "Tests finished (but
not all written)." is the bottom line in the test output area.  All lines
currently red can be attributed to this bug once my fix for bug 9779, or another
fix for it, is checked in.

DOES NOT WORK CORRECTLY ON:
 * Linux, apprunner, 1999-10-30-08-M11

Comment 1

[jst]

I have a fix for this but for some reason I'm unable to create an attachment,
in stead I made the patch available at

      http://www.citec.fi/~jst/nsGenericDOMDataNode.cpp.diff

The patch uses nsIDOMNode::CloneNode() in SplitText to create the new node,
this way if we're splitting a node the new node will allways be of same type
as the node we're splitting.

Vidur, let me know if this looks OK and I'll check it in.

Comment 2

[vidur (gone)]

In an attempt to get my bug list in order again, marking all the bugs I have
currently as ASSIGNED.

Comment 3

[vidur (gone)]

Sorry it took me so long to get to this bug. As David mentioned, the DOM
Level 1 spec wasn't completely clear about what type of node to create. We
discussed this issue a few weeks ago and modified the wording to clarify that
the new node should be of the same type as the original.

Thanks for the patch Johnny. The one issue with it is that the CloneNode() does
an unnecessary copy of the text in the original node. An ugly alternative is to
move splitText out of the generic inner object into the individual classes.
Another option is to implement a CloneNode() variant on in internal interface
(say nsITextContent) that doesn't do a copy. Hmm...

Comment 4

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

Does it?  The CloneNode is called with PR_FALSE.

Comment 5

[jst]

David, it does, and IMO it also should according to the DOM spec, correct me
if I'm wrong.

Vidur, one possible sollution to the extra copying would be something close
to this...

	PRUnicahr *newContent = new PRUnichar[length - aOffset + 1];

	mText.CopyTo(newText, aOffset, length - aOffset);

	// Replace our text with nsnull to avoid copying the text in
	// CloneNode();
	mText.SetTo((PRUnichar *)nsnull, 0, PR_FALSE);

	...

	CloneNode();

	...

	// This will take ownership of newText and delete it when mText
	// is deleted...
	mText.SetTo(newText, length - aOffset, PR_TRUE);

	...

This only does one copy of the text that remains in the original node. This
still does two copys of the data in the new node, one when it gets the data
for the new node and one when it sets the content of the new node, this might be
harder to avoid...

Should I create a new patch with code that does this (note, we need to check
if mText holds char * or PRUnichar * but that is trivial)?

Comment 6

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

From http://www.w3.org/TR/REC-DOM-Level-1/level-one-core.html#ID-3A0ED0A4 :

Cloning an Element copies all attributes and their values, including those
generated by the XML processor to represent defaulted attributes, but this
method does not copy any text it contains unless it is a deep clone, since the
text is contained in a child Text node. Cloning any other type of node simply
returns a copy of this node.

Comment 7

[vidur (gone)]

David: Yes, but the first part of the paragraph only applies to Elements
(specifically, the notion of deep cloning only appies to nodes that can have
children). A copy of a Text node contains a copy of the character data from the
original.

John: If you want to post a modified patch, I'd be happy to integrate it.
Thanks!

Comment 8

[jst]

dbaron, I get

JavaScript Error: ReferenceError: canOutputWarn is not defined
URL: http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/CDATASection_2
LineNo: 18

from your CDATASection_2 test the first time I press the button, if I pres it
again the test runs! Is is just me or...

vidur, new patch comming soon...

Comment 9

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

canOutputWarn() is defined in a linked script.  I'm not sure what's wrong
there...

In regard to the previous comments, I must've been thinking text nodes were like
elements or something.  And once I start thinking something, it's sometimes hard
to stop!

Comment 10

[jst]

New patch available at

	http://www.citec.fi/~jst/SplitText.diff

The patch also includes a char * version of nsTextFragment::SetTo(...)
(PRUnicahr * version already existed), this is needed in SplitText to avoid
unnecessary copying of char * data.

Dbaron, the javascript error I saw yesterday didn't show up today, go figure...

Comment 11

[vidur (gone)]

Fixed on 12/21/1999. I decide to create a Clone() equivalent on nsITextContent
and parameterize whether the contained text was copied or not. Thanks for your
help guys!

Comment 12

[ckritzer (gone)]

Marking VERIFIED FIXED on:
- Linux6 2000-03-07-09 Commercial build
- MacOS9 2000-03-07-08 Commercial build
- Win98 2000-03-07-09 Commercial build