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

confirming (to get off unconfirmed lists)

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

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?

investigating this. anthonyd

setting to p1. anthonyd

hahahaha, ok, after investigating, it turns out extractContents IS implemented, but it calls cloneContents, which is NOT implemented. still working... 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

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

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

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

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

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

setting milestone to 0.9

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

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

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

So isn't this bug a dup of 58970?

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.

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

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?

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

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

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.

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.

changing milestone to 0.9.2

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

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...

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

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.

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.

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.

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

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...

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.

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.

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