58969 - Tracking Bug for Range Spec

Status: VERIFIED FIXED

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

Description

[Dylan Schiemann]

BuildID:    20001031

extractContents DOM 2 Range method not yet implemented

Reproducible: Always
Steps to Reproduce:
1. attempt to extractContents from a Range returns not implemented error

Actual Results:  NS_ERROR_NOT_IMPLEMENTED

testcase to follow

Comment 1

[Dylan Schiemann]

Attachment: testcase

Comment 2

[Doron Rosenberg (IBM)]

confirming (to get off unconfirmed lists)

Comment 3

[anthonyd]

please, need a little help here, im not seeing this.  where is the output for 
this testcase supposed to show up?

anthonyd

Comment 4

[Dylan Schiemann]

The testcase should alert the result of extractContents.  Instead the not
implemented error is returned in the JavaScript console, preventing the testcase
from alerting the contents.  Is there a better display mechanism than alert that
would be helpful for these testcases?

Comment 5

[anthonyd]

investigating this.

anthonyd

Comment 6

[anthonyd]

setting to p1.
anthonyd

Comment 7

[anthonyd]

hahahaha, ok, after investigating, it turns out extractContents IS implemented, 
but it calls cloneContents, which is NOT implemented.  still working...

anthonyd

Comment 8

[anthonyd]

joe,
I'm adding you to the cc list so maybe you can tell me what is wrong (or left to 
be done) on the partial implementation of Range::cloneContents().  Also, 
changing summary to reflect true problem.

thanks,
anthonyd

Comment 9

[Dylan Schiemann]

I've added the dependency to 58970, which is for cloneContents not being
implemented.

Comment 10

[David Flanagan]

Dylan:

What I'd do for testcases like yours is use try/catch to catch
the exception that is being thrown, then use alert() to display it.
That way folks can see the problem without going to the JS console

Comment 11

[Joe Francis]

finishing up range can be pushed off

Comment 12

[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 13

[Jeff Yates]

Attachment: Patch for Range functionality

Comment 14

[Jeff Yates]

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

Comment 15

[Jeff Yates]

Note:  new attachment above has comments in the .js file.

Comment 16

[anthonyd]

Jeff Yates,
It doesnt look lie you set the mime type correctly when attaching the patch 
(both of them).  They look like garbage to me.  Can you reattach and specify 
text?  Thanks.

anthonyd

Comment 17

[anthonyd]

setting milestone to 0.9

Comment 18

[Jeff Yates]

Attachment: RangePatch.js file to patch the Range Object

Comment 19

[Jeff Yates]

Attachment: Test Case

Comment 20

[Jeff Yates]

Attached wrong test case, it was for the extract bug.  Attaching proper one now.

Jeff.

Comment 21

[Jeff Yates]

Attachment: Clone Test Case

Comment 22

[anthonyd]

Hey Jeff,
I'm sorry but I'm still having diffculty pulling off of bugzilla your entire 
patch.  I got hte html for it, nut i havent beena ble to pull off the js file.  
I get an error not found.  Can you just try zipping it up and attaching it, or 
emailing it to me directly?
Thanks,
anthonyd

Comment 23

[kinmoz]

I believe anthonyd has most of this working, and is currently testing things.

Moving to 0.9.1.

Comment 24

[kinmoz]

So isn't this bug a dup of 58970?

Comment 25

[Dylan Schiemann]

This bug tracks that extractContents does not work.  Presumably fixing 58970,
cloneContents, would also fix this.  However, until 58970 is fixed, this cannot
be verified.

Comment 26

[anthonyd]

im going to add the big patch and the new snazzy test case

Comment 27

[anthonyd]

Attachment: new fancy range patch

Comment 28

[anthonyd]

Attachment: this is the new css laced test case

Comment 29

[Johnny Stenback (:jst)]

I had a look at the patches and in general the look ok, but there's one general
change I'd suggest you make, there's a bunch of places that do things like this:

+    res = call_something();
+    if(NS_FAILED(res)) return res;
+    return NS_OK;

The "if (NS_FAILED(res)) return res;" part is unnecessary, simply:

+    return call_something();

does the same, and in less code (res = ...; return res; would work too if you
wanto keep the result code in a variable for debugging purposes).

Also, here's a few random comments:

At the end of nsRange::InsertNode():

  ...
  if (...) {
    ...
    return;
  } else
    ...
    return;
  }

  return;
}

