BuildID: 20001031 surroundContents DOM 2 Range method not yet implemented Reproducible: Always Steps to Reproduce: 1. attempt to surroundContents of a Range returns not implemented error Actual Results: NS_ERROR_NOT_IMPLEMENTED testcase to follow

*** Bug 58973 has been marked as a duplicate of this bug. ***

Thanks for catching the duplicate... Bugzilla failed when I submitted it, and wouldn't show me 58973 when I attempted to verify the submission.

Reporter is this still a problem in the latest nightlies?

It is still in the 20001211 nightly... Anthony is working on DOM Range bugs. You will see the error message in tools -> JavaScript console.

Marking NEW as per reporters comments.

setting milestone, and priority. anthonyd

finishing up range can be pushed off

I am attaching a JavaScript "Patch" that can be used as a work around for the range object. This patch adds support for the following: Range.innerHTML - read only Range.extractContents - as per W3C specs Range.cloneContents - as per W3C specs Range.insertNode - as per W3C specs Range.surroundContents - as per W3C specs Range.deleteContents - as per W3C specs - fixes buggy support from Mozilla Range.jmyCompareNode - extends Mozilla's compareNode to include the following 2 constants Range.NODE_BEFORE_AND_INSIDE = -1; Range.NODE_INSIDE_AND_AFTER = -2; I would like a C++ programmer to implement these in C if possible. Jeff Yates

When attaching textfiles to bugs please attach them as text/plain instead of in a zip file, thanks.

Jeff Yates, It looks like you didnt specify the mime type for your patches. Can you please reattach with text mime type please? thanks. anthonyd

moving to mozilla 0.9 anthonyd

moving to mozilla1.0

checked in

see 58969 for details

Reopening bug. The following code crashes Mozilla 0.9.2 (07-18-2001 build) on Win2k: var range = document.createRange(); range.selectNodeContents(document.lastChild); //this initializes the range so it can be tested var extractFrag = range.createContextualFragment( "<b>this is a fragment</b> use to " + "test <b>the surroundContents method</b>"); range.selectNode(extractFrag.childNodes[1]) var testEl = document.createElement("i"); if( range.surroundContents ) try{ alert("before surroundContents"); range.surroundContents(testEl); //crashes on this statement alert("after surroundContents"); } catch(e){} Jeff Yates http://www.pbwizard.com

Is the patch in this bug still needed, the crash was fixed by the fix for bug 91614, WORKSFORME now.

Milestone reality check.

i will check in the fix to keep this from crashing, but the whole method still needs to be re-written. anthonyd

no longer crashes with a null document frag. still need to be rewritten to work correctly in such a case though. that is currently scheduled for 1.0. anthonyd

Dear gentlemen, Why does this bug have status RESOLVED and resolution FIXED? I can still easily crash Mozilla 0.9.7 in this way. I was really hoping that I could use this feature, without it the Range implementation just isn't complete and fully unusable. Please comment on this, Martin van Dijken

A little Extra Info, I use the Mozilla 0.9.7 release build which is 2001122106 on a Win98 platform.

Reopening to make sure we investigate this further.

How about setting a new target milestone? 0.9.4 is _long_ out of the door.

Its noteable that the nifty feature that is bug 62467 could be enabled by the fixing of this bug.

The Target Milestone (0.9.4) is long passed. This bug should be retargeted. I can confirm that Mozilla no longer crashes, so the 'crash' keyword can be removed.

What is holding this fix back? Comment 30 indicates it no longer crashes. So can this now be landed? The milestone needs changed (and crash keyword removed). Please comment.

reassigning range bug to kin.

Removing Crash keyword based on comments. Not sure whats holding this one up.

This testcase shows two more uses of surroundContents, one for moving a paragraph, and one for highlighting a paragraph, and shows no errors or crashes using mozilla1.0.0

The attached testcase demonstrates problems using surroundContents(). Try it: - mark some text using the mouse - click the "Mark selection bold" button Expected result: - Selection is now in bold Actual result: - Nothing, or sometimes other parts of the text are suddenly made bold. (using 2002053012)

The previous testcase contains three different selection cases: * Selection across paragraphs (with partial selection) From the spec: The surroundContents() method raises an exception if the Range partially selects a non-Text node. An example of a Range for which surroundContents() raises an exception is: <FOO>AB<BAR>CD</BAR>E</FOO> This invalidates the previous testcase for any selection across paragraphs if the whole paragraph is not selected. However, mozilla is not throwing an exception which it should be. * Selection within a paragraph It seems that selection is ignored the first time the surroundContents command is called on a selection (though alerting the selection shows that the selection has occurred)... however, this seems to be an interaction problem between selection and surroundContents, not surroundContents on its own (this problem does not occur when surroundContents is used without selections). When the selection finally does occur, it mistakenly moves the surrounded range to become the first child of its parent instead of splitting a text node and inserting the new node between the split text nodes. This is an error in mozilla. * Selection across paragraphs (full selection of elements) Seems to work as expected.

In the last comment B<BAR>C should be considered the partial selection

Selection within same node is covered by http://bugzilla.mozilla.org/show_bug.cgi?id=135928 . So this bug now seems to only cover "surroundContents on invalid partial non-textNode selection does not throw an exception". Either the summary should be changed, or this bug should be closed, and a new bug opened for the missing exception.

It's been a year since the last comment on this. Could I ask for an update on the status of this bug?

Works in Mozilla 1.4RC3! (did not work in 1.3)

I have a fix that rewrites SurroundContents() and ExtractContents() so that things work better. Will post shortly.

Ok here's my 1st attempt at fixing SurroundContents() and ExtractContents().

Comment on attachment 133566 [details] [diff] [review] Patch Rev 1 (SurroundContents() and ExtractContents() rewrite) Never code when dying to sleep. I woke up this morning and remembered I forgot to change a few things that are different from the DeleteContents() case, like the order in which I insert into the doc fraga tree. Withdrawing review request. I'll post an updated patch sometime this weekend.

Okay this patch works. Before asking for review, I'm going to look into cleaning up some loose ends that existed prior to this patch, in particular support for ProcessingInstructions. Also the version of SurroundContents() in this patch only splits TextNodes, it should split all CharacterData and ProcessingInstructions.

Ok this version of the patch introduces some methods that operate on "DataNodes" which are CharacterData (TextNodes, Comments) and Processing Instructions. I've also modified CloneContents() and DeleteContents() to use these methods. While testing this patch, I noticed that it exposes a bug in selection, which can also be seen with the test case in this bug ... after hiliting some text and hitting the make bold button, the selection selects all the text before the text that was made bold ... I've verified that the selection is actually around the bold node, after the surround operation, but selection is just drawing the hiliting wrong.

review notes: In nsRange::SplitDataNode() after you have the comment: // Get the length of the data in aNode. you duplicate the GetDataLengthInDataNode() code. Call GetDataLengthInDataNode() instead. By the way do we really want to Clone() the node in SplitDataNode()? In the case of really huge text nodes (imagine just a big ole text docuemnt that is all in one text node) won't this cause twice the peak memory need? Is there a way we can create an empty node and split the underlying data without copying (I doubt it but I ask anyway)? Failing that, shouldn't we calculate which side of the split has less data and copy that side into a new node? Put a blank line before GetDataLengthInDataNode definition. In nsresult nsRange::ExtractContents(nsIDOMDocumentFragment** aReturn): where you have the while loop and check for (node == startContainer || node == endContainer) and do all the cloning... why not just make use of the SplitDataNode() function you already wrote to split the data nodes, and then just move the appropriate half into the docfrag? This would also let you get rid of the code right below the commment: // If node wasn't cloned above, it must be completely contained [...] Otherwise I like ExtractContents() very much, nicely done. SurroundContents() looks perfecto. The only change I really demand is the first comment I made about duplicated code. The rest I leave up to your judgement. r=mozeditor

Here's patch Rev 2.1 which addresses mozeditor's main concerns. > In nsRange::SplitDataNode() after you have the comment: > // Get the length of the data in aNode. > you duplicate the GetDataLengthInDataNode() code. Call > GetDataLengthInDataNode() instead. Done. Thanks for catching that, I split out that code from SplitDataNode() to make a util routine but forgot to call it from there. > By the way do we really want to Clone() the node in SplitDataNode()? In the > case of really huge text nodes (imagine just a big ole text docuemnt that is > all in one text node) won't this cause twice the peak memory need? Is there a > way we can create an empty node and split the underlying data without copying > (I doubt it but I ask anyway)? I used Clone() as a shortcut so that I didn't have to differenciate between the node types. The alternative would be to use the Substring() method on the CharacterData interface, I'd have to lookup what's available for Processing Instructions, figure out what type of node it we have, create a new node of the same type, then try to insert the substring ... because I don't want to delete the original string data until I'm positive everything has worked, we will end up with 3 copies of the data that is supposed to be split off. > Failing that, shouldn't we calculate which > side of the split has less data and copy that side into a new node? I can't do that since the range spec specifies that the original node must remain in the document. The original node could contain the side with the least amount of data. > Put a blank line before GetDataLengthInDataNode definition. Done > In nsresult nsRange::ExtractContents(nsIDOMDocumentFragment** aReturn): > where you have the while loop and check for (node == startContainer || node == > endContainer) and do all the cloning... why not just make use of the > SplitDataNode() function you already wrote to split the data nodes, and then > just move the appropriate half into the docfrag? This would also let you get > rid of the code right below the commment: > // If node wasn't cloned above, it must be completely contained [...] The reason I didn't use SplitDataNode() is because the start and end points of the range could both lie in the same data node. The spec is a bit fuzzy there, but I opted to behave the same as the situation with a real container ... that is, the data within the range is removed, but the container remains in tact ... splitting the data node would leave me with 2 adjacent text nodes in that case.

Comment on attachment 136231 [details] [diff] [review] Patch Rev 2.1 Here's patch Rev 2.1 which addresses mozeditor's main concerns. > In nsRange::SplitDataNode() after you have the comment: > // Get the length of the data in aNode. > you duplicate the GetDataLengthInDataNode() code. Call > GetDataLengthInDataNode() instead. Done. Thanks for catching that, I split out that code from SplitDataNode() to make a util routine but forgot to call it from there. > By the way do we really want to Clone() the node in SplitDataNode()? In the > case of really huge text nodes (imagine just a big ole text docuemnt that is > all in one text node) won't this cause twice the peak memory need? Is there a > way we can create an empty node and split the underlying data without copying > (I doubt it but I ask anyway)? I used Clone() as a shortcut so that I didn't have to differenciate between the node types. The alternative would be to use the Substring() method on the CharacterData interface, I'd have to lookup what's available for Processing Instructions, figure out what type of node it we have, create a new node of the same type, then try to insert the substring ... because I don't want to delete the original string data until I'm positive everything has worked, we will end up with 3 copies of the data that is supposed to be split off. > Failing that, shouldn't we calculate which > side of the split has less data and copy that side into a new node? I can't do that since the range spec specifies that the original node must remain in the document. The original node could contain the side with the least amount of data. > Put a blank line before GetDataLengthInDataNode definition. Done > In nsresult nsRange::ExtractContents(nsIDOMDocumentFragment** aReturn): > where you have the while loop and check for (node == startContainer || node == > endContainer) and do all the cloning... why not just make use of the > SplitDataNode() function you already wrote to split the data nodes, and then > just move the appropriate half into the docfrag? This would also let you get > rid of the code right below the commment: > // If node wasn't cloned above, it must be completely contained [...] The reason I didn't use SplitDataNode() is because the start and end points of the range could both lie in the same data node. The spec is a bit fuzzy there, but I opted to behave the same as the situation with a real container ... that is, the data within the range is removed, but the container remains in tact ... splitting the data node would leave me with 2 adjacent text nodes in that case.

Comment on attachment 136231 [details] [diff] [review] Patch Rev 2.1 - In nsRange::DeleteDataInDataNode(): + PRUint32 dataStrLen = dataStr.Length(); + PRUint32 endOffset = aOffset + aLength; + + if (endOffset > dataStrLen) + return NS_ERROR_FAILURE; + + nsAutoString leftStr, rightStr; + + if (aOffset > 0) + dataStr.Left(leftStr, aOffset); + + if(endOffset != dataStrLen) + dataStr.Right(rightStr, dataStrLen - endOffset); + + dataStr = leftStr + rightStr; + + return instr->SetData(dataStr); How about eliminating some of those string copying in favor of some string helpers? Something like: return instr->SetData(StringHead(dataStr, aOffset) + StringTail(dataStr, dataStr.Length() - (aOffset + aLength))); sr=jst with that.

Patch Rev 2.2 addresses jst's issue above. I also did 2 minor tweaks to SurroundContents(): 1. Added code to remove the children of aNewParent prior to insertion per the spec. 2. Replaced all of the code I added in previous patches, which split data nodes and inserted aNewParent, with a call to the pre-existing InsertNode() function which handles the splitting and insertion anyways. Here's the snippet of code that I added to SurroundContents(): // Spec says we need to remove all of aNewParent's // children prior to insertion. nsCOMPtr<nsIDOMNodeList> children; res = aNewParent->GetChildNodes(getter_AddRefs(children)); if (NS_FAILED(res)) return res; if (!children) return NS_ERROR_FAILURE; PRUint32 numChildren = 0; res = children->GetLength(&numChildren); if (NS_FAILED(res)) return res; nsCOMPtr<nsIDOMNode> tmpNode; while (numChildren) { nsCOMPtr<nsIDOMNode> child; res = children->Item(--numChildren, getter_AddRefs(child)); if (NS_FAILED(res)) return res; if (!child) return NS_ERROR_FAILURE; res = aNewParent->RemoveChild(child, getter_AddRefs(tmpNode)); if (NS_FAILED(res)) return res; } // Insert aNewParent at the range's start point. res = InsertNode(aNewParent); if (NS_FAILED(res)) return res;

Comment on attachment 136447 [details] [diff] [review] Patch Rev 2.2 Requesting reviews again for the tweaks I made to SurroundContents().

Comment on attachment 136447 [details] [diff] [review] Patch Rev 2.2 r=mozeditor

So patch 2.2 has r and sr and fixes peterv's test case. Kin, could you check it in and close the bug please ?

This thing has bitrotted badly. Can someone take care of this?

Updating Patch Rev 2.2, which has bit rotted, to the 05/01/2004 Trunk.

Comment on attachment 147485 [details] [diff] [review] Patch Rev 2.2 (Updated to 05/01/2004 Trunk) Hmmm ignore the last patch, some of my changes from the original patch rev 2.2, for the subtree iterator, were lost when I updated and tried to resolve conflicts from the decomtamination landing.

Comment on attachment 147485 [details] [diff] [review] Patch Rev 2.2 (Updated to 05/01/2004 Trunk) r=jfrancis

With my patch for bug 205635, I am now passing the testcases.

The original bug (implementing Range.surroundContents()) is now fixed; marking this bug accordingly.