135922 - [FIXr]Range.insertNode doesn't split textnodes

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

The spec says "If the start boundary point of the Range is in a Text node, the
insertNode operation splits the Text node at the boundary point." Mozilla
doesn't handle this correctly, we don't split the text node but instead try to
append the new node to the text node, which of course always fails.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Testcase

Comment 2

[Nathan Kurz]

Unfortunately, the problem is not just that the text node is not split, but that
nsRange::insertNode() is broken in just about every conceivable way. 

http://lxr.mozilla.org/seamonkey/source/content/base/src/nsRange.cpp#2059 

2073   if( (nsIDOMNode::CDATA_SECTION_NODE == tNodeType) ||
2074       (nsIDOMNode::TEXT_NODE == tNodeType) )

Should be checking type of tStartContainer, not type of node to be inserted.

2081     return tSCParentNode->InsertBefore(aN, tSCParentNode,   
                                            getter_AddRefs(tResultNode));

1) Need to splitText if necessary (see SurroundContents for details)
2) Second arg should be tStartContainer, so we don't try to insert before self.

2092   if(tStartOffset == (PRInt32)tChildListLength)

Comparison shouldn't be necessary.  tChildList->Item() should return null if 
offset greater than tChildListLength, and tStartContainer->InsertBefore reverts
to append if tChildNode is null.

Comment 3

[Nathan Kurz]

Attachment: Another test case

Here's another test case for range.insertNode()

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Version of peterv's testcase that does not go into infinite recursion if insertNode works right

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch based on Nathan's suggestion

Comment 6

[Boris Zbarsky [:bzbarsky]]

taking

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 121596 [details] [diff] [review]
Patch based on Nathan's suggestion

Yay, nsRange!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121596 [details] [diff] [review]
Patch based on Nathan's suggestion

Requesting 1.4b approval; this is a simple change to bring us in line with the
spec and prevent content disappearing completely when insertNode is used on
ranges.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 121596 [details] [diff] [review]
Patch based on Nathan's suggestion

a=asa (on behalf of drivers) for checkin to 1.4b

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed.