You are viewing a plain text version of this content. The canonical link for it is here.
Posted to j-dev@xerces.apache.org by Lynn Monson <lm...@flipdog.com> on 2000/10/23 22:55:01 UTC
Range insertNode bug
While using Range.insertNode(), I came across what I think are two small,
but significant bugs. The existing Xerces code for Range.insertNode()
assumes that the end point of the range is unrelated to the starting point.
However, if the ending container is the same as the start container, or if
the end container is the parent of the start container, the range is left in
an incorrect state.
For example:
Assume my document is the following:
<root>abc</root>
If my range is:
start container: abc
start offset: 1
end container: abc
end offset: 3
Then after inserting the node <foo/>, I am left with the document:
<root>a<foo/>bc</root>
But my range was not updated correctly. It reads:
start container: a
start offset: 1
end container: a
end offset: 3
The end container is incorrect, and the end offset is illegal. I believe
the range should be:
end container: bc
end offset: 2
A similar situation exists if the original range had used <root> as the end
container with an offset of 1. Included below is a suggested fix for this
problem. As always, thanks in advance for considering it.
Index: RangeImpl.java
===================================================================
RCS file:
/home/cvspublic/xml-xerces/java/src/org/apache/xerces/dom/RangeImpl.java,v
retrieving revision 1.8
diff -u -p -r1.8 RangeImpl.java
--- RangeImpl.java 2000/10/19 17:10:29 1.8
+++ RangeImpl.java 2000/10/23 19:09:54
@@ -667,6 +667,18 @@ public class RangeImpl implements Range
boolean MULTIPLE_MODE = false;
if (fStartContainer.getNodeType() == Node.TEXT_NODE) {
if (newNode.getNodeType()!= Node.TEXT_NODE) { // result is 3
text nodes...
+
+ // As written, the following code always inserts exactly
+ // two new nodes. One is the "newNode" passed as an
argument,
+ // and the other is the clone of the original text node.
+ // However, the above code that excludes
+ // DOCUMENT_FRAGMENT_NODE from being inserted is an error.
+ // When that is corrected, the content of the fragment will
+ // be inserted. That can mean any number of additional
+ // nodes will be inserted. This variable is a placeholder
+ // to count the number of nodes that were inserted.
+ int numNodesInserted = 2;
+
cloneCurrent = fStartContainer.cloneNode(false);
((TextImpl)cloneCurrent).setNodeValueInternal(
(cloneCurrent.getNodeValue()).substring(fStartOffset));
@@ -686,6 +698,20 @@ public class RangeImpl implements Range
parent.appendChild(cloneCurrent);
}
}
+
+ // If the end container was the same as the
+ // start container, we need to refer instead
+ // to the new, split, text node
+ if (fEndContainer == fStartContainer) {
+ fEndContainer = cloneCurrent;
+ fEndOffset -= fStartOffset;
+ }
+ else
+ if (fEndContainer == fStartContainer.getParentNode() )
+ {
+ fEndOffset += numNodesInserted;
+ }
+
// signal other Ranges to update their start/end
containers/offsets
signalSplitData(fStartContainer, cloneCurrent,
fStartOffset);
Re: Range insertNode bug
Posted by Elena Litani <hl...@jtcsv.com>.
Lynn, thanks for reporting the bug and producing a patch.
While looking at it, I discovered some other problems with insertNode().
So I compared the implementation against specs and here are the changes
I came up with:
1. Proper exception handling (which was not done)
2. Include handling DocumentFragment. Actually, it is handled
automatically by insertBefore(), or appendChild(). So the only thing I
needed to do is not to throw "DOM012 Invalid node type" exception.
3. Update ranges for insertion
a) Lynns patch for text nodes; I have changed it a little bit to
incorporate DocumentFragment insertions
b) Updating ranges for all other nodes also need to be done.
ex.
DOM tree:<body><p/></body>.
Range(start;end): body,0; body,1
After insertion of insert <h1>:
DOM tree: <body></h1><p/></body>.
Range(start;end): body,0; body,2
4. Deleted normalization of text nodes. According to specs (2.9)
normalization should be handled by application. That is if 2 adjacent
Text nodes are created -> we don't merge 'em.
There is a special case which is handled the same:
fStartContainer=text node and fStartOffset=0. We still split text node
although one of the resulting text nodes will be empty.
Anyways, I attach patch.
Elena
Lynn Monson wrote:
>
> While using Range.insertNode(), I came across what I think are two small,
> but significant bugs. The existing Xerces code for Range.insertNode()
> assumes that the end point of the range is unrelated to the starting point.
> However, if the ending container is the same as the start container, or if
> the end container is the parent of the start container, the range is left in
> an incorrect state.
>
> For example:
>
> Assume my document is the following:
> <root>abc</root>
>
> If my range is:
>
> start container: abc
> start offset: 1
> end container: abc
> end offset: 3
>
> Then after inserting the node <foo/>, I am left with the document:
>
> <root>a<foo/>bc</root>
>
> But my range was not updated correctly. It reads:
>
> start container: a
> start offset: 1
> end container: a
> end offset: 3
>
> The end container is incorrect, and the end offset is illegal. I believe
> the range should be:
>
> end container: bc
> end offset: 2
>
> A similar situation exists if the original range had used <root> as the end
> container with an offset of 1. Included below is a suggested fix for this
> problem. As always, thanks in advance for considering it.
>
> Index: RangeImpl.java
> ===================================================================
> RCS file:
> /home/cvspublic/xml-xerces/java/src/org/apache/xerces/dom/RangeImpl.java,v
> retrieving revision 1.8
> diff -u -p -r1.8 RangeImpl.java
> --- RangeImpl.java 2000/10/19 17:10:29 1.8
> +++ RangeImpl.java 2000/10/23 19:09:54
> @@ -667,6 +667,18 @@ public class RangeImpl implements Range
> boolean MULTIPLE_MODE = false;
> if (fStartContainer.getNodeType() == Node.TEXT_NODE) {
> if (newNode.getNodeType()!= Node.TEXT_NODE) { // result is 3
> text nodes...
> +
> + // As written, the following code always inserts exactly
> + // two new nodes. One is the "newNode" passed as an
> argument,
> + // and the other is the clone of the original text node.
> + // However, the above code that excludes
> + // DOCUMENT_FRAGMENT_NODE from being inserted is an error.
> + // When that is corrected, the content of the fragment will
> + // be inserted. That can mean any number of additional
> + // nodes will be inserted. This variable is a placeholder
> + // to count the number of nodes that were inserted.
> + int numNodesInserted = 2;
> +
> cloneCurrent = fStartContainer.cloneNode(false);
> ((TextImpl)cloneCurrent).setNodeValueInternal(
> (cloneCurrent.getNodeValue()).substring(fStartOffset));
> @@ -686,6 +698,20 @@ public class RangeImpl implements Range
> parent.appendChild(cloneCurrent);
> }
> }
> +
> + // If the end container was the same as the
> + // start container, we need to refer instead
> + // to the new, split, text node
> + if (fEndContainer == fStartContainer) {
> + fEndContainer = cloneCurrent;
> + fEndOffset -= fStartOffset;
> + }
> + else
> + if (fEndContainer == fStartContainer.getParentNode() )
> + {
> + fEndOffset += numNodesInserted;
> + }
> +
> // signal other Ranges to update their start/end
> containers/offsets
> signalSplitData(fStartContainer, cloneCurrent,
> fStartOffset);
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: xerces-j-dev-unsubscribe@xml.apache.org
> For additional commands, e-mail: xerces-j-dev-help@xml.apache.org