'else-after-return', the 'else' part of that if is not needed since you
unconditionally return in the if case.

You don't care about the return value from these calls (there are more than one)?:

+    this->InsertNode(aN);

Looks like nsRange::SurroundContents() doesn't end with a return, this needs to
be fixed. Also, you ignore the last call in the function (this->DoSetRange())?

Nitpicks of the day:

In nsRange::SurroundContents():

+  if( (nsIDOMNode::CDATA_SECTION_NODE == tStartNodeType) ||
+     (nsIDOMNode::TEXT_NODE == tStartNodeType) )

       ^- this doesn't line up, this same pattern is found in a few places
throughout the diff.

Wanna fix some of this an attach a new patch?

Comment 30

[anthonyd]

Attachment: new patch for kin and jst

Comment 31

[anthonyd]

adding kin and jst to the cc list for r= and sr=
new patch guy with most of jst's changes
anthonyd

Comment 32

[Johnny Stenback (:jst)]

A couple of more (non critical) comments...

In nsRange::CopyContents(), towards the bottom of the NODE_BEFORE_AND_AFTER case
theres:

+        clonedNode =  do_QueryInterface(tText);

You don't need to QI here, clonedNode is an nsIDOMNode and tText is a
nsIDOMText, which inherits nsIDOMNode so a simple assignment would be cheaper here.

Also, I still see code that does:

+      if(NS_FAILED(res)) 
+        return res;
+      return NS_OK;

where return res is all that's needed, and there are still some indentation
inconsistencies in the diff...

I don't require that you make the above changes (except for the unnecessary QI),
but I'd encourage you to at least look at them, no need to attach a new diff...

sr=jst

Comment 33

[kinmoz]

Questions/Comments ...


CloneContents() comments:

  -- Is it really neccessary to query interface clonedNode to clonedFrag again?
     I see this being done in 2 places:

    nsCOMPtr<nsIDOMNode>clonedNode = do_QueryInterface(clonedFrag);
    CopyContents(commonAncestor, clonedNode, this);
    clonedFrag = do_QueryInterface(clonedNode);

  -- No error checking is being done when calling CopyContents().


InsertNode() comments:

  -- Spec says that if the startContainer is in a text
     node, that the textnode should be split at that
     point and the aN node inserted between the split
     nodes. Right now we put aN before the startContainer.

  -- tSCParentNode->InsertBefore(aN, tSCParentNode, getter_AddRefs(tResultNode))
     is not correct right? you can't do a parent.insertBefore(newNode, parent),
     since the 2nd arg has to be a child of parent or null?

  -- Why do we care what type aN is? That is, why
     is aN == textnode so special?


SurroundContents() comments:

  -- After you call SplitText(), it's probably a good
     idea to set tStartOffset = 0, just in case any
     code below the split references tStartOffset.

  -- If one/both of our range boundary points are in textnodes
     shouldn't we check to see if a split is even neccessary
     before calling SplitText? That is if startOffset==0 or
     endOffset==length is there a need to split?

  -- GetCommonAncestorContainer() uses mStart/EndOffset
     and mStart/EndContainer to figure out what the common
     ancestor is, are we sure that after we've split the
     start/end textnodes that Range Gravity has not reset
     mStart/EndOffset and mStart/EndContainer to some
     unexpected place?

  -- Spec says you should make sure to clear any children
     aN might have before it is inserted.

  -- We're not checking for an error after calling ExtractContents().

  -- Spec says you should throw an error if your range partially selects
     a non-text node. I don't see any checks for this ... or are we
     relying on ExtractContents to tell us this?

  -- After we ExtractContents(), why can't we just rely on Range
     Gravity, and call InsertNode for all cases. I'm not exactly
     sure why we have to handle a textnode case and a non-textnode
     cases seperately?

  -- Spec says that when we are all done extracting and inserting,
     that we should do a selectNode(aN).

CopyContents() comments:

  -- Looking at the way CloneContents() is implemented and some of
     the cases in CopyContents() for example NODE_BEFORE_AND_AFTER
     and NODE_BEFORE, it looks like we may not copy everything we
     need to since the code only copies if the partially selected
     node is a textnode/cdata.

  -- Are we relying on CopyContents to detect partial selections
     for SurroundContents? If so, this may interfere with partial
     selections, which are allowed for CloneContents.

  -- We're also not checking errors for CopyContents() calls in
     the NODE_AFTER case.

Comment 34

[anthonyd]

i checked in the fix for clonContents, and Im turning this bug into a tracking 
bug for further range spec issues that either exist, or will exist.

Comment 35

[anthonyd]

changing milestone to 0.9.2

Comment 36

[anthonyd]

closing this bug, becaue it was stupid idea in the first place.  I will open a 
new bug for surroundContents and a another bug for extractContents.  I will 
paste kins comments and attacht he patch there too.

anthonyd

Comment 37

[Dylan Schiemann]

I don't think tracking bugs are a bad idea, because it lets people like me that
are interested in the Range follow the progress without having to be added as a
cc to every new Range bug...

Comment 38

[Dylan Schiemann]

Anthony, it looks like http://bugzilla.mozilla.org/show_bug.cgi?id=30838 was
created as a tracking bug for Range a while back...

Comment 39

[Martin van Dijken]

This bug needs to be reopened. I can definitely crash Moz 0.9.7 on Win98 in this
manner. I don't know what fixes have been made earlier, but they're not working
any more. 

Comment 40

[Fabian Guisset]

Martin could you please say which testcase crashes your 0.9.7? I tried the two
testcases in this bug and did not crash on win2k. If you crash on one of your
own testcase, please file a new bug and add your testcase, thanks.

Comment 41

[Martin van Dijken]

Well, this is a bit of bugzilla courtesy of which I'm not sure how to handle it.
I'm quite certain this is the same problem, yet you say that the testcases found
here do not crash your moz. I'll add a new testcase here, which demonstrates
what I mean. If you don't agree with my assumption that this is the same bug let
me know. 

Comment 42

[Martin van Dijken]

Attachment: Range test case.

This test case tests most Range.???Contents methods. Crashes my Mozilla

Comment 43

[Martin van Dijken]

All right, maybe we are talking about a new bug, I don't know. 
I have the following HTML:

Dikke test<a onclick="alert('blaat')">blaat</A>
...more HTML.

When I select both a part from the first text-node and a part from the text-node
under the a-node, mozilla crashes. As long as I don't select parts or all of the
A-node, I get an exception most of the times (which is not desired behavior in
my opinion either, but that's a whole other matter). If I only select a part of
a text-node the behavior works fine. 

Guys, I'm confused...

Comment 44

[Martin van Dijken]

Can anybody please verify my statement that this bug should be reopened? I'd
really like a solution as well of course, but I can understand that that takes
time. I do find it very annoying that I haven't heard anything yet and the bug
is still resolved. 

Comment 45

[kinmoz]

So it seems I've inherited anthonyd's range bugs ... this bug has been morphed
from the original reported problem, to a tracking bug, then closed with other
bugs spun off from here, so rather then add more confusion, lets just open new
bugs and add references to them to tracking bug 30838 which Dylan mentioned above.

I opened another bug regarding Martin's reported crash. It's bug 120366, and
I've already added a ref to it in bug 30838.

Comment 46

[rubydoo123]

based on Kin's last comment, marking verified -- issues are tracked elsewhere