Tracking Bug for Range Spec

VERIFIED FIXED in mozilla1.0

Status

()

Core
DOM: Core & HTML
P1
normal
VERIFIED FIXED
17 years ago
5 years ago

People

(Reporter: Dylan Schiemann, Assigned: anthonyd)

Tracking

({dom2, testcase})

Trunk
mozilla1.0
dom2, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [select][range][embed])

Attachments

(10 attachments)

(Reporter)

Description

17 years ago
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
(Reporter)

Updated

17 years ago
Priority: P3 → P5
(Reporter)

Comment 1

17 years ago
Created attachment 18612 [details]
testcase

Updated

17 years ago
Keywords: testcase

Comment 2

17 years ago
confirming (to get off unconfirmed lists)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 3

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

anthonyd
(Reporter)

Comment 4

17 years ago
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?
(Assignee)

Comment 5

17 years ago
investigating this.

anthonyd
(Assignee)

Comment 6

17 years ago
setting to p1.
anthonyd
Priority: P5 → P1
(Assignee)

Comment 7

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

anthonyd
(Assignee)

Comment 8

17 years ago
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
Summary: Range extractContents not implemented → Range cloneContents not implemented, breaks extractContents
(Reporter)

Comment 9

17 years ago
I've added the dependency to 58970, which is for cloneContents not being
implemented.
Depends on: 58970

Comment 10

17 years ago
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
Keywords: correctness
OS: Windows 2000 → All
Hardware: PC → All

Comment 11

17 years ago
finishing up range can be pushed off
Target Milestone: mozilla0.9 → mozilla0.9.1
Keywords: dom2
Component: DOM Level 2 → DOM Traversal-Range

Comment 12

17 years ago
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

17 years ago
Created attachment 27459 [details]
Patch for Range functionality

Comment 14

17 years ago
Created attachment 27507 [details]
Updated patch with test cases (had an error in surroundContent method)

Comment 15

17 years ago
Note:  new attachment above has comments in the .js file.
(Assignee)

Comment 16

17 years ago
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
(Assignee)

Comment 17

17 years ago
setting milestone to 0.9
Target Milestone: mozilla0.9.1 → mozilla0.9

Comment 18

17 years ago
Created attachment 27648 [details]
RangePatch.js file to patch the Range Object

Comment 19

17 years ago
Created attachment 27649 [details]
Test Case

Comment 20

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

Jeff.

Comment 21

17 years ago
Created attachment 27650 [details]
Clone Test Case
(Assignee)

Comment 22

17 years ago
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

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

Moving to 0.9.1.
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 24

17 years ago
So isn't this bug a dup of 58970?
(Reporter)

Comment 25

17 years ago
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.
(Assignee)

Comment 26

17 years ago
im going to add the big patch and the new snazzy test case
(Assignee)

Comment 27

17 years ago
Created attachment 34900 [details] [diff] [review]
new fancy range patch
(Assignee)

Comment 28

17 years ago
Created attachment 34901 [details]
this is the new css laced 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?
(Assignee)

Comment 30

17 years ago
Created attachment 34980 [details] [diff] [review]
new patch for kin and jst
(Assignee)

Comment 31

17 years ago
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

Comment 33

17 years ago
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.
(Assignee)

Comment 34

17 years ago
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.
Summary: Range cloneContents not implemented, breaks extractContents → Tracking Bug for Range Spec
(Assignee)

Comment 35

17 years ago
changing milestone to 0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Updated

17 years ago
Whiteboard: [select]
(Reporter)

Updated

17 years ago
Depends on: 83363

Updated

17 years ago
Whiteboard: [select] → [select][range]

Updated

17 years ago
Target Milestone: mozilla0.9.2 → mozilla1.0

Updated

17 years ago
Whiteboard: [select][range] → [select][range][embed]
(Assignee)

Updated

17 years ago
Summary: Tracking Bug for Range Spec → RANGE: Tracking Bug for Range Spec
(Assignee)

Comment 36

17 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Summary: RANGE: Tracking Bug for Range Spec → Tracking Bug for Range Spec
(Reporter)

Comment 37

17 years ago
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...
(Reporter)

Comment 38

17 years ago
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

16 years ago
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

16 years ago
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

16 years ago
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

16 years ago
Created attachment 64267 [details]
Range test case. 

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

Comment 43

16 years ago
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

16 years ago
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. 

Updated

16 years ago
Blocks: 30838

Comment 45

16 years ago
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

15 years ago
based on Kin's last comment, marking verified -- issues are tracked elsewhere
Status: RESOLVED → VERIFIED
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.