58974 - Range surroundContents not implemented

Status: RESOLVED FIXED

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

Description

[Dylan Schiemann]

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

Comment 1

[Dylan Schiemann]

Attachment: testcase

Comment 2

[Stephen Walker]

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

Comment 3

[Dylan Schiemann]

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

Comment 4

[Keyser Sose]

Reporter is this still a problem in the latest nightlies? 

Comment 5

[Dylan Schiemann]

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

Comment 6

[Keyser Sose]

Marking NEW as per reporters comments.

Comment 7

[anthonyd]

setting milestone, and priority.
anthonyd

Comment 8

[Joe Francis]

finishing up range can be pushed off

Comment 9

[Jeff Yates]

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

Comment 10

[Jeff Yates]

Attachment: Patch for Range functionality

Comment 11

[Jeff Yates]

Attachment: Updated patch with test cases and code commenting (had an error in surroundContent method)

Comment 12

[Johnny Stenback (:jst)]

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

Comment 13

[anthonyd]

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

Comment 14

[anthonyd]

moving to mozilla 0.9
anthonyd

Comment 15

[Jeff Yates]

Attachment: JS Patch File

Comment 16

[Jeff Yates]

Attachment: Test Case

Comment 17

[rubydoo123]

moving to mozilla1.0

Comment 18

[anthonyd]

checked in

Comment 19

[anthonyd]

see 58969 for details

Comment 20

[Jeff Yates]

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

Comment 21

[Johnny Stenback (:jst)]

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

Comment 22

[Blake Ross]

Milestone reality check.

Comment 23

[anthonyd]

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

anthonyd

Comment 24

[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

Comment 25

[Martin van Dijken]

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

Comment 26

[Martin van Dijken]

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

Comment 27

[Johnny Stenback (:jst)]

Reopening to make sure we investigate this further.

Comment 28

[Marten ter Borgh]

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

Comment 29

[Caspy7]

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

Comment 30

[Fuzzy Gorilla]

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.

Comment 31

[Caspy7]

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.

Comment 32

[anthonyd]

reassigning range bug to kin.

Comment 33

[Keyser Sose]

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

Comment 34

[Dylan Schiemann]

Attachment: testcase

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

Comment 35

[Raymond Penners]

Attachment: Using surroundContents() to make the selection bold

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)

Comment 36

[Dylan Schiemann]

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.

Comment 37

[Dylan Schiemann]

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

Comment 38

[Dylan Schiemann]

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.

Comment 39

[Caspy7]

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

Comment 40

[Chris "SoopahMan" Moschini]

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

Comment 41

[kinmoz]

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

Comment 42

[kinmoz]

Attachment: Patch Rev 1 (SurroundContents() and ExtractContents() rewrite)

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

Comment 43

[kinmoz]

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.

Comment 44

[kinmoz]

Attachment: Patch Rev 1.1

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.

Comment 45

[kinmoz]

Attachment: Patch Rev 2

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.

Comment 46

[Joe Francis]

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

Comment 47

[kinmoz]

Attachment: 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 48

[kinmoz]

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 49

[Johnny Stenback (:jst)]

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.

Comment 50

[kinmoz]

Attachment: Patch Rev 2.2

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 51

[kinmoz]

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

Requesting reviews again for the tweaks I made to SurroundContents().

Comment 52

[Joe Francis]

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

r=mozeditor

Comment 53

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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 ?

Comment 54

[Giacomo Magnini]

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

Comment 55

[kinmoz]

Attachment: Patch Rev 2.2 (Updated to 05/01/2004 Trunk)

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

Comment 56

[kinmoz]

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 57

[Joe Francis]

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

r=jfrancis

Comment 58

[Alex Vincent [:WeirdAl]]

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

Comment 59

[Alex Vincent [:WeirdAl]]

